Issue 29907589: Issue 7054 - Update the adblockpluscore dependency to 5cb695da5a40, adblockplusui to f86abf2efdfd (Closed)
|
Created:
Oct. 12, 2018, 4:56 a.m. by Jon Sonesen Modified:
Jan. 10, 2019, 9:22 a.m. CC:
Felix Dahlke Visibility:
Public. |
Patch Set 1 #
Total comments: 8
Patch Set 2 : #Patch Set 3 : Add all the changes #Patch Set 4 : Add dependency update #
Total comments: 5
Patch Set 5 : Address PS3 Changes #
Total comments: 22
Patch Set 6 : Address PS5 Comments #Patch Set 7 : Actually address comments #
Total comments: 1
Patch Set 8 : Address PS7 comment #
Total comments: 15
Patch Set 9 : Address PS8 Comments #Patch Set 10 : Rebase, address PS9 comments #
Total comments: 3
Patch Set 11 : Update dependency for adblockpluscore #
Total comments: 8
Patch Set 12 : Address PS11 Comments #
Total comments: 2
Patch Set 13 : Address PS12 Comment #
MessagesTotal messages: 99
Please file an issue to update the adblockpluscore dependency to include the related change (#6856). Take care to look through the issues for any other included changes, check for integration notes. Add a hints for testers section to the issue which guides the testers through what will need to be tested for all the included changes too. Then you can perform the dependency update along with these changes in this review. Note: We'll be pushing this to the "next" branch, so make sure to start from there instead of "master".
Added some comments inline. Just to be sure, this would be for the next branch of adblockpluschrome. The dependency update would also have to include changes to adblockplusui, otherwise the build would be broken, which means this needs to be coordinated. Also, there is no hurry to do this. https://codereview.adblockplus.org/29907589/diff/29907590/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29907589/diff/29907590/lib/subscriptionIni... lib/subscriptionInit.js:52: firstRun = FilterStorage.knownSubscriptions.size == 0; The intention was to expose the subscriptions generator function and keep knownSubscriptions as "private" to the module. Let's use `FilterStorage.subscriptionCount` here. https://codereview.adblockplus.org/29907589/diff/29907590/lib/subscriptionIni... lib/subscriptionInit.js:74: for (let subscription of [...FilterStorage.knownSubscriptions.values()]) Similarly, let's make this `FilterStorage.subscriptions()` (note: there's no need for a temporary array). https://codereview.adblockplus.org/29907589/diff/29907590/lib/subscriptionIni... lib/subscriptionInit.js:107: for (let subscription of [...subscriptions]) Not required. https://codereview.adblockplus.org/29907589/diff/29907590/lib/subscriptionIni... lib/subscriptionInit.js:267: for (let subscription of [...subscriptions]) Not required.
On 2018/10/15 23:54:47, Manish Jethani wrote: > Just to be sure, this would be for the next branch of adblockpluschrome. The > dependency update would also have to include changes to adblockplusui, otherwise > the build would be broken, which means this needs to be coordinated. Yea, Thomas has a patch on Gitlab[https://gitlab.com/ThomasGreiner/adblockplusui/commit/a950df95af6b923c2e9fcd5d97efff843e57c439] So, while there is no rush it seems relative ready. Also, I planned on working it this week a bit as well. Thanks for the comments, I will update the patch.
On 2018/10/15 23:58:01, Jon Sonesen wrote: > On 2018/10/15 23:54:47, Manish Jethani wrote: > > > Just to be sure, this would be for the next branch of adblockpluschrome. The > > dependency update would also have to include changes to adblockplusui, > otherwise > > the build would be broken, which means this needs to be coordinated. > > Yea, Thomas has a patch on > Gitlab[https://gitlab.com/ThomasGreiner/adblockplusui/commit/a950df95af6b923c2e9fcd5d97efff843e57c439] Nice. This would also require changes in lib/hitLogger.js and lib/indexedDBBackup.js (as I mentioned in #6856). While we're at it, maybe we should go ahead and rename FilterStorage to filterStorage now in core as part of #6891.
On 2018/10/16 00:04:31, Manish Jethani wrote: > On 2018/10/15 23:58:01, Jon Sonesen wrote: > > On 2018/10/15 23:54:47, Manish Jethani wrote: > > > > > Just to be sure, this would be for the next branch of adblockpluschrome. The > > > dependency update would also have to include changes to adblockplusui, > > otherwise > > > the build would be broken, which means this needs to be coordinated. > > > > Yea, Thomas has a patch on > > > Gitlab[https://gitlab.com/ThomasGreiner/adblockplusui/commit/a950df95af6b923c2e9fcd5d97efff843e57c439] > > Nice. > > This would also require changes in lib/hitLogger.js and lib/indexedDBBackup.js > (as I mentioned in #6856). > > While we're at it, maybe we should go ahead and rename FilterStorage to > filterStorage now in core as part of #6891. Can you also remove FilterStorage from lib/devtools.js please? https://hg.adblockplus.org/adblockpluschrome/file/83f0424abd71/lib/devtools.j... It's not being used, I don't see why it should be imported. Since this change is mainly about FilterStorage, it's a nice opportunity to make this change as well.
On 2018/10/15 23:54:47, Manish Jethani wrote: > Just to be sure, this would be for the next branch of adblockpluschrome. That's right, see my earlier comment.
On 2018/10/16 00:04:31, Manish Jethani wrote: > While we're at it, maybe we should go ahead and rename FilterStorage to > filterStorage now in core as part of #6891. This change has landed: https://hg.adblockplus.org/adblockpluscore/rev/58b0ccc6c289 Now we should update the dependency to this version and include the necessary changes.
See https://issues.adblockplus.org/ticket/7054#comment:3, unless I'm mistaken I think issue 7054 can be used for all these changes.
On 2018/10/22 16:14:28, kzar wrote: > See https://issues.adblockplus.org/ticket/7054#comment:3, unless I'm mistaken I > think issue 7054 can be used for all these changes. Yeah you are right :)
I am still on this, just having realized I should button up some other reviews and I ended up shelving this. I would like to finish 7015 and 6994 this week then focus on this more once they are done. Does this sound ok? I figured since this is for `next` time isn't too critical here.
I am still on this, just having realized I should button up some other reviews and I ended up shelving this. I would like to finish 7015 and 6994 this week then focus on this more once they are done. Does this sound ok? I figured since this is for `next` time isn't too critical here.
On 2018/10/24 21:41:45, Jon Sonesen wrote: > I am still on this, just having realized I should button up some other reviews > and I ended up shelving this. I would like to finish 7015 and 6994 this week > then focus on this more once they are done. Does this sound ok? I figured since > this is for `next` time isn't too critical here. Yes, I think it would be better to do it all in one dependency update, even if we have to wait for it.
https://codereview.adblockplus.org/29907589/diff/29907590/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29907589/diff/29907590/lib/subscriptionIni... lib/subscriptionInit.js:52: firstRun = FilterStorage.knownSubscriptions.size == 0; On 2018/10/15 23:54:47, Manish Jethani wrote: > The intention was to expose the subscriptions generator function and keep > knownSubscriptions as "private" to the module. Let's use > `FilterStorage.subscriptionCount` here. Acknowledged. https://codereview.adblockplus.org/29907589/diff/29907590/lib/subscriptionIni... lib/subscriptionInit.js:74: for (let subscription of [...FilterStorage.knownSubscriptions.values()]) On 2018/10/15 23:54:47, Manish Jethani wrote: > Similarly, let's make this `FilterStorage.subscriptions()` (note: there's no > need for a temporary array). Acknowledged. https://codereview.adblockplus.org/29907589/diff/29907590/lib/subscriptionIni... lib/subscriptionInit.js:107: for (let subscription of [...subscriptions]) On 2018/10/15 23:54:47, Manish Jethani wrote: > Not required. Acknowledged. https://codereview.adblockplus.org/29907589/diff/29907590/lib/subscriptionIni... lib/subscriptionInit.js:267: for (let subscription of [...subscriptions]) On 2018/10/15 23:54:47, Manish Jethani wrote: > Not required. Acknowledged.
Something appears to have gone wrong with the Rietveld upload (it's just acting up as it does every now and then), I can't see the diff for lib/whitelisting.js A few other points: 1. See https://issues.adblockplus.org/ticket/7003 integration notes. Now filter matching has been optimized for whitelisting, so it should be OK to call `defaultMatcher.matchesAny()` directly instead of calling the method on `defaultMatcher.whitelist`. Accordingly we should make this additional change in lib/whitelisting.js 2. We need to also update the dependency in the same commit, so we should add the change to the dependencies file; the idea is that after applying the patch the extension should build and work correctly with no errors 3. I believe the ticket number in the commit message should be 7054, correct me if I'm wrong The changes are looking good to me so far, there might be more places where we need to make changes, I'll just double check.
On 2018/11/30 07:53:53, Manish Jethani wrote: > The changes are looking good to me so far, there might be more places where we > need to make changes, I'll just double check. For changeset b0170eda8137 in adblockpluscore, I would like to suggest the following changes: https://github.com/adblockplus/adblockpluschrome/blob/7902e3c1dd0d33a7ed6aff5... Let updateFilters() take a Subscription object or an array of Filter objects. If the first parameter is an instance of the Subscription class, use the searchFilter() method instead of the includes() method: function updateFilters(filters, added) { let includes = filters.includes.bind(filters); if (filters instanceof Subscription) includes = filter => filters.searchFilter(filter) != -1; // further down just call includes() without any "this" object } The in onSubscriptionAdded() we just need to pass the Subscription object rather than its `filters` property. https://github.com/adblockplus/adblockpluschrome/blob/7902e3c1dd0d33a7ed6aff5... Replace with searchFilter() https://github.com/adblockplus/adblockpluschrome/blob/7902e3c1dd0d33a7ed6aff5... Replace with filters() https://github.com/adblockplus/adblockpluschrome/blob/7902e3c1dd0d33a7ed6aff5... Replace with filterCount https://github.com/adblockplus/adblockpluschrome/blob/7902e3c1dd0d33a7ed6aff5... Replace with filterCount
The issue number is 7054, not 7194. Also, the title should probably mention that you're updating the adblockpluscore and adblockplusui dependencies.
On 2018/11/30 11:35:24, kzar wrote: > The issue number is 7054, not 7194. Also, the title should probably mention that > you're updating the adblockpluscore and adblockplusui dependencies. Done.
On 2018/11/30 08:15:46, Manish Jethani wrote: > On 2018/11/30 07:53:53, Manish Jethani wrote: > > > The changes are looking good to me so far, there might be more places where we > > need to make changes, I'll just double check. > > For changeset b0170eda8137 in adblockpluscore, I would like to suggest the > following changes: > > [...] Jon, were you able to make these suggested changes?
https://codereview.adblockplus.org/29907589/diff/29960555/dependencies File dependencies (right): https://codereview.adblockplus.org/29907589/diff/29960555/dependencies#newcode5 dependencies:5: adblockplusui = adblockplusui hg:cc4d5ca74953 git:04b3033 Forgot the abpui dependency update, will reupload https://codereview.adblockplus.org/29907589/diff/29960555/lib/firefoxDataClea... File lib/firefoxDataCleanup.js (right): https://codereview.adblockplus.org/29907589/diff/29960555/lib/firefoxDataClea... lib/firefoxDataCleanup.js:57: let position = subscription.searchFilter(filter); Done. https://codereview.adblockplus.org/29907589/diff/29960555/lib/hitLogger.js File lib/hitLogger.js (right): https://codereview.adblockplus.org/29907589/diff/29960555/lib/hitLogger.js#ne... lib/hitLogger.js:117: for (let filter of subscription.filters()) Done. https://codereview.adblockplus.org/29907589/diff/29960555/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29907589/diff/29960555/lib/requestBlocker.... lib/requestBlocker.js:306: if (arg instanceof Subscription && arg.filterCount == 0) Done. https://codereview.adblockplus.org/29907589/diff/29960555/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29907589/diff/29960555/lib/subscriptionIni... lib/subscriptionInit.js:83: subscription.filterCount > 0) Done.
On 2018/12/06 11:04:50, Manish Jethani wrote: > On 2018/11/30 08:15:46, Manish Jethani wrote: > > On 2018/11/30 07:53:53, Manish Jethani wrote: > > > > > The changes are looking good to me so far, there might be more places where > we > > > need to make changes, I'll just double check. > > > > For changeset b0170eda8137 in adblockpluscore, I would like to suggest the > > following changes: > > > > [...] > > Jon, were you able to make these suggested changes? Still need to upload the update filters changes as well as the proper adblockplusui dependency, I am testing these things now. Thanks for your patience, my bad.
On 2018/12/06 23:11:34, Jon Sonesen wrote: > On 2018/12/06 11:04:50, Manish Jethani wrote: > > On 2018/11/30 08:15:46, Manish Jethani wrote: > > > On 2018/11/30 07:53:53, Manish Jethani wrote: > > Jon, were you able to make these suggested changes? > Done.
On 2018/12/06 23:39:44, Jon Sonesen wrote: > On 2018/12/06 23:11:34, Jon Sonesen wrote: > > On 2018/12/06 11:04:50, Manish Jethani wrote: > > > On 2018/11/30 08:15:46, Manish Jethani wrote: > > > > On 2018/11/30 07:53:53, Manish Jethani wrote: > > > Jon, were you able to make these suggested changes? > > > Done. Sorry, Jon, it looks like you've only made one of the several changes I recommended. Please see my comment above. https://codereview.adblockplus.org/29907589/diff/29960565/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29907589/diff/29960565/lib/devtools.js#new... lib/devtools.js:162: let includes = filters.includes.bind(filters); Sorry about my slightly misleading example earlier, this will fail if `filters` is not an array. We need something like: let includes = filters instanceof Subscription ? filter => filters.searchFilter(filter) != -1 : filters.includes.bind(filters); https://codereview.adblockplus.org/29907589/diff/29960565/lib/devtools.js#new... lib/devtools.js:162: let includes = filters.includes.bind(filters); We also need to pass the `Subscription` object to this function in `onSubscriptionAdded`. https://codereview.adblockplus.org/29907589/diff/29960565/lib/devtools.js#new... lib/devtools.js:180: if (includes(record.request.type)) This appears to be incorrect. https://codereview.adblockplus.org/29907589/diff/29960565/lib/devtools.js#new... lib/devtools.js:200: if (includes(record.request.type)) This too appears to be incorrect.
https://codereview.adblockplus.org/29907589/diff/29960565/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29907589/diff/29960565/lib/devtools.js#new... lib/devtools.js:162: let includes = filters.includes.bind(filters); On 2018/12/07 09:14:47, Manish Jethani wrote: > Sorry about my slightly misleading example earlier, this will fail if `filters` > is not an array. > > We need something like: > > let includes = filters instanceof Subscription ? > filter => filters.searchFilter(filter) != -1 : > filters.includes.bind(filters); On second thoughts, I think we should just add a new `subscription` parameter to this function. function updateFilters(subscription, filters, added) We can pass null everywhere except `onSubscriptionAdded`, in which case `filters` will be null instead.
On 2018/12/07 09:14:47, Manish Jethani wrote: > On 2018/12/06 23:39:44, Jon Sonesen wrote: > > On 2018/12/06 23:11:34, Jon Sonesen wrote: > > > On 2018/12/06 11:04:50, Manish Jethani wrote: > > > > On 2018/11/30 08:15:46, Manish Jethani wrote: > > > > > On 2018/11/30 07:53:53, Manish Jethani wrote: > > > > Jon, were you able to make these suggested changes? > > > > > Done. > > Sorry, Jon, it looks like you've only made one of the several changes I > recommended. Please see my comment above. Mm ... my bad, I got confused between the patch sets. OK, looks like we have all the changes (modulo my most recent comments). I'll also apply the patch locally and test it out.
Please could you correct the revisions in the subject? Please can you confirm you tested the extension still builds, the tests still pass and that the code paths you touched still work correctly? I think you still need to adjust `FilterStorage` in qunit/common.js and qunit/tests/indexedDBBackup.js. https://codereview.adblockplus.org/29907589/diff/29960565/dependencies File dependencies (right): https://codereview.adblockplus.org/29907589/diff/29960565/dependencies#newcode4 dependencies:4: adblockpluscore = adblockpluscore hg:e9f07f79bf07 git:466bc32 Nit: It looks like you added a space on the end of these lines? https://codereview.adblockplus.org/29907589/diff/29960565/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29907589/diff/29960565/lib/filterComposer.... lib/filterComposer.js:94: let filter = defaultMatcher.matchesAny( Previously we generated a filter if a whitelisting filter wasn't found, but now we only generate one if a blacklisting or whitelisting filter wasn't found. Do I misunderstand?
https://codereview.adblockplus.org/29907589/diff/29960565/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29907589/diff/29960565/lib/filterComposer.... lib/filterComposer.js:94: let filter = defaultMatcher.matchesAny( On 2018/12/07 10:12:53, kzar wrote: > Previously we generated a filter if a whitelisting filter wasn't found, but now > we only generate one if a blacklisting or whitelisting filter wasn't found. Do I > misunderstand? You're right, this would change the behavior. Whether it should be changed is a separate question, but let's not do it in this dependency update. We can file a separate issue for it. Either we make another change to core to support this use case or we change the behavior. For now, I think we could just continue accessing the "private" _whitelist property (as suggested in the integration notes [1]) [1]: https://issues.adblockplus.org/ticket/6940#Integrationnotes
https://codereview.adblockplus.org/29907589/diff/29960565/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29907589/diff/29960565/lib/filterComposer.... lib/filterComposer.js:94: let filter = defaultMatcher.matchesAny( On 2018/12/07 10:53:04, Manish Jethani wrote: > On 2018/12/07 10:12:53, kzar wrote: > > Previously we generated a filter if a whitelisting filter wasn't found, but > now > > we only generate one if a blacklisting or whitelisting filter wasn't found. Do > I > > misunderstand? > > You're right, this would change the behavior. Whether it should be changed is a > separate question, but let's not do it in this dependency update. We can file a > separate issue for it. Either we make another change to core to support this use > case or we change the behavior. > > For now, I think we could just continue accessing the "private" _whitelist > property (as suggested in the integration notes [1]) > > [1]: https://issues.adblockplus.org/ticket/6940#Integrationnotes We have the same problem in lib/whitelisting.js. Using _whitelist for this dependency update, sounds good enough to me. https://codereview.adblockplus.org/29907589/diff/29960565/lib/firefoxDataClea... File lib/firefoxDataCleanup.js (right): https://codereview.adblockplus.org/29907589/diff/29960565/lib/firefoxDataClea... lib/firefoxDataCleanup.js:1: /* FWIW, this file can be removed now. I don't think that we need to still support the migration path from the legacy Gecko extension to Adblock Plus 3.5. If we do so before landing this dependency update, we don't need to instruct the testers to redundantly retest this logic.
https://codereview.adblockplus.org/29907589/diff/29960565/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29907589/diff/29960565/lib/filterComposer.... lib/filterComposer.js:94: let filter = defaultMatcher.matchesAny( On 2018/12/08 00:52:26, Sebastian Noack wrote: > On 2018/12/07 10:53:04, Manish Jethani wrote: > > On 2018/12/07 10:12:53, kzar wrote: > > > Previously we generated a filter if a whitelisting filter wasn't found, but > > now > > > we only generate one if a blacklisting or whitelisting filter wasn't found. > Do > > I > > > misunderstand? > > > > You're right, this would change the behavior. Whether it should be changed is > a > > separate question, but let's not do it in this dependency update. We can file > a > > separate issue for it. Either we make another change to core to support this > use > > case or we change the behavior. > > > > For now, I think we could just continue accessing the "private" _whitelist > > property (as suggested in the integration notes [1]) > > > > [1]: https://issues.adblockplus.org/ticket/6940#Integrationnotes > > We have the same problem in lib/whitelisting.js. Using _whitelist for this > dependency update, sounds good enough to me. No, it should not be a problem in lib/whitelisting.js, because in that case the types are $document, $genericblock, $generichide, and $elemhide, all of which should return a whitelist filter even if no blocking filter exists [1] This change in core was made in fact keeping lib/whitelisting.js in mind. Now if we use `defaultMatcher` directly instead of `defaultMatcher._whitelist`, we get the benefit of caching as well. [1]: https://github.com/adblockplus/adblockpluscore/blob/5e45ca01856f981f4c99e4945...
https://codereview.adblockplus.org/29907589/diff/29960565/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29907589/diff/29960565/lib/filterComposer.... lib/filterComposer.js:94: let filter = defaultMatcher.matchesAny( On 2018/12/07 10:53:04, Manish Jethani wrote: > On 2018/12/07 10:12:53, kzar wrote: > > Previously we generated a filter if a whitelisting filter wasn't found, but > now > > we only generate one if a blacklisting or whitelisting filter wasn't found. Do > I > > misunderstand? > > You're right, this would change the behavior. Whether it should be changed is a > separate question, but let's not do it in this dependency update. We can file a > separate issue for it. Either we make another change to core to support this use > case or we change the behavior. > > For now, I think we could just continue accessing the "private" _whitelist > property (as suggested in the integration notes [1]) > > [1]: https://issues.adblockplus.org/ticket/6940#Integrationnotes FYI https://issues.adblockplus.org/ticket/7162
Thanks for the feedback! https://codereview.adblockplus.org/29907589/diff/29960565/dependencies File dependencies (right): https://codereview.adblockplus.org/29907589/diff/29960565/dependencies#newcode4 dependencies:4: adblockpluscore = adblockpluscore hg:e9f07f79bf07 git:466bc32 On 2018/12/07 10:12:53, kzar wrote: > Nit: It looks like you added a space on the end of these lines? Acknowledged. https://codereview.adblockplus.org/29907589/diff/29960565/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29907589/diff/29960565/lib/devtools.js#new... lib/devtools.js:162: let includes = filters.includes.bind(filters); On 2018/12/07 09:18:22, Manish Jethani wrote: > On 2018/12/07 09:14:47, Manish Jethani wrote: > > Sorry about my slightly misleading example earlier, this will fail if > `filters` > > is not an array. > > > > We need something like: > > > > let includes = filters instanceof Subscription ? > > filter => filters.searchFilter(filter) != -1 : > > filters.includes.bind(filters); > > On second thoughts, I think we should just add a new `subscription` parameter to > this function. > > function updateFilters(subscription, filters, added) > > We can pass null everywhere except `onSubscriptionAdded`, in which case > `filters` will be null instead. What do you think about making it an optional parameter with a default of null? `function updateFilters(subscriptions=null, filters, added)` https://codereview.adblockplus.org/29907589/diff/29960565/lib/devtools.js#new... lib/devtools.js:180: if (includes(record.request.type)) On 2018/12/07 09:14:47, Manish Jethani wrote: > This appears to be incorrect. Acknowledged. https://codereview.adblockplus.org/29907589/diff/29960565/lib/devtools.js#new... lib/devtools.js:200: if (includes(record.request.type)) On 2018/12/07 09:14:47, Manish Jethani wrote: > This too appears to be incorrect. Acknowledged. https://codereview.adblockplus.org/29907589/diff/29960565/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29907589/diff/29960565/lib/filterComposer.... lib/filterComposer.js:94: let filter = defaultMatcher.matchesAny( On 2018/12/07 10:53:04, Manish Jethani wrote: > On 2018/12/07 10:12:53, kzar wrote: > > Previously we generated a filter if a whitelisting filter wasn't found, but > now > > we only generate one if a blacklisting or whitelisting filter wasn't found. Do > I > > misunderstand? > > You're right, this would change the behavior. Whether it should be changed is a > separate question, but let's not do it in this dependency update. We can file a > separate issue for it. Either we make another change to core to support this use > case or we change the behavior. > > For now, I think we could just continue accessing the "private" _whitelist > property (as suggested in the integration notes [1]) > > [1]: https://issues.adblockplus.org/ticket/6940#Integrationnotes Acknowledged. https://codereview.adblockplus.org/29907589/diff/29960565/lib/firefoxDataClea... File lib/firefoxDataCleanup.js (right): https://codereview.adblockplus.org/29907589/diff/29960565/lib/firefoxDataClea... lib/firefoxDataCleanup.js:1: /* On 2018/12/08 00:52:26, Sebastian Noack wrote: > FWIW, this file can be removed now. I don't think that we need to still support > the migration path from the legacy Gecko extension to Adblock Plus 3.5. If we do > so before landing this dependency update, we don't need to instruct the testers > to redundantly retest this logic. Acknowledged. So I should delete?
Thanks for the feedback!
https://codereview.adblockplus.org/29907589/diff/29960565/dependencies File dependencies (right): https://codereview.adblockplus.org/29907589/diff/29960565/dependencies#newcode4 dependencies:4: adblockpluscore = adblockpluscore hg:e9f07f79bf07 git:466bc32 On 2018/12/09 15:52:09, Jon Sonesen wrote: > On 2018/12/07 10:12:53, kzar wrote: > > Nit: It looks like you added a space on the end of these lines? > > Acknowledged. The next line too. https://codereview.adblockplus.org/29907589/diff/29960565/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29907589/diff/29960565/lib/devtools.js#new... lib/devtools.js:162: let includes = filters.includes.bind(filters); On 2018/12/09 15:52:09, Jon Sonesen wrote: > On 2018/12/07 09:18:22, Manish Jethani wrote: > > On 2018/12/07 09:14:47, Manish Jethani wrote: > > > Sorry about my slightly misleading example earlier, this will fail if > > `filters` > > > is not an array. > > > > > > We need something like: > > > > > > let includes = filters instanceof Subscription ? > > > filter => filters.searchFilter(filter) != -1 : > > > filters.includes.bind(filters); > > > > On second thoughts, I think we should just add a new `subscription` parameter > to > > this function. > > > > function updateFilters(subscription, filters, added) > > > > We can pass null everywhere except `onSubscriptionAdded`, in which case > > `filters` will be null instead. > > What do you think about making it an optional parameter with a default of null? > > `function updateFilters(subscriptions=null, filters, added)` That would be fine but not very useful since the last parameter `added` must always be given. The current version is odd, let's make it like this: function updateFilters(subscription, filters, added) Also I notice that the code still bombs on the first line because `filters.includes` does not exist. In general, as part of the dependency update (or any patch), you have to test every single code path you are touching. We can't just commit something that has not been tested. https://codereview.adblockplus.org/29907589/diff/29960565/lib/firefoxDataClea... File lib/firefoxDataCleanup.js (right): https://codereview.adblockplus.org/29907589/diff/29960565/lib/firefoxDataClea... lib/firefoxDataCleanup.js:1: /* On 2018/12/09 15:52:09, Jon Sonesen wrote: > On 2018/12/08 00:52:26, Sebastian Noack wrote: > > FWIW, this file can be removed now. I don't think that we need to still > support > > the migration path from the legacy Gecko extension to Adblock Plus 3.5. If we > do > > so before landing this dependency update, we don't need to instruct the > testers > > to redundantly retest this logic. > > Acknowledged. So I should delete? If we delete it, normally it would be a separate Trac ticket and patch, and then we would rebase this patch on top of it. What do you think, Sebastian?
On 2018/12/07 10:12:54, kzar wrote: > Please could you correct the revisions in the subject? > > Please can you confirm you tested the extension still builds, the tests still > pass and that the code paths you touched still work correctly? > > I think you still need to adjust `FilterStorage` in qunit/common.js and > qunit/tests/indexedDBBackup.js. What about these comments?
That last patch set was missing local changes sorry to waste time
https://codereview.adblockplus.org/29907589/diff/29962555/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29907589/diff/29962555/lib/devtools.js#new... lib/devtools.js:162: let includes = filters instanceof Subscription ? The point of adding a `subscription` parameter was that we wouldn't have to do this check. In the current patch the `subscription` parameter is not even being used at all. Also once again clearly the code is going to fail because `filters` is null when this is called from `onSubscriptionAdded`. Like I said earlier, we should make it a practice to test any code before submitting it for review. Jon, do you mind if someone took over? Normally I would not insist but a lot of people are waiting for this dependency update and it's taking forever. At this rate it may take another couple of weeks.
https://codereview.adblockplus.org/29907589/diff/29960565/lib/firefoxDataClea... File lib/firefoxDataCleanup.js (right): https://codereview.adblockplus.org/29907589/diff/29960565/lib/firefoxDataClea... lib/firefoxDataCleanup.js:1: /* On 2018/12/10 07:48:27, Manish Jethani wrote: > On 2018/12/09 15:52:09, Jon Sonesen wrote: > > On 2018/12/08 00:52:26, Sebastian Noack wrote: > > > FWIW, this file can be removed now. I don't think that we need to still > > support > > > the migration path from the legacy Gecko extension to Adblock Plus 3.5. If > we > > do > > > so before landing this dependency update, we don't need to instruct the > > testers > > > to redundantly retest this logic. > > > > Acknowledged. So I should delete? > > If we delete it, normally it would be a separate Trac ticket and patch, and then > we would rebase this patch on top of it. > > What do you think, Sebastian? Yes, a seperate change would make sense. (Mind that I said "before landing" this change.) Not sure if we need an issue for that though.
On 2018/12/11 08:06:49, Manish Jethani wrote: > https://codereview.adblockplus.org/29907589/diff/29962555/lib/devtools.js > File lib/devtools.js (right): > > https://codereview.adblockplus.org/29907589/diff/29962555/lib/devtools.js#new... > lib/devtools.js:162: let includes = filters instanceof Subscription ? > The point of adding a `subscription` parameter was that we wouldn't have to do > this check. In the current patch the `subscription` parameter is not even being > used at all. > > Also once again clearly the code is going to fail because `filters` is null when > this is called from `onSubscriptionAdded`. > > Like I said earlier, we should make it a practice to test any code before > submitting it for review. > > Jon, do you mind if someone took over? Normally I would not insist but a lot of > people are waiting for this dependency update and it's taking forever. At this > rate it may take another couple of weeks. go for it
I would like to point out that I have regularly been uploading patch sets and not having them looked at for days at a time. But I understand the time constraint here and will try to be better moving forward
Message was sent while issue was closed.
On 2018/12/11 22:11:34, Jon Sonesen wrote: > I would like to point out that I have regularly been uploading patch sets and > not having them looked at for days at a time. But I understand the time > constraint here and will try to be better moving forward My bad, the patch sets were weak, thank for y'all's patience
Hey Guys, There is an issue with this code, it may be from either the messaging from the UI, or the matchesAny function but essentially it is not fetching the subscription info of any user defined filters (whether that be in devtools or the options page) so what happened when deleting the filters is that they are never actually deleted and exist as a subscription-less filter. I messed with it all weekend and think it's possible I could figure it out. At this point though I am not sure it's worth it as I have this week off and there is still this push to get it out ASAP. So not sure where that leaves me. Perhaps it's time to pass it off since I cannot work this week (or at least did not plan to) Thanks
Actually, it looks like the user-defined filters are in fact freed after refreshing the extension.
(Copying in Felix.) > There is an issue with this code, it may be from either the > messaging from the UI, or the matchesAny function but essentially it > is not fetching the subscription info of any user defined filters > (whether that be in devtools or the options page) so what happened > when deleting the filters is that they are never actually deleted > and exist as a subscription-less filter. I messed with it all > weekend and think it's possible I could figure it out. At this point > though I am not sure it's worth it as I have this week off and there > is still this push to get it out ASAP. Better to start with the symptoms instead of guessing what the cause is. So, please could you describe what the symptoms are of the problem you've noticed? Also, it's probably better to get the Git revision of the adblockpluscore dependency correct before testing further. If you're using Git locally you might be testing with an out of date copy of the core code. https://codereview.adblockplus.org/29907589/diff/29963555/dependencies File dependencies (right): https://codereview.adblockplus.org/29907589/diff/29963555/dependencies#newcode4 dependencies:4: adblockpluscore = adblockpluscore hg:fac3c11b26e2 git:466bc32 This is now the correct Mercurial revision, but the Git revision is out of date. Please can you update that as well? https://codereview.adblockplus.org/29907589/diff/29963555/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29907589/diff/29963555/lib/devtools.js#new... lib/devtools.js:184: if (filter) Nit: Please add braces since the body spans more than one line. (Did this pass linting?) https://codereview.adblockplus.org/29907589/diff/29963555/lib/devtools.js#new... lib/devtools.js:185: if (!includes(filter)) Woudln't it make more sense to do this: if (filter && !includes(filter)) Than what you've got so far?: if (filter) if (!includes(filter)) https://codereview.adblockplus.org/29907589/diff/29963555/lib/devtools.js#new... lib/devtools.js:224: function onFilterAdded(filter, subscription) It seems weird to add the `subscription` parameter which is then never used? (Same below.)
Please could you also put the correct Mercurial revision of the adblockpluscore dependency in the title of this review?
(FYI I've merged the master branch back into the next branch today, so you might need to rebase your changes on top of the next branch.)
https://codereview.adblockplus.org/29907589/diff/29963555/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29907589/diff/29963555/lib/devtools.js#new... lib/devtools.js:162: let includes = subscription ? It seems the parentheses are not required. For indentation, I would normally do it like this: let includes = subscription ? filter => subscription.searchFilter(filter) != -1 : filters.includes.bind(filters); https://codereview.adblockplus.org/29907589/diff/29963555/lib/devtools.js#new... lib/devtools.js:185: if (!includes(filter)) On 2018/12/18 11:29:57, kzar wrote: > Woudln't it make more sense to do this: > > if (filter && !includes(filter)) This would have to be: if (!filter || !includes(filter)) ... If the filter is null then the list doesn't include it. https://codereview.adblockplus.org/29907589/diff/29963555/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29907589/diff/29963555/lib/filterComposer.... lib/filterComposer.js:94: let filter = defaultMatcher.matchesAny( We need to use the new `isWhitelisted` function here. defaultMatcher.isWhitelisted(...) Same parameters. Note: it doesn't return a filter, it returns true or false. Please rename `filter` to `whitelisted`.
Thanks for your help guys, also still having the issue of needing to refresh the extension to remove subscriptions/user defined filters. To test this I went to the abp test pages for blocking and blocked an image in devtools and the removed the filter yet it persists. https://codereview.adblockplus.org/29907589/diff/29963555/dependencies File dependencies (right): https://codereview.adblockplus.org/29907589/diff/29963555/dependencies#newcode4 dependencies:4: adblockpluscore = adblockpluscore hg:fac3c11b26e2 git:466bc32 On 2018/12/18 11:29:56, kzar wrote: > This is now the correct Mercurial revision, but the Git revision is out of date. > Please can you update that as well? Done. https://codereview.adblockplus.org/29907589/diff/29963555/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29907589/diff/29963555/lib/devtools.js#new... lib/devtools.js:162: let includes = subscription ? On 2018/12/18 17:12:16, Manish Jethani wrote: > It seems the parentheses are not required. > > For indentation, I would normally do it like this: > > let includes = subscription ? > filter => subscription.searchFilter(filter) != -1 : > filters.includes.bind(filters); Done. https://codereview.adblockplus.org/29907589/diff/29963555/lib/devtools.js#new... lib/devtools.js:185: if (!includes(filter)) On 2018/12/18 17:12:16, Manish Jethani wrote: > On 2018/12/18 11:29:57, kzar wrote: > > Woudln't it make more sense to do this: > > > > if (filter && !includes(filter)) > > This would have to be: > > if (!filter || !includes(filter)) > ... > > If the filter is null then the list doesn't include it. Done. https://codereview.adblockplus.org/29907589/diff/29963555/lib/devtools.js#new... lib/devtools.js:224: function onFilterAdded(filter, subscription) On 2018/12/18 11:29:57, kzar wrote: > It seems weird to add the `subscription` parameter which is then never used? > (Same below.) Done. https://codereview.adblockplus.org/29907589/diff/29963555/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29907589/diff/29963555/lib/filterComposer.... lib/filterComposer.js:94: let filter = defaultMatcher.matchesAny( On 2018/12/18 17:12:16, Manish Jethani wrote: > We need to use the new `isWhitelisted` function here. > > defaultMatcher.isWhitelisted(...) > > Same parameters. > > Note: it doesn't return a filter, it returns true or false. Please rename > `filter` to `whitelisted`. Done.
Thanks for your help guys, also still having the issue of needing to refresh the extension to remove subscriptions/user defined filters. To test this I went to the abp test pages for blocking and blocked an image in devtools and the removed the filter yet it persists.
> Thanks for your help guys, also still having the issue of needing to > refresh the extension to remove subscriptions/user defined > filters. To test this I went to the abp test pages for blocking and > blocked an image in devtools and the removed the filter yet it > persists. Interesting, and you don't experience that problem before these changes are included in the extension? > Please could you also put the correct Mercurial revision of the > adblockpluscore dependency in the title of this review? > > (FYI I've merged the master branch back into the next branch today, > so you might need to rebase your changes on top of the next branch.) What about these comments? https://codereview.adblockplus.org/29907589/diff/29963555/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29907589/diff/29963555/lib/devtools.js#new... lib/devtools.js:224: function onFilterAdded(filter, subscription) On 2018/12/19 10:05:20, Jon Sonesen wrote: > On 2018/12/18 11:29:57, kzar wrote: > > It seems weird to add the `subscription` parameter which is then never used? > > (Same below.) > > Done. Thanks, but why did you add it in the first place? How come you can safely remove it again now? https://codereview.adblockplus.org/29907589/diff/29963555/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29907589/diff/29963555/lib/filterComposer.... lib/filterComposer.js:94: let filter = defaultMatcher.matchesAny( On 2018/12/19 10:05:21, Jon Sonesen wrote: > On 2018/12/18 17:12:16, Manish Jethani wrote: > > We need to use the new `isWhitelisted` function here. > > > > defaultMatcher.isWhitelisted(...) > > > > Same parameters. > > > > Note: it doesn't return a filter, it returns true or false. Please rename > > `filter` to `whitelisted`. > > Done. Nit: I have to agree with Manish, `whitelisted` or `isWhitelisted` or similar would be a better name for the variable than `hasFilter`.
On 2018/12/19 10:16:53, kzar wrote: > > Thanks for your help guys, also still having the issue of needing to > > refresh the extension to remove subscriptions/user defined > > filters. To test this I went to the abp test pages for blocking and > > blocked an image in devtools and the removed the filter yet it > > persists. > > Interesting, and you don't experience that problem before these changes > are included in the extension? Actually it would really help, Jon, if you shared the exact steps you are following so we can reproduce the issue to which you are referring. For this also we'll need a patch that applies cleanly on the next branch (you need to rebase).
https://codereview.adblockplus.org/29907589/diff/29963555/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29907589/diff/29963555/lib/devtools.js#new... lib/devtools.js:162: let includes = subscription ? On 2018/12/19 10:05:20, Jon Sonesen wrote: > On 2018/12/18 17:12:16, Manish Jethani wrote: > > It seems the parentheses are not required. > > > > For indentation, I would normally do it like this: > > > > let includes = subscription ? > > filter => subscription.searchFilter(filter) != -1 : > > filters.includes.bind(filters); > > Done. It should be aligned with the "b" in "subscription" (two spaces). (We disabled the strict checks in ESLint because they did not entirely agree with our style, otherwise ESLint would have complained.)
On 2018/12/19 10:24:57, Manish Jethani wrote: > For this also we'll need a patch that applies cleanly on the next branch > (you need to rebase). Yep, I mentioned that earlier and in the very message you quoted.
Here are some steps to reproduce the issue I am seeing: 1. Navigate to https://testpages.adblockplus.org/en/filters/blocking 2. Add the very first suggested filter: "||testpages.adblockplus.org/testcasefiles/blocking/addresscomplete/image.jpg" via the options page in the advanced tab 3. Refresh page (image is blocked) 4. Open the options page and navigate to the user-defined filters and delete the test filter. 5. Refresh the page (image is still) 6. Open the AdblockPlus Devtools Panel (the image filter still shows as an unnamed subscription) 7. Refresh extension either from chrome://extensions or from the background page 8. Filter should be removed Please note: This issue does not occur prior to these changes (on the current version of ABP and the dev build) Sorry for missing the rebase it was quite late last night (and the indentation, I could have swore it was on the 'b') I'll get on these comments right now. Also, the subscription parameter in the function definitions was erroneously added by me while trying to fix the filter issue, they were never there, although TBH it may make more sense to go by the subscription for the updateFilters method I do not know, however, that is why it was safe to remove.
On 2018/12/19 15:52:26, Jon Sonesen wrote: > Here are some steps to reproduce the issue I am seeing: > > [...] Thanks, Jon, this is very helpful. I'll try to reproduce the issue after applying the patch.
On 2018/12/19 15:57:55, Manish Jethani wrote: > On 2018/12/19 15:52:26, Jon Sonesen wrote: > > Here are some steps to reproduce the issue I am seeing: > > > > [...] > > Thanks, Jon, this is very helpful. I'll try to reproduce the issue after > applying the patch. Could you please rebase as well? Thanks.
Rebase done
On 2018/12/19 15:57:55, Manish Jethani wrote: > On 2018/12/19 15:52:26, Jon Sonesen wrote: > > Here are some steps to reproduce the issue I am seeing: > > > > [...] > > Thanks, Jon, this is very helpful. I'll try to reproduce the issue after > applying the patch. I am able to reproduce the issue. We broke this when we got rid of the keyword-by-filter map: https://issues.adblockplus.org/ticket/6992 The keyword for a filter is not always the same. In this case, the keyword while adding the filter is "addresscomplete" but the keyword while removing the filter is "testcasefiles", so the filter is never removed. We should open a ticket for Core. Meanwhile I don't think this bug should prevent the dependency update. Nice catch, Jon!
On 2018/12/19 16:49:25, Manish Jethani wrote: > I am able to reproduce the issue. > > We broke this when we got rid of the keyword-by-filter map: > > https://issues.adblockplus.org/ticket/6992 > > The keyword for a filter is not always the same. In this case, the keyword while > adding the filter is "addresscomplete" but the keyword while removing the filter > is "testcasefiles", so the filter is never removed. > > We should open a ticket for Core. Meanwhile I don't think this bug should > prevent the dependency update. Hmm, if there's been a regression in the core code, wouldn't it make sense to first fix the regression before updating the core dependency? That would save updating the dependency twice, and introducing the buggy behaviour for people building from the next branch of adblockpluschrome. > Nice catch, Jon! Yea, glad you caught that one!
On 2018/12/19 16:58:40, kzar wrote: > On 2018/12/19 16:49:25, Manish Jethani wrote: > > I am able to reproduce the issue. > > > > We broke this when we got rid of the keyword-by-filter map: > > > > https://issues.adblockplus.org/ticket/6992 > > > > The keyword for a filter is not always the same. In this case, the keyword > while > > adding the filter is "addresscomplete" but the keyword while removing the > filter > > is "testcasefiles", so the filter is never removed. > > > > We should open a ticket for Core. Meanwhile I don't think this bug should > > prevent the dependency update. > > Hmm, if there's been a regression in the core code, wouldn't it make sense to > first fix the regression before updating the core dependency? That would save > updating the dependency twice, and introducing the buggy behaviour for people > building from the next branch of adblockpluschrome. Alright, makes sense. Let's wait until the issue is fixed. I have reopened the issue and left a comment: https://issues.adblockplus.org/ticket/6992#comment:35
On 2018/12/19 17:02:12, Manish Jethani wrote: > Alright, makes sense. Let's wait until the issue is fixed. > > I have reopened the issue and left a comment: > > https://issues.adblockplus.org/ticket/6992#comment:35 Ace, thanks for doing that. FWIW I've split that out into a separate issue (#7181) and marked it as blocking this issue (#7054). I'm really happy we caught this before even updating the dependency, let alone doing a release 👍.
Hey Jon, Manish has landed that fix now so you'll need to update the adblockpluscore dependency revision again, sorry! We've kept issue 7054 up to date with the latest details.
https://codereview.adblockplus.org/29907589/diff/29965573/qunit/common.js File qunit/common.js (right): https://codereview.adblockplus.org/29907589/diff/29965573/qunit/common.js#new... qunit/common.js:29: filterStorage.subscriptions = []; Regarding this, I notice that `filterStorage.subscriptions` is no longer an array. But nor is `Subscription.knownFilters` a plain object. It seems the qunit tests are out of sync with adblockpluscore and have been so for a while. Meanwhile the tests still pass (!). In my opinion, let's file a separate issue to get the qunit tests on par with the current state of adblockpluscore.
https://codereview.adblockplus.org/29907589/diff/29965573/qunit/common.js File qunit/common.js (right): https://codereview.adblockplus.org/29907589/diff/29965573/qunit/common.js#new... qunit/common.js:29: filterStorage.subscriptions = []; On 2018/12/20 17:21:40, Manish Jethani wrote: > Regarding this, I notice that `filterStorage.subscriptions` is no longer an > array. But nor is `Subscription.knownFilters` a plain object. > ... > In my opinion, let's file a separate issue to get the qunit tests on par with the current state of adblockpluscore. If it's new since the dependency update that `filterStorage.subscriptions` is no longer an array and that `Subscription.knownFilters` is no longer a plain Object, then we should fix the tests now with the dependency update. Looking at it, `filterStorage.subscriptions` has changed with the dependency update, so we should correct this now. `Subscription.knownFilters` changed previously, so we should have already adjusted the tests for that previously. I don't mind if we fix the use of `Subscription.knownFilters` before the dependency update, or as a part of it now.
Thanks for fixing that guys, I have some things to do today but will work on it this evening and should have a patch up for you all by morning your time. https://codereview.adblockplus.org/29907589/diff/29965573/qunit/common.js File qunit/common.js (right): https://codereview.adblockplus.org/29907589/diff/29965573/qunit/common.js#new... qunit/common.js:29: filterStorage.subscriptions = []; On 2018/12/20 17:50:46, kzar wrote: > On 2018/12/20 17:21:40, Manish Jethani wrote: > > Regarding this, I notice that `filterStorage.subscriptions` is no longer an > > array. But nor is `Subscription.knownFilters` a plain object. > > ... > > In my opinion, let's file a separate issue to get the qunit tests on par with > the current state of adblockpluscore. > > If it's new since the dependency update that `filterStorage.subscriptions` is no > longer an array and that `Subscription.knownFilters` is no longer a plain > Object, then we should fix the tests now with the dependency update. > > Looking at it, `filterStorage.subscriptions` has changed with the dependency > update, so we should correct this now. `Subscription.knownFilters` changed > previously, so we should have already adjusted the tests for that previously. I > don't mind if we fix the use of `Subscription.knownFilters` before the > dependency update, or as a part of it now. I am actually working on this, I had noticed it as of the last upload I forgot to write a comment though.
FWIW the update fixed the issues I was having. Tests seem to be passing and all of the UI elements seem to be behaving normally now. Depending on urgency we could just do the qunit test update as a separate issue.
On 2018/12/21 07:40:21, Jon Sonesen wrote: > FWIW the update fixed the issues I was having. Tests seem to be passing and all > of the UI elements seem to be behaving normally now. Depending on urgency we > could just do the qunit test update as a separate issue. Great, that sounds positive, glad we're making progress. Let's fix the qunit tests here. Like we discussed, the `filterStorage.subscriptions` changes need to be made as a part of this dependency update anyway, so we might as well fix the `Subscription.knownFilters` changes in the tests while we're at it and get them working properly again.
https://codereview.adblockplus.org/29907589/diff/29967555/qunit/common.js File qunit/common.js (right): https://codereview.adblockplus.org/29907589/diff/29967555/qunit/common.js#new... qunit/common.js:32: Filter.knownFilters = Object.create(null); Is this code being used to re-initialize the objects? if so it seems I should replace this with empty lists for filterStorage._subscriptions and Filter._filters instead. Is that wrong?
Just wanted to get feedback on the next change before moving forward, but will pick it up in the morning. I am not familiar with these tests, thanks and happy belated holidays! https://codereview.adblockplus.org/29907589/diff/29967555/qunit/common.js File qunit/common.js (right): https://codereview.adblockplus.org/29907589/diff/29967555/qunit/common.js#new... qunit/common.js:32: Filter.knownFilters = Object.create(null); On 2019/01/04 00:36:28, Jon Sonesen wrote: > Is this code being used to re-initialize the objects? if so it seems I should > replace this with empty lists for filterStorage._subscriptions and > Filter._filters instead. Is that wrong? Nevermind I goofed and shouldn't be messing with those instead change it: filterStorage.knownSubscriptions = new Map(); Subscription.knownSubscriptions = new Map(); Filter.knownFilters = new Map(); Is that more correct? Then also should I still set the private properties back to an empty array?
Just wanted to get feedback on the next change before moving forward, but will pick it up in the morning. I am not familiar with these tests, thanks and happy belated holidays!
Just wanted to get feedback on the next change before moving forward, but will pick it up in the morning. I am not familiar with these tests, thanks and happy belated holidays!
Just a heads up I am having intermittent element hiding failures even on the current master it isn't always the same element hiding test which fails either I have been lookin into this today
https://codereview.adblockplus.org/29907589/diff/29967555/dependencies File dependencies (right): https://codereview.adblockplus.org/29907589/diff/29967555/dependencies#newcode4 dependencies:4: adblockpluscore = adblockpluscore hg:5cb695da5a40 git:b6ef032 Thanks for updating the dependency, but please could you also update the title of the review? https://codereview.adblockplus.org/29907589/diff/29967555/qunit/common.js File qunit/common.js (right): https://codereview.adblockplus.org/29907589/diff/29967555/qunit/common.js#new... qunit/common.js:32: Filter.knownFilters = Object.create(null); On 2019/01/04 01:12:21, Jon Sonesen wrote: > On 2019/01/04 00:36:28, Jon Sonesen wrote: > > Is this code being used to re-initialize the objects? if so it seems I should > > replace this with empty lists for filterStorage._subscriptions and > > Filter._filters instead. Is that wrong? > > Nevermind I goofed and shouldn't be messing with those instead change it: > > filterStorage.knownSubscriptions = new Map(); > Subscription.knownSubscriptions = new Map(); > Filter.knownFilters = new Map(); > > Is that more correct? Then also should I still set the private properties back > to an empty array? It seems to me that this code isn't actually used anywhere, so I've opened #7191 and #7192 to remove it.
https://codereview.adblockplus.org/29907589/diff/29967555/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29907589/diff/29967555/lib/devtools.js#new... lib/devtools.js:197: if (!includes(record.filter)) `record.filter` can be null. We should do the same check as above. i.e. `if (!record.filter || !includes(record.filter))` Or we could define `includes` as `filter => filter && subscription.searchfilter(filter) != -1`
On 2019/01/04 04:34:26, Jon Sonesen wrote: > Just a heads up I am having intermittent element hiding failures even on the > current master it isn't always the same element hiding test which fails either I > have been lookin into this today If it's an issue that exists on the current master then it's not related to this dependency update. Let's try to land this. You could open a ticket for the issue you are seeing.
On 2019/01/04 04:34:26, Jon Sonesen wrote: > Just a heads up I am having intermittent element hiding failures even on the > current master it isn't always the same element hiding test which fails either I > have been lookin into this today If it's an issue that exists on the current master then it's not related to this dependency update. Let's try to land this. You could open a ticket for the issue you are seeing.
https://codereview.adblockplus.org/29907589/diff/29967555/qunit/common.js File qunit/common.js (right): https://codereview.adblockplus.org/29907589/diff/29967555/qunit/common.js#new... qunit/common.js:32: Filter.knownFilters = Object.create(null); On 2019/01/04 11:41:18, kzar wrote: > It seems to me that this code isn't actually used anywhere, so > I've opened #7191 and #7192 to remove it. (I recommend removing all your changes to this file, that will make your life easier later when you rebase again.)
On 2019/01/04 15:48:20, kzar wrote: > https://codereview.adblockplus.org/29907589/diff/29967555/qunit/common.js > File qunit/common.js (right): > > https://codereview.adblockplus.org/29907589/diff/29967555/qunit/common.js#new... > qunit/common.js:32: Filter.knownFilters = Object.create(null); > On 2019/01/04 11:41:18, kzar wrote: > > It seems to me that this code isn't actually used anywhere, so > > I've opened #7191 and #7192 to remove it. > > (I recommend removing all your changes to this file, that will make your life > easier later when you rebase again.) Hey Jon, is there anything else holding up this patch now? I think we're almost done, you need to update the patch so we can proceed with it.
On 2019/01/07 17:01:46, Manish Jethani wrote: > On 2019/01/04 15:48:20, kzar wrote: > > https://codereview.adblockplus.org/29907589/diff/29967555/qunit/common.js > > File qunit/common.js (right): > > > > > https://codereview.adblockplus.org/29907589/diff/29967555/qunit/common.js#new... > > qunit/common.js:32: Filter.knownFilters = Object.create(null); > > On 2019/01/04 11:41:18, kzar wrote: > > > It seems to me that this code isn't actually used anywhere, so > > > I've opened #7191 and #7192 to remove it. > > > > (I recommend removing all your changes to this file, that will make your life > > easier later when you rebase again.) > > Hey Jon, is there anything else holding up this patch now? I think we're almost > done, you need to update the patch so we can proceed with it. No, nothing holding it up I'm in the middle of treatment for a chronic health condition and on Friday the side effects we're had enough to debilitate me. I just woke up and will get a new patch up once I start work.
On 2019/01/07 17:03:14, Jon Sonesen wrote: > On 2019/01/07 17:01:46, Manish Jethani wrote: > > On 2019/01/04 15:48:20, kzar wrote: > > > https://codereview.adblockplus.org/29907589/diff/29967555/qunit/common.js > > > File qunit/common.js (right): > > > > > > > > > https://codereview.adblockplus.org/29907589/diff/29967555/qunit/common.js#new... > > > qunit/common.js:32: Filter.knownFilters = Object.create(null); > > > On 2019/01/04 11:41:18, kzar wrote: > > > > It seems to me that this code isn't actually used anywhere, so > > > > I've opened #7191 and #7192 to remove it. > > > > > > (I recommend removing all your changes to this file, that will make your > life > > > easier later when you rebase again.) > > > > Hey Jon, is there anything else holding up this patch now? I think we're > almost > > done, you need to update the patch so we can proceed with it. > > No, nothing holding it up I'm in the middle of treatment for a chronic health > condition and on Friday the side effects we're had enough to debilitate me. I Sorry to hear that, Jon. Hope you feel better. > just woke up and will get a new patch up once I start work. Thanks.
Thanks guys! https://codereview.adblockplus.org/29907589/diff/29967555/dependencies File dependencies (right): https://codereview.adblockplus.org/29907589/diff/29967555/dependencies#newcode4 dependencies:4: adblockpluscore = adblockpluscore hg:5cb695da5a40 git:b6ef032 On 2019/01/04 11:41:16, kzar wrote: > Thanks for updating the dependency, but please could you also update the title > of the review? Done. https://codereview.adblockplus.org/29907589/diff/29967555/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29907589/diff/29967555/lib/devtools.js#new... lib/devtools.js:197: if (!includes(record.filter)) On 2019/01/04 14:51:46, Manish Jethani wrote: > `record.filter` can be null. We should do the same check as above. > i.e. `if (!record.filter || !includes(record.filter))` > > Or we could define `includes` as > `filter => filter && subscription.searchfilter(filter) != -1` Done.
https://codereview.adblockplus.org/29907589/diff/29975558/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29907589/diff/29975558/lib/devtools.js#new... lib/devtools.js:184: if (!filter || !includes(filter)) The first condition here is now unnecessary.
https://codereview.adblockplus.org/29907589/diff/29975558/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29907589/diff/29975558/lib/devtools.js#new... lib/devtools.js:184: if (!filter || !includes(filter)) On 2019/01/08 00:58:31, Manish Jethani wrote: > The first condition here is now unnecessary. Sorry, I forgot to remove it! Done.
Thanks, Jon. The changes are looking good to me now. Dave, what do you think?
We're definitely getting there. I've checked through all the changes again and I can't see any obvious mistakes. I've worked through the integration notes and I can't see any omissions. The revisions look good in the dependencies file and the title. The tests all passed for me, the code linted, and I've smoke tested all the things listed in the "Hints for testers" section on Chrome successfully. My only remaining concern is that the included changes to `CombinedMatcher.prototype.matchesAny` are non trivial, have you ensured that they haven't resulted in regressions / changed behaviour? For example, I notice that in our developer tools pane we are no longer listing whitelisted requests as being whitelisted, unless they have a matching blocking filter as well. That isn't great, and I worry that there might be other subtle differences which turn out to be more serious. > I'm in the middle of treatment for a chronic health condition and on > Friday the side effects we're had enough to debilitate me. Sorry to hear that Jon, hope you're feeling better now.
On 2019/01/08 11:49:48, kzar wrote: > My only remaining concern is that the included changes to > `CombinedMatcher.prototype.matchesAny` are non trivial, have you > ensured that they haven't resulted in regressions / changed behaviour? > For example, I notice that in our developer tools pane we are no > longer listing whitelisted requests as being whitelisted, unless they > have a matching blocking filter as well. That isn't great, and I worry > that there might be other subtle differences which turn out to be more > serious. The change about looking up whitelist filters only if there is also a blocking filter is documented in https://issues.adblockplus.org/ticket/7003. This was an intentional change in behavior. Even though the unit tests should cover most of the changes, it's possible that a regression has sneaked in (as we saw with https://issues.adblockplus.org/ticket/7181), but I'm not sure what we can do at this point other than to let QA have a go and test everything ourselves thoroughly. I can't think of any subtle changes in behavior, for what it's worth.
On 2019/01/08 13:18:57, Manish Jethani wrote: > The change about looking up whitelist filters only if there is also a blocking > filter is documented in https://issues.adblockplus.org/ticket/7003. This was an > intentional change in behavior. I see, you mean this new devtools pane behaviour is expected. While I read through the issue before, I hadn't spotted that note at the end of the testing hints about the devtools pane to be fair. Sorry, I must have missed it. I'm still not massively happy about this though honestly, it seems pretty confusing to explain to people that whitelisting filters no longer show up in our developer tools pane, unless there's a corresponding blocking filter, or in other certain situations like that the filter whitelists the entire document. I'm not sure how a filter list author, or general user would be able to reliably use our developer tools pane to know if a whitelisting filter they just wrote is correct. > ...I'm not sure what we can do at this point other than to let QA have a go > and test everything ourselves thoroughly. I can't think of any subtle changes > in behavior, for what it's worth. Well, surely we (as a part of this review) go through the 6 places that function is called, look at the logic and try to figure out if something's going to break with the function's new behaviour? I can't see any other ways those places might have problems, I had a look through them this morning. Have you taken a look as well? Otherwise this LGTM. Please could you take a look Sebastian, and also give your opinion on this intentional regression[1] in the devtools pane behaviour? [1] - https://issues.adblockplus.org/ticket/7003#Hintsfortesters
On 2019/01/08 14:24:30, kzar wrote: > On 2019/01/08 13:18:57, Manish Jethani wrote: > > The change about looking up whitelist filters only if there is also a blocking > > filter is documented in https://issues.adblockplus.org/ticket/7003. This was > an > > intentional change in behavior. > > [...] > > I'm still not massively happy about this though honestly, it seems pretty > confusing > to explain to people that whitelisting filters no longer show up in our > developer > tools pane, unless there's a corresponding blocking filter, or in other certain > situations like that the filter whitelists the entire document. I'm not sure how > a > filter list author, or general user would be able to reliably use our developer > tools > pane to know if a whitelisting filter they just wrote is correct. It seems pretty straightforward to me: the developer tools will show a whitelist filter only if a request was whitelisted. To test your whitelist filter, you would have to write a blocking filter for the given URL first. Anyway, let's take this discussion to Trac. > > ...I'm not sure what we can do at this point other than to let QA have a go > > and test everything ourselves thoroughly. I can't think of any subtle changes > > in behavior, for what it's worth. > > Well, surely we (as a part of this review) go through the 6 places that function > is > called, look at the logic and try to figure out if something's going to break > with > the function's new behaviour? > > I can't see any other ways those places might have problems, I had a look > through them > this morning. Have you taken a look as well? Yes, when I made the changes in core (and there are multiple), I had reviewed how it would affect the extension. I had missed lib/filterComposer.js but we've covered that now with https://issues.adblockplus.org/ticket/7162 So it all looks fine to me now as it is. PS: We could possibly add another option to `CombinedMatcher.prototype.matchesAny` to always look up whitelist filters when the developer tools is open. But I don't see the point: you could never reliably test if your whitelist filters were correct because it would match only one of possibly multiple filters. The validation of the filters should be done in the options page.
On 2019/01/08 14:24:30, kzar wrote: > Please could you take a look Sebastian, and also give your > opinion > on this intentional regression[1] in the devtools pane behaviour? > > [1] - https://issues.adblockplus.org/ticket/7003#Hintsfortesters I didn't caught up on the whole discussion yet. But a regression we are aware of is always considered release blocker. So it seems backwards to me to perform a dependency update that deliberately introduces a new regression. If the fix isn't trivial, can we perhaps backout the offending change in adblockpluscore? Otherwise, NOT LGTM from my side.
On 2019/01/09 11:07:36, Sebastian Noack wrote: > I didn't caught up on the whole discussion yet. But a regression we are aware of > is always considered release blocker. So it seems backwards to me to perform a > dependency update that deliberately introduces a new regression. If the fix > isn't trivial, can we perhaps backout the offending change in adblockpluscore? > Otherwise, NOT LGTM from my side. In that case, perhaps we can go with your suggestion Mansih of adding a new parameter to `CombinedMatcher.prototype.matchesAny`, so that whitelisting filters are always found when the developer tools are open? On 2019/01/08 14:55:18, Manish Jethani wrote: > PS: We could possibly add another option to > `CombinedMatcher.prototype.matchesAny` to always look up whitelist filters when > the developer tools is open...
On 2019/01/09 11:07:36, Sebastian Noack wrote: > On 2019/01/08 14:24:30, kzar wrote: > > Please could you take a look Sebastian, and also give your > > opinion > > on this intentional regression[1] in the devtools pane behaviour? > > > > [1] - https://issues.adblockplus.org/ticket/7003#Hintsfortesters > > I didn't caught up on the whole discussion yet. But a regression we are aware of > is always considered release blocker. So it seems backwards to me to perform a > dependency update that deliberately introduces a new regression. If the fix > isn't trivial, can we perhaps backout the offending change in adblockpluscore? > Otherwise, NOT LGTM from my side. Why is this a regression? It's a planned change in the behavior (we discussed this in Trac #7003). I thought we agreed that we wanted it to work this way.
On 2019/01/09 13:27:28, Manish Jethani wrote: > Why is this a regression? I consider the new behaviour worse than the existing behaviour, I explained why above. But, it seems you don't. Fair enough, I guess it's subjective. > It's a planned change in the behavior (we discussed this in Trac #7003). I wasn't copied in to that issue, and wasn't a part of the discussion. The first I noticed this new behaviour was when I examined the code where `matchesAny` was called. Later on, you pointed out that you did document this new behaviour at the bottom of the testing hints section, which I had missed. > I thought we agreed that we wanted it to work this way. It seem that you're right, skimming through the discussion on the issue I see a comment from Sebastian[1] which seems to say this new behaviour is OK. So, perhaps you don't consider this a regression like I did after all Sebastian? [1] - https://issues.adblockplus.org/ticket/7003#comment:10
On 2019/01/09 11:25:49, kzar wrote:
> In that case, perhaps we can go with your suggestion Mansih of adding a new
> parameter to `CombinedMatcher.prototype.matchesAny`, so that whitelisting
> filters are always found when the developer tools are open?
This is not so simple, by the way. A request can be associated with multiple
tabs (if it's from a service worker). What we could do in lib/requestBlocker.js
is as follows:
...
getRelatedTabIds(details).then(tabIds =>
{
for (let tabId of tabIds)
{
if (!filter && HitLogger.hasListener(tabId))
filter = defaultMatcher.isWhitelisted(...);
logRequest(tabId, ...);
}
});
And `defaultMatcher.isWhitelisted` would return a filter instead of boolean.
But this would require an additional lookup (`HitLogger.hasListener`).
On 2019/01/09 14:29:50, Manish Jethani wrote:
> On 2019/01/09 11:25:49, kzar wrote:
>
> > In that case, perhaps we can go with your suggestion Mansih of adding a new
> > parameter to `CombinedMatcher.prototype.matchesAny`, so that whitelisting
> > filters are always found when the developer tools are open?
>
> This is not so simple, by the way. A request can be associated with multiple
> tabs (if it's from a service worker). What we could do in
lib/requestBlocker.js
> is as follows:
>
> ...
>
> getRelatedTabIds(details).then(tabIds =>
> {
> for (let tabId of tabIds)
> {
> if (!filter && HitLogger.hasListener(tabId))
> filter = defaultMatcher.isWhitelisted(...);
>
> logRequest(tabId, ...);
> }
> });
>
> And `defaultMatcher.isWhitelisted` would return a filter instead of boolean.
>
> But this would require an additional lookup (`HitLogger.hasListener`).
Better idea, add a new method `hasListeners` to `HitLogger`:
hasListeners()
{
return eventEmitter._listeners.size > 0;
}
If this returns true, we look up the whitelist filter explicitly. In core we'll
make sure to remove any entries with 0 listeners.
On 2019/01/09 14:38:44, Manish Jethani wrote: > In core we'll make sure to remove any entries with 0 listeners. Here we go: https://codereview.adblockplus.org/29977555/
On 2019/01/09 14:54:27, Manish Jethani wrote: > On 2019/01/09 14:38:44, Manish Jethani wrote: > > > In core we'll make sure to remove any entries with 0 listeners. > > Here we go: > > https://codereview.adblockplus.org/29977555/ Now there's a Trac ticket where we can discuss this. We may want to log the whitelist filter, just for convenience for filter list authors, but it should not be considered a "filter hit" (no `filter.hitCount` event should be emitted) based only on the fact that DevTools is open.
On 2019/01/09 17:34:28, Manish Jethani wrote: > On 2019/01/09 14:54:27, Manish Jethani wrote: > > On 2019/01/09 14:38:44, Manish Jethani wrote: > > > > > In core we'll make sure to remove any entries with 0 listeners. > > > > Here we go: > > > > https://codereview.adblockplus.org/29977555/ > > Now there's a Trac ticket where we can discuss this. > > We may want to log the whitelist filter, just for convenience for filter list > authors, but it should not be considered a "filter hit" (no `filter.hitCount` > event should be emitted) based only on the fact that DevTools is open. Forgot to link to the ticket: https://issues.adblockplus.org/ticket/7202
Now, I got what we are talking about. If a request doesn't match any blocking filter, whitelist filters aren't considered anymore, and therefore it can happen that whitelist filters that were logged in the devtools panel before aren't logged anymore if there isn't a blocking filter that matches the same request. Yeah, we discussed this before, and I think that is OK. Sorry for the the confusion. If there is no matching blocking filter, than any whitelist filter is irrelevant for that request (i.e. the request wouldn't be blocked anyway regardless of the presence of any whitelsit filter). One could even argue that it was a bug that whitelist filters were logged to the devtools panel in this scenario before. LGTM.
Message was sent while issue was closed.
Thanks a ton for all your help here guys you're the best, I am pretty pumped to have this done!
Message was sent while issue was closed.
On 2019/01/09 21:49:32, Jon Sonesen wrote: > Thanks a ton for all your help here guys you're the best, I am pretty pumped to > have this done! Woohoo, congrats :) 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
