Opened 17 months ago

Closed 17 months ago

Last modified 16 months ago

#6784 closed change (fixed)

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

Reported by: hfiguiere Assignee: hfiguiere
Priority: P2 Milestone: Adblock-Plus-3.3-for-Chrome-Opera-Firefox
Module: Platform Keywords:
Cc: mjethani, sebastian, kzar, saroyanm, wspee, greiner Blocked By:
Blocking: #6782, #6802 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29833597/

Description (last modified by mjethani)

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

Change History (45)

comment:1 Changed 17 months ago by hfiguiere

  • Description modified (diff)

comment:2 Changed 17 months ago by hfiguiere

  • Description modified (diff)
  • Summary changed from Upgrade adblockplus core for anti-circumvention and snippets (rev TBD) to Upgrade adblockplus core for anti-circumvention and snippets hg:ba2fb17a1dc1

comment:3 Changed 17 months ago by hfiguiere

  • Description modified (diff)

comment:4 Changed 17 months ago by hfiguiere

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

comment:5 Changed 17 months ago by sebastian

  • Cc saroyanm wspee added

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.

comment:6 Changed 17 months ago by wspee

  • Cc greiner added

comment:7 Changed 17 months ago by hfiguiere

  • Description modified (diff)
  • Summary changed from Upgrade adblockplus core for anti-circumvention and snippets hg:ba2fb17a1dc1 to Upgrade adblockplus core for anti-circumvention and snippets hg:5f851931ffea

comment:8 Changed 17 months ago by hfiguiere

  • Description modified (diff)
  • Summary changed from Upgrade adblockplus core for anti-circumvention and snippets hg:5f851931ffea to Upgrade adblockplus core for anti-circumvention and snippets hg:bec7c35902d3

comment:9 Changed 17 months ago by hfiguiere

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:10 Changed 17 months ago by hfiguiere

  • Owner set to hfiguiere

comment:11 Changed 17 months ago by hfiguiere

  • Description modified (diff)

comment:12 Changed 17 months ago by greiner

See also duplicate ticket #6753.

comment:13 Changed 17 months ago by hfiguiere

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

comment:14 Changed 17 months ago by hfiguiere

  • Description modified (diff)

comment:15 Changed 17 months ago by hfiguiere

  • Description modified (diff)

comment:16 Changed 17 months ago by hfiguiere

  • Description modified (diff)

comment:17 Changed 17 months ago by 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.

comment:18 Changed 17 months ago by mjethani

  • Description modified (diff)
  • Summary changed from Upgrade adblockplus core for anti-circumvention and snippets hg:bec7c35902d3 to Upgrade adblockplus core for anti-circumvention and snippets hg:8e466288a0c3

comment:19 Changed 17 months ago by mjethani

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

comment:20 Changed 17 months ago by kzar

  • Summary changed from Upgrade adblockplus core for anti-circumvention and snippets hg:8e466288a0c3 to Update adblockpluscore and adblockplusui dependencies for snippet filters, and the anti-circumvention filter list

comment:21 Changed 17 months ago by kzar

  • Description modified (diff)

comment:22 Changed 17 months ago by kzar

  • Description modified (diff)

comment:23 Changed 17 months ago by hfiguiere

  • Description modified (diff)

comment:24 Changed 17 months ago by mjethani

  • Blocking 6782 added

comment:25 Changed 17 months ago by 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?

comment:26 follow-up: Changed 17 months ago by 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.

comment:27 Changed 17 months ago by hfiguiere

  • Description modified (diff)

comment:28 Changed 17 months ago by kzar

  • Description modified (diff)

comment:29 Changed 17 months ago by kzar

  • Description modified (diff)

comment:30 Changed 17 months ago by mjethani

  • Description modified (diff)

comment:31 Changed 17 months ago by 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.

comment:32 Changed 17 months ago by hfiguiere

  • Description modified (diff)

comment:33 Changed 17 months ago by mjethani

  • Description modified (diff)

comment:34 follow-up: Changed 17 months ago by mjethani

  • Description modified (diff)

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.

comment:35 in reply to: ↑ 34 ; follow-up: Changed 17 months ago by 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."

comment:36 Changed 17 months ago by mjethani

  • Description modified (diff)

comment:37 in reply to: ↑ 35 Changed 17 months ago by 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.

Last edited 17 months ago by mjethani (previous) (diff)

comment:38 Changed 17 months ago by greiner

  • Blocking 6802 added

comment:39 in reply to: ↑ 26 Changed 17 months ago by 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.

comment:40 Changed 17 months ago by 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.

comment:41 Changed 17 months ago by kzar

  • Priority changed from Unknown to P2
  • Ready set

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.

comment:42 Changed 17 months ago by mjethani

Alright, makes sense.

comment:44 Changed 17 months ago by hfiguiere

  • Milestone set to Adblock-Plus-for-Chrome-Opera-Firefox-next
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:45 Changed 16 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

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

Note: See TracTickets for help on using tickets.