Opened 8 weeks ago

Last modified 5 hours ago

#7054 reviewing change

Update the adblockpluscore dependency to fac3c11b26e2, adblockplusui to f86abf2efdfd

Reported by: greiner Assignee: jsonesen
Priority: P2 Milestone:
Module: Platform Keywords:
Cc: sebastian, kzar, wspee, jsonesen, mjethani, hfiguiere Blocked By: #7027, #7149, #7150, #7151
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29907589/

Description (last modified by kzar)

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:fac3c11b26e2 git:06c76b9. The following relevant changes will be included:

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

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).

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

Change History (72)

comment:1 Changed 8 weeks ago by greiner

  • Blocked By 7027 added
  • Description modified (diff)

comment:2 Changed 8 weeks ago by greiner

  • Description modified (diff)

comment:3 Changed 7 weeks ago by greiner

  • Cc sebastian kzar wspee added
  • Description modified (diff)
  • Summary changed from Update adblockplusui dependency to TBD (release-2018-5.-9) to Update adblockplusui dependency to hg:106d231b728d (release-2018-5.-9)

Added commit hashes.

comment:4 Changed 7 weeks ago by kzar

  • Cc jsonesen added
  • Description modified (diff)
  • Summary changed from Update adblockplusui dependency to hg:106d231b728d (release-2018-5.-9) to Update adblockplusui and adblockpluscore dependencies for FilterStorage.subscriptions changes

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.

comment:5 Changed 7 weeks ago by 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?

Last edited 7 weeks ago by jsonesen (previous) (diff)

comment:6 Changed 7 weeks ago by 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.

comment:7 Changed 7 weeks ago by 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).

comment:8 Changed 7 weeks ago by jsonesen

  • Summary changed from Update adblockplusui and adblockpluscore dependencies for FilterStorage.subscriptions changes to Update adblockplusui, adblockpluschrome and adblockpluscore dependencies for FilterStorage.subscriptions changes

comment:9 Changed 7 weeks ago by greiner

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

comment:10 Changed 7 weeks ago by sebastian

  • Cc mjethani added

comment:11 Changed 7 weeks ago by 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.

comment:12 follow-up: Changed 6 weeks ago by greiner

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

comment:13 in reply to: ↑ 12 Changed 6 weeks ago by 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.

comment:14 Changed 3 weeks ago by 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.

comment:15 Changed 3 weeks ago by greiner

  • Description modified (diff)

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

comment:16 Changed 3 weeks ago by mjethani

Thanks, Thomas.

We can proceed with the dependency update now, Jon.

comment:17 Changed 3 weeks ago by kzar

  • Owner set to jsonesen
  • Priority changed from Unknown to P2

comment:18 Changed 3 weeks ago by kzar

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

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.

comment:19 Changed 3 weeks ago by greiner

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

comment:20 Changed 3 weeks ago by jsonesen

Yeah I am on it, thanks ya'll

comment:21 follow-up: Changed 3 weeks ago by 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.

comment:22 in reply to: ↑ 21 Changed 3 weeks ago by 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

comment:23 Changed 2 weeks ago by hfiguiere

  • Cc hfiguiere added

comment:24 Changed 2 weeks ago by jsonesen

  • Description modified (diff)

comment:25 Changed 2 weeks ago by jsonesen

  • Description modified (diff)

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.

comment:26 Changed 2 weeks ago by jsonesen

  • Blocked By 7149 added

comment:27 follow-ups: Changed 13 days ago by 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.

comment:28 in reply to: ↑ 27 Changed 13 days ago by 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

comment:29 in reply to: ↑ 27 Changed 13 days ago by 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.

comment:30 Changed 13 days ago by kzar

  • Summary changed from Update adblockplusui, adblockpluschrome and adblockpluscore dependencies for FilterStorage.subscriptions changes to Update adblockplusui and adblockpluscore dependencies for FilterStorage.subscriptions changes

comment:31 Changed 12 days ago by greiner

  • Blocked By 7150 added

comment:32 Changed 12 days ago by greiner

  • Summary changed from Update adblockplusui and adblockpluscore dependencies for FilterStorage.subscriptions changes to Update adblockplusui and adblockpluscore dependencies for FilterStorage.subscriptions and Subscription.prototype.filters changes

Updated ticket title to more accurately reflect its content.

comment:33 Changed 12 days ago by 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".

comment:34 Changed 12 days ago by kzar

  • Description modified (diff)

...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.

comment:35 Changed 12 days ago by kzar

  • Description modified (diff)

comment:36 Changed 12 days ago by greiner

  • Blocked By 7151 added

comment:37 Changed 12 days ago by 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.

comment:38 Changed 12 days ago by kzar

  • Description modified (diff)

comment:39 Changed 12 days ago by kzar

  • Description modified (diff)

comment:40 Changed 12 days ago by kzar

  • Description modified (diff)

comment:41 Changed 12 days ago by kzar

  • Description modified (diff)

comment:42 Changed 12 days ago by kzar

  • Description modified (diff)

comment:43 Changed 12 days ago by kzar

  • Description modified (diff)
  • Summary changed from Update adblockplusui and adblockpluscore dependencies for FilterStorage.subscriptions and Subscription.prototype.filters changes to Update the adblockpluscore dependency to c1f377857d7f, adblockplusui to cc4d5ca74953

comment:44 Changed 12 days ago by kzar

  • Description modified (diff)

comment:45 Changed 12 days ago by kzar

  • Description modified (diff)

comment:46 Changed 12 days ago by 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.

comment:47 Changed 12 days ago by kzar

  • Description modified (diff)

comment:48 Changed 8 days ago by kzar

  • Description modified (diff)

comment:49 Changed 8 days ago by greiner

  • Description modified (diff)
  • Summary changed from Update the adblockpluscore dependency to c1f377857d7f, adblockplusui to cc4d5ca74953 to Update the adblockpluscore dependency to c1f377857d7f, adblockplusui to f86abf2efdfd

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

comment:50 Changed 8 days ago by kzar

  • Ready set

Looks good, thanks!

comment:51 Changed 8 days ago by kzar

  • Description modified (diff)

comment:52 Changed 8 days ago by kzar

  • Summary changed from Update the adblockpluscore dependency to c1f377857d7f, adblockplusui to f86abf2efdfd to Update the adblockpluscore dependency to e9f07f79bf07, adblockplusui to f86abf2efdfd

comment:53 follow-up: Changed 7 days ago by kzar

Any objection if I take this one over Jon?

comment:54 in reply to: ↑ 53 ; follow-up: Changed 7 days ago by 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.

comment:55 in reply to: ↑ 54 Changed 7 days ago by 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.

Last edited 7 days ago by kzar (previous) (diff)

comment:56 Changed 7 days ago by 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.

comment:57 Changed 7 days ago by 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.

comment:58 Changed 7 days ago by kzar

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

comment:59 follow-up: Changed 5 days ago by sebastian

  • Description modified (diff)

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.

comment:60 Changed 4 days ago by mjethani

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

Just FYI

Last edited 4 days ago by mjethani (previous) (diff)

comment:61 Changed 26 hours ago by jsonesen

  • Owner jsonesen deleted

comment:62 Changed 26 hours ago by 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.

comment:63 follow-ups: Changed 13 hours ago by 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?

comment:64 in reply to: ↑ 63 Changed 8 hours ago by 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.

Last edited 8 hours ago by jsonesen (previous) (diff)

comment:65 in reply to: ↑ 63 Changed 8 hours ago by kzar

  • Owner set to 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 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

comment:66 Changed 8 hours ago by kzar

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

comment:67 Changed 7 hours ago by 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)

comment:68 Changed 7 hours ago by jsonesen

  • Description modified (diff)
  • Summary changed from Update the adblockpluscore dependency to e9f07f79bf07, adblockplusui to f86abf2efdfd to Update the adblockpluscore dependency to fac3c11b26e2, adblockplusui to f86abf2efdfd

comment:69 Changed 7 hours ago by jsonesen

  • Description modified (diff)

comment:70 Changed 6 hours ago by kzar

  • Description modified (diff)

comment:71 Changed 6 hours ago by kzar

  • Description modified (diff)

comment:72 in reply to: ↑ 59 Changed 5 hours ago by 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!

Note: See TracTickets for help on using tickets.