Opened on 10/17/2018 at 08:41:43 AM
Closed on 01/10/2019 at 01:23:43 AM
Last modified on 03/13/2019 at 12:15:08 AM
#7054 closed change (fixed)
Update the adblockpluscore dependency to 5cb695da5a40, adblockplusui to f86abf2efdfd
Reported by: | greiner | Assignee: | jsonesen |
---|---|---|---|
Priority: | P2 | Milestone: | Adblock-Plus-3.5-for-Chrome-Opera-Firefox |
Module: | Platform | Keywords: | |
Cc: | sebastian, kzar, wspee, jsonesen, mjethani, hfiguiere, Ross, philll, rscott, ukacar | Blocked By: | #7027, #7149, #7150, #7151, #7181, #7191 |
Blocking: | Platform: | Unknown / Cross platform | |
Ready: | yes | Confidential: | no |
Tester: | Unknown | Verified working: | no |
Review URL(s): |
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: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.
Attachments (0)
Change History (83)
comment:1 Changed on 10/17/2018 at 08:42:49 AM by greiner
- Blocked By 7027 added
- Description modified (diff)
comment:3 Changed on 10/22/2018 at 08:05:43 AM 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)
comment:4 Changed on 10/22/2018 at 08:56:38 AM 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 on 10/23/2018 at 02:08:37 AM 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?
comment:6 Changed on 10/23/2018 at 06:32:35 AM 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 on 10/23/2018 at 03:28:25 PM 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 on 10/24/2018 at 09:21:33 PM 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 on 10/25/2018 at 10:53:21 AM 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 on 10/25/2018 at 06:59:13 PM by sebastian
- Cc mjethani added
comment:11 Changed on 10/25/2018 at 07:36:30 PM 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: ↓ 13 Changed on 11/02/2018 at 11:21:56 AM 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 on 11/02/2018 at 12:19:36 PM 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 on 11/20/2018 at 03:47:56 AM 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 on 11/20/2018 at 01:21:37 PM by greiner
- Description modified (diff)
Applied the adblockplusui change again and updated the hashes in the ticket description.
comment:16 Changed on 11/20/2018 at 01:28:42 PM by mjethani
Thanks, Thomas.
We can proceed with the dependency update now, Jon.
comment:17 Changed on 11/20/2018 at 05:20:45 PM by kzar
- Owner set to jsonesen
- Priority changed from Unknown to P2
comment:18 Changed on 11/20/2018 at 05:23:34 PM 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 on 11/20/2018 at 05:35:02 PM by greiner
comment:20 Changed on 11/20/2018 at 06:24:46 PM by jsonesen
Yeah I am on it, thanks ya'll
comment:21 follow-up: ↓ 22 Changed on 11/20/2018 at 07:21:54 PM 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 on 11/22/2018 at 03:47:13 AM 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 on 11/28/2018 at 10:21:07 PM by hfiguiere
- Cc hfiguiere added
comment:24 Changed on 11/29/2018 at 03:58:33 PM by jsonesen
- Description modified (diff)
comment:25 Changed on 11/29/2018 at 04:37:51 PM 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 on 11/29/2018 at 04:48:54 PM by jsonesen
- Blocked By 7149 added
comment:27 follow-ups: ↓ 28 ↓ 29 Changed on 11/29/2018 at 05:01:57 PM 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 on 11/29/2018 at 05:09:03 PM 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 on 11/29/2018 at 10:00:03 PM by mjethani
comment:30 Changed on 11/30/2018 at 11:34:40 AM 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 on 11/30/2018 at 02:30:51 PM by greiner
- Blocked By 7150 added
comment:32 Changed on 11/30/2018 at 02:32:00 PM 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 on 11/30/2018 at 02:45:58 PM 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 on 11/30/2018 at 02:55:00 PM 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 on 11/30/2018 at 02:56:01 PM by kzar
- Description modified (diff)
comment:36 Changed on 11/30/2018 at 02:57:23 PM by greiner
- Blocked By 7151 added
comment:37 Changed on 11/30/2018 at 02:59:27 PM 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 on 11/30/2018 at 03:05:08 PM by kzar
- Description modified (diff)
comment:39 Changed on 11/30/2018 at 03:20:15 PM by kzar
- Description modified (diff)
comment:40 Changed on 11/30/2018 at 03:56:57 PM by kzar
- Description modified (diff)
comment:41 Changed on 11/30/2018 at 04:08:08 PM by kzar
- Description modified (diff)
comment:42 Changed on 11/30/2018 at 04:09:58 PM by kzar
- Description modified (diff)
comment:43 Changed on 11/30/2018 at 04:23:28 PM 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 on 11/30/2018 at 04:29:17 PM by kzar
- Description modified (diff)
comment:45 Changed on 11/30/2018 at 04:37:05 PM by kzar
- Description modified (diff)
comment:46 Changed on 11/30/2018 at 04:39:23 PM by kzar
comment:47 Changed on 11/30/2018 at 04:43:03 PM by kzar
- Description modified (diff)
comment:48 Changed on 12/04/2018 at 01:13:11 PM by kzar
- Description modified (diff)
comment:49 Changed on 12/04/2018 at 02:41:45 PM 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:51 Changed on 12/04/2018 at 03:17:17 PM by kzar
- Description modified (diff)
comment:52 Changed on 12/04/2018 at 03:21:25 PM 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: ↓ 54 Changed on 12/06/2018 at 07:56:15 AM by kzar
Any objection if I take this one over Jon?
comment:54 in reply to: ↑ 53 ; follow-up: ↓ 55 Changed on 12/06/2018 at 08:31:12 AM 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 on 12/06/2018 at 08:42:17 AM 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.
comment:56 Changed on 12/06/2018 at 10:16:32 AM 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 on 12/06/2018 at 10:29:32 AM 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 on 12/06/2018 at 11:12:45 AM by kzar
But this is a dependency update, and your patch sets do not yet update the dependency?
comment:59 follow-up: ↓ 72 Changed on 12/08/2018 at 12:50:15 AM 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 on 12/09/2018 at 10:36:15 AM by mjethani
After this dependency update, the next branch of adblockpluschrome will perform ~30% faster than the master branch.
Just FYI
comment:61 Changed on 12/11/2018 at 10:13:51 PM by jsonesen
- Owner jsonesen deleted
comment:62 Changed on 12/11/2018 at 10:18:28 PM 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: ↓ 64 ↓ 65 Changed on 12/12/2018 at 10:40:40 AM 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 on 12/12/2018 at 04:10:29 PM 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.
comment:65 in reply to: ↑ 63 Changed on 12/12/2018 at 04:17:14 PM 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 on 12/12/2018 at 04:18:05 PM by kzar
(Whoops, sorry I didn't notice you'd already responded here as well Jon.)
comment:67 Changed on 12/12/2018 at 05:07:13 PM by mjethani
We landed a couple more changes in core so we should include those as well (changeset fac3c11b26e2):
comment:68 Changed on 12/12/2018 at 05:24:40 PM 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 on 12/12/2018 at 05:25:32 PM by jsonesen
- Description modified (diff)
comment:70 Changed on 12/12/2018 at 06:24:13 PM by kzar
- Description modified (diff)
comment:71 Changed on 12/12/2018 at 06:27:27 PM by kzar
- Description modified (diff)
comment:72 in reply to: ↑ 59 Changed on 12/12/2018 at 06:38:22 PM 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!
comment:73 Changed on 12/20/2018 at 09:54:21 AM by kzar
- Blocked By 7181 added
comment:74 Changed on 12/20/2018 at 11:15:10 AM by mjethani
Fixed #7181.
The new dependency for adblockpluscore should be hg:5cb695da5a40 git:b6ef032.
comment:75 Changed on 12/20/2018 at 11:40:12 AM by kzar
- Description modified (diff)
- Summary changed from Update the adblockpluscore dependency to fac3c11b26e2, adblockplusui to f86abf2efdfd to Update the adblockpluscore dependency to 5cb695da5a40, adblockplusui to f86abf2efdfd
comment:76 follow-up: ↓ 77 Changed on 12/21/2018 at 01:38:00 PM by 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.
comment:77 in reply to: ↑ 76 Changed on 12/21/2018 at 05:49:58 PM by 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.
comment:78 Changed on 01/04/2019 at 11:29:12 AM by kzar
- Blocked By 7191 added
comment:79 Changed on 01/09/2019 at 09:45:31 PM by abpbot
A commit referencing this issue has landed:
Issue 7054 - Update the adblockpluscore dependency to 5cb695da5a40, adblockplusui to f86abf2efdfd
comment:80 Changed on 01/10/2019 at 01:23:43 AM by sebastian
- Milestone set to Adblock-Plus-for-Chrome-Opera-Firefox-next
- Resolution set to fixed
- Status changed from reviewing to closed
comment:81 Changed on 01/10/2019 at 09:38:52 AM by kzar
- Cc Ross philll rscott ukacar added
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.
comment:82 Changed on 01/29/2019 at 04:42:38 AM by mjethani
Somehow a typo slipped by in the commit.
Jon, can you submit another patch to fix the typo?
comment:83 Changed on 03/13/2019 at 12:14:47 AM by sebastian
For reference, we filed #7354 for the regression, and fixed the typo.
Added commit hashes.