Update the adblockpluscore dependency to 5cb695da5a40, adblockplusui to f86abf2efdfd

[greiner]

Background

We haven't updated the adblockpluscore, nor adblockplusui dependencies
in the next branch for some time. We'd like to do that now, being careful to make the corresponding changes to the adblockpluschrome code.

What to change

Update the adblockpluscore dependency to hg:5cb695da5a40 git:b6ef032. The following relevant changes will be included:

• ​Issue 7094 - Encapsulate management of subscription filters
• ​Issue 6994 - Use shortcut matching for location-only filters
• ​Issue 7074 - Keep filter objects instead of text
• ​Issue 6940 - Use underscore prefixes in lib/matcher.js
• ​Issue 7016 - Convert serialization functions into generators
• ​Issue 7003 - Look up whitelist filter only if URL is blocked
• ​Issue 6891, 6741 - Rename FilterStorage to filterStorage
• ​Issue 7052 - Use string-based matching for literal patterns
• ​Issue 7046 - Defer caching of domain maps
• ​Issue 6857 - Do not serialize empty special subscriptions
• ​Issue 6856 - Remove FilterStorage.moveSubscription
• ​Issue 6992 - Remove keyword-by-filter map
• ​Issue 7089 - Match filters by type for non-default types
• ​Issue 7162 - Add isWhitelisted to CombinedMatcher
• ​Issue 7181 - Keep a keyword-by-filter map after all

Update the adblockplusui dependency to hg:f86abf2efdfd git:1660877. The following relevant changes will be included:

• ​Issue 7150 - Adapted code to work with latest changes to Subscription.prototype.filters
• ​Issue 7151 - Renamed FilterStorage to filterStorage
• ​Issue 7027 - Adapted code to work with latest changes to FilterStorage.subscriptions

Adjust the code that uses FilterStorage, to use filterStorage instead. See the integration notes of 6891 for more details.

Adjust the code which uses filterStorage.subscriptions to except a generator.See the integration notes of 6856 for more details.

Adjust the code that uses Subscription.prototype.filters. See the integration notes in 7094.

Adjust the code which uses the blacklist or whitelist properties of CombinedMatcher. See the integration notes of 6940.

Consider how the behaviour of CombinedMatcher.prototype.matchesAny has been tweaked, and adjust the coresponding code accordingly. See the integration notes of 7003.

Ensure that the hasFilter, getKeywordForFilter and isSlowFilter methods of CombinedMatcher are no longer used, since they were removed. Note: There is a new isSlowFilter function in the matcher module which can be used instead. See the integration notes of 6992.

Hints for testers

• Verify that request blocking and element hiding still work correctly (#6992, #7046, #7052, #6940, #7074, #6994, #7089).
• Verify that custom filters and subscriptions are stored properly, even after the browser is restarted (#6856, #6857, #7094, #6891, #6741, #7016, #7027).
• Ensure that whitelisting still works properly (#7003, #7089, #7162).
• Check the options page works correctly, including adding/removing subscriptions and custom filters (#7150, #7151).
• Check that anti-adblock notifications still work correctly (#7150, #7151).
• Ensure that after a custom filter is removed, it is really removed (#7181).

Many of the linked issues have a "Hints for testers" section with more details, I highly recommend reading through those as well.

[greiner]

Added commit hashes.

[kzar]

Won't ​these changes needs to be done at the same time? If so, please could you update this issue and your code review Jon?

Please could you also check through which adblockpluscore and adblockplusui changes are included by these dependency updates, and check if any other things need to be tested, or any other adblockpluschrome changes need to be made at the same time.

[jsonesen]

Yeah, this can include those changes as well.

I'm finishing for the day in a bit so, I will do it tomorrow.

Should it include all changes up to the tip of the next bookmark?

[greiner]

A quick heads-up: Due to ongoing QA for the 3.4 release we're going to land at least one bug fix in master so, unless we want to include this dependency update in 3.4, we'll have to backout #7027 and commit it again after the 3.4 release is out since, unlike on git, we're only operating on a single Mercurial branch.

So please keep that in mind when working on this dependency update and sorry about the trouble.

[sebastian]

So it seems the adblockplusui dependency needs to be updated for #7027, when the adblockpluscore dependency is updated for #6856 (which won't be included in the 3.4 release). So yeah, I think you have to back out #7027 for now, if you want to update the adblockplusui dependency again before the 3.4 release (in order to get another bug fix into the release).

[greiner]

The change has been backed out (see ​01d1a648c42a) and I can apply it again as soon as 3.4 is out the door.

[mjethani]

If I understand correctly, in the long run the way to avoid this would be to maintain a next branch in adblockplusui as well.

[greiner]

Either that or we switch over to git where we already have various branches set up.

[kzar]

Replying to greiner:

Either that or we switch over to git where we already have various branches set up.

Or even better, combine the adblockpluschrome + adblockplusui repositories so this whole class of problems goes away... _and_ switch to Git :p.

[mjethani]

Can we do this dependency update now? It's getting harder to test any further changes in core, including new snippets.

Here we would have to update the dependencies for both UI and core at the same time, I believe there's a ​patch in progress for this, but it needs the update from UI.

[greiner]

Applied the adblockplusui change again and updated the hashes in the ticket description.

[mjethani]

Thanks, Thomas.

We can proceed with the dependency update now, Jon.

[kzar]

Thanks for updating the hashes in the description.

Let me know when you've checked through the included changes for integration notes, and updated the testing instructions Jon and I'll mark the issue as Ready.

[greiner]

Yep, the integration notes from #6856 are what I based the changes in #7027 on.

[jsonesen]

Yeah I am on it, thanks ya'll

[mjethani]

Jon, you could look at #6607 for what goes into a dependency update ticket. Basically we list down all the changes since the last dependency update, then each of those tickets must have a "Hints for testers" section with proper testing hints. If not, the same should be included in the dependency update ticket (this one), but normally one would just nudge the developers who worked on the issue to update their tickets with the testing hints.

[jsonesen]

Replying to mjethani:

Jon, you could look at #6607 for what goes into a dependency update ticket. Basically we list down all the changes since the last dependency update, then each of those tickets must have a "Hints for testers" section with proper testing hints. If not, the same should be included in the dependency update ticket (this one), but normally one would just nudge the developers who worked on the issue to update their tickets with the testing hints.

Ack, thanks for the details it's really helpful

[jsonesen]

I messed up the last ticket update. There have been a fair number of changes and we might as well update the dependency to the current adblockpluscore tip. This will require adjustments to the UI for #7094.

[greiner]

Which changes in particular in #7094 are you referring to? Because that ticket has an empty "What to change" section and no integration notes. Plus, the ticket description suggests that Subscription.filters will return an array of filter texts whereas the commit that has landed so far appears to implement it as a generator that yields filter objects.

[jsonesen]

Replying to greiner:

Which changes in particular in #7094 are you referring to?

My bad: ​https://hg.adblockplus.org/adblockpluscore/rev/b0170eda8137 this is the particular commit I am referring

[mjethani]

Replying to greiner:

Which changes in particular in #7094 are you referring to? Because that ticket has an empty "What to change" section and no integration notes.

This is fixed now.

[greiner]

Updated ticket title to more accurately reflect its content.

[greiner]

I noticed that in hg:e9f07f79bf07 git:2dd3c99 both hashes refer to the same commit but the first to the one on "next" and the latter to the one on "master".

[kzar]

...both hashes refer to the same commit but the first to the one on "next" and the latter to the one on "master"

That's right, I noticed too. I'm just fixing up the issue for Jon now, bear with me.

[greiner]

Turns out that this dependency update also brings in https://issues.adblockplus.org/ticket/6891#comment:7 so I've created #7151 for the UI-related changes necessary for that.

[kzar]

Phew, I'm more or less happy with this issue now! Thanks to Thomas for double-checking it for me!

Once the changes for #7150 and #7151 have landed to adblockplusui, I will update the description again to include those commits and the final adblockplusui hash! Then I will mark this Ready.

[greiner]

Both UI changes are done so I updated the ticket description accordingly.

[kzar]

Looks good, thanks!

[kzar]

Any objection if I take this one over Jon?

[jsonesen]

Replying to kzar:

Any objection if I take this one over Jon?

No, but I have been waiting for you to review my latest patch set for almost week and now you want to take it.

[kzar]

Replying to jsonesen:

Replying to kzar:

Any objection if I take this one over Jon?

No, but I have been waiting for you to review my latest patch set for almost week and now you want to take it.

Well, I added a comment to the review 5 or 6 days ago and you only just responded to that now.

If you want to carry on with this, then by all means do so, I have no strong desire to do this myself. Let me know on the review when the changes are ready for review, e.g. the dependency update is included along with the other required changes mentioned here, then I'll take a look.

[mjethani]

I'll just add that it's becoming really important to get this done for further development in adblockpluscore, because every time somebody uploads a patch for review it must be tested against the extension, and right now the only way to test it is to make some or all of these changes manually.

[jsonesen]

My bad for not responding to the comments I was certain that I had. I am also aware that this is important which is why I uploaded patch sets the same day of the last group of comments. I'm on vacation this week and planned to still work on it but never realized I failed to comment. If it needs to get done now then just go for it, but it's mostly done already.

[kzar]

But this is a dependency update, and your patch sets do not yet update the dependency?

[sebastian]

I updated the issue description to only reflect the changes that are new compared to the previous version of the dependencies used as of the current master branch and the last release.

[mjethani]

After this dependency update, the next branch of adblockpluschrome will perform ~30% faster than the master branch.

Just FYI

[jsonesen]

Sorry for the holdup, this was my bad for being weak on the review and just missing obvious stuff. I apologize also for not letting someone take over sooner. Won't happen again.

[kzar]

I was under the impression you felt pretty strongly about finishing this Jon, and you've already been working on it for some time. Are you sure you don't want to finish what you started?

Also, if someone's to take this over it would be useful to know how you've tested your changes so far. I did ask in the code review, but I'll quote it here in case it helps too:

Please can you confirm you tested the extension still builds, the tests still
pass and that the code paths you touched still work correctly?

[jsonesen]

Replying to kzar:

I was under the impression you felt pretty strongly about finishing this Jon, and you've already been working on it for some time. Are you sure you don't want to finish what you started?

I do feel like I want to finish this, I have been on it for a while and put quite some time into it (even if a chunk was me being lame and forgetting to tell you all I uploaded. Which I thought I fixed.) Either way, I don't want to annoy anyone by holding this up longer.

Please can you confirm you tested the extension still builds, the tests still
pass and that the code paths you touched still work correctly?

So far I have tested that all the test scripts are passing, but failed to properly test the devtools panel (which I believe is what has been holding me up) I have built the extension and done a fair amount of browsing on Firefox and Chrome and the changes currently on the review seem to be working with the exception of devtools.

[kzar]

Replying to kzar:

I was under the impression you felt pretty strongly about finishing this Jon, and you've already been working on it for some time. Are you sure you don't want to finish what you started?

I spoke with Jon in IRC, and he does indeed want to finish this. While I can understand the desire to get this pushed quickly, and I agree that it has been progressing kind of slowly, I think it's only fair he's allowed to finish. Assigning this back to you now Jon, please could you reopen your codereview?

...it would be useful to know how you've tested your changes so far. I did ask in the code review, but I'll quote it here in case it helps too:

He also gave me some more information about how the changes were tested so far:

<jonsonesen> Also, I had tested using the test pages then also running devtools locally in Firefox and chrome, it seemed things were working so
<jonsonesen> I thought I wa in the clear but forgot to add exceptions/block items from inside devtools

[kzar]

(Whoops, sorry I didn't notice you'd already responded here as well Jon.)

[mjethani]

We landed a couple more changes in core so we should include those as well (changeset ​fac3c11b26e2):

1. Filter matching should be faster now (#7089)
2. You can use defaultMatcher.isWhitelisted in lieu of the old whitelist property (no need to access the private _whitelist property) (#7162)

[kzar]

Replying to sebastian:

I updated the issue description to only reflect the changes that are new compared to the previous version of the dependencies used as of the current master branch and the last release.

I just noticed this, nice one thanks!

[mjethani]

Fixed #7181.

The new dependency for adblockpluscore should be hg:5cb695da5a40 git:b6ef032.

[mjethani]

We've landed the change for #7178. While this is not necessary to include in this dependency update, it may not be such a bad idea either.

[kzar]

Replying to mjethani:

We've landed the change for #7178. While this is not necessary to include in this dependency update, it may not be such a bad idea either.

Perhaps we should stick with the revision of adblockpluscore we have for now, unless we find more regressions. I agree it's nice to get the latest and greatest, but I worry that we'll never land anything if we keep updating revision.

[abpbot]

A commit referencing this issue has landed:
​Issue 7054 - Update the adblockpluscore dependency to 5cb695da5a40, adblockplusui to f86abf2efdfd

[kzar]

Heads up, this one is a pretty huge change that will need plenty of testing. I (simplistically) summarised what to test here in the "Hints for testers" section, but I recommend also checking the testing hints for the linked issues as well.

[mjethani]

Somehow a ​typo slipped by in the commit.

Jon, can you submit another patch to fix the typo?

[sebastian]

For reference, we filed #7354 for the regression, and fixed the typo.