Update adblockpluscore and adblockplusui dependencies for snippet filters, and the anti-circumvention filter list

[hfiguiere]

Background

We've done some work in the adblockplusui and adblockpluscore repositories towards adding the anti-circumvention filter list, and support for snippet filters. We'd now like to update the dependencies in adblockpluschrome to include that work.

Here are the changes for adblockpluscore:

• Issue #6690 - Always ignore trailing dot in document domain
• Issue #6737 - Use String.includes instead of String.indexOf
• Issue #6559 - Use Map object for known subscriptions, change SpecialSubscription.defaultsMap to Map object
• Issue #6735 - Store domains in lower case
• Issue #6619 - Qualify CSS selectors in document style sheet correctly
• Issue #6727 - Use string rather than map for single-domain filters
• Issue #6437 - Skip elements not affected by DOM mutations
• Issue #6689 - Add type property to Subscription class, added anti-CV filter list subscription
• Issue #6538, #6781 - Add the Snippets feature
• Issue #6733 - Allow empty values in filter options
• Issue #6798 - Implement hide-if-shadow-contains snippet

Here is the change for adblockplusui:

• Issue #6739 - Adapted code to work with Map implementation of FilterStorage.knownSubscriptions

What to change

Update the adblockpluscore dependency to hg:658f0229baa1 git:b64b8dc, and the adblockplusui dependency to hg:9a652397b9af git:2fe1f42.

Hint for testers

Please see the individual issues that we have linked to above for details, but here's an overview:

• Anti-circumvention subscription: on update from a previous release, the "ABP filters" list should be in the list of subscription in the UI. See also #6783.
• Test that the $rewrite filter option can be used with an empty value (#6733). Note the "=" is still required, so $rewrite= should be accepted, but not $rewrite.
• Snippets should be enabled, and snippet filters should load. Other changes need to land first as well, so you can't test those yet here. See #6781 and #6782 for the status and testing hints. In particular #6782 needs to be closed (fixed) before anything related to Snippets can be tested.
• Internal changes will require a thorough testing of the filter blocking, exceptions, etc.
  • Test domain names with and without trailing dots. See issue #6690
  • Test domain matching with uppercase and lowercase. See issue #6735
  • Element hiding emulation filters should be tested. See issue #6619

[hfiguiere]

We need to update adblockplusui at the same time to get https://issues.adblockplus.org/ticket/6739 that is required.

[sebastian]

I discussed that with Winsley before. Right after the 3.2 release, #6739 (and only this change) will be merged in the adblockplusui Mercurial repository (they do their development on GitLab and only merge stuff to Mercurial for us to integrate). Then we can update adblockpluscore to the latest revision, and simultaneously update the adblockplusui dependency without pulling in unrelated UI changes at the same time, while not breaking anytinhg (hopefully).

For reference, #6739 (in adblockplusui) and #6559 (in adblockpluscore) have a mutual depdency, i.e. if those changes aren't pulled in simultaneously things will break.

[greiner]

See also duplicate ticket #6753.

[hfiguiere]

I looked for one before even filing this one :-(

[mjethani]

The last patch for the Snippets feature has landed, but I'm also working on the implementation of a couple of "snippets" and would like to include those as well.

Please wait, I'll update this issue with the final commit hash.

[mjethani]

On second thoughts, we should update the dependency now. I have updated the issue.

[greiner]

I just noticed that the adblockpluscore dependency update also includes changes for the new anti-circumvention list. Does that mean that we also need to include the UI changes from #6731 already or will those not be necessary yet for this update?

[hfiguiere]

Ideally we would have the change, but we can do without. The anti-circumvention subscription will be opted-in and you can still disable with the existing UI, and eventually re-add it manually.

[mjethani]

@hfiguiere @kzar for the lowercase domain matching tests, we can say that we should test filters with mixed-case domain names (e.g. ExAmPlE.com##.foo) and see that they work. This is the best we can do for manual testing.

For Snippets, I'll add more detailed testing advice in #6781 and #6782.

[mjethani]

I have updated all the core issues with hints for testers.

I think we can drop the "FIXMEs" here now, the testers should look at the individual issues that we have linked to.

[kzar]

Replying to mjethani:

I have updated all the core issues with hints for testers.

Thanks for that.

I think we can drop the "FIXMEs" here now, the testers should look at the individual issues that we have linked to.

Neither the testing hints of #6733, nor #6735 give even one concrete example. Please could you add those either where the FIXME comments where in this description, or in the testing hints of the respective issues?

Here's the kind of thing I mean: "For example, given the rewrite filter ..$rewrite= ensure that requests made to https://example.com/path are redirected on to https://example.com/other-path."

[mjethani]

Replying to kzar:

Neither the testing hints of #6733, nor #6735 give even one concrete example. Please could you add those either where the FIXME comments where in this description, or in the testing hints of the respective issues?

OK, I have updated those two issues now.

In one of the issues I just copied the same example that was mentioned in the "Background" section of the issue description. I don't see the point of spoon-feeding examples to the tester (which means they will not read the rest of the issue description and not really understand the change).

In the other issue I just simply stated what the tester should already know, it should be part of the test suite already. I just don't get the point of this exercise.

[greiner]

Replying to hfiguiere:

Ideally we would have the change, but we can do without. The anti-circumvention subscription will be opted-in and you can still disable with the existing UI, and eventually re-add it manually.

FYI: We have agreed that we need to include the UI changes related to the anti-circumvention list in that same release. I've created #6802 to include the necessary changes.

[mjethani]

I'm wondering if we should go ahead and land this anyway so at least we can do some testing for snippets? There will have to be another dependency update for core, mostly likely, before code freeze, so we can do the next core and UI updates together.

[kzar]

OK, I have updated those two issues now.

Ace, thanks. This issue looks pretty good now, marking it as Ready.

I just don't get the point of this exercise.

The purpose of adding concrete examples with testing instructions is to make it easier for the testers to quickly see exactly what they're testing, and how to do it.

If you've been staring at some code for three weeks, it might be immediately obvious to you what needs to be tested. But for someone who's never even seen the issue before, maybe it's not so obvious.

Even if the issue description gives all the information, sometimes it might not be so clear what must be tested. Take the $rewrite= example. In my opinion this example:

As an example, the filter &trackingId=*$rewrite= should rewrite https://example.com/?foo=bar&trackingId=123 as https://example.com/?foo=bar.

Is much more easy to understand than this explanation:

The $rewrite option allows the filter author to replace a part of a URL with a different value. For example, /\/foo\b/$rewrite=/bar will replace /foo in the URL with /bar. The option can also be used to strip out a part of the URL. For example, /(.*)&trackingId=.*/$rewrite=$1 will strip out the &trackingId=123 part at the end of the URL. Unfortunately for stripping to work the URL pattern in the filter must be a regular expression, because the value of the option cannot be blank. If blank values were allowed in filter options, the same filter could be written as &trackingId=*$rewrite=.

Even though yes, everything you need to know is technically in that explanation.

Don't get me wrong, that explanation is really useful and I'm glad it's there. It's just a bit obtuse for the purposes of testing.

[mjethani]

Alright, makes sense.

[abpbot]

A commit referencing this issue has landed:
​Issue 6784 - Upgrade adblockpluscore to 658f0229baa1 and adblockplusui to 9a652397b9af

[Ross]

Done. The points in the testing section work as described and child tickets have been tested.

ABP 3.2.0.2103
Chrome 68 / 55 / 49 / Windows 10
Firefox 61 / 55 / 51 / Windows 10