Issue 29712599: Issue 6420 - Fixed poping up whitelisted website notification for several custom filter lists (Closed)
|
Created:
March 1, 2018, 12:13 p.m. by saroyanm Modified:
March 6, 2018, 6:07 p.m. Reviewers:
Thomas Greiner Visibility:
Public. |
DescriptionIssue 6420 - Fixed popping up whitelisted website notification for several custom filter lists
Patch Set 1 #
Total comments: 9
Patch Set 2 : Addressed Thomas's comment #Patch Set 3 : Updated as discussed with Thomas #
Total comments: 9
Patch Set 4 : #
Total comments: 2
MessagesTotal messages: 15
Thomas can you please have a look
https://codereview.adblockplus.org/29712599/diff/29712600/js/desktop-options.js File js/desktop-options.js (right): https://codereview.adblockplus.org/29712599/diff/29712600/js/desktop-options.... js/desktop-options.js:1161: isCustomFiltersLoaded = false; Why not instead just move `isCustomFiltersLoaded = true` to after `loadCustomFilters()` here? Because this is the only location where we load custom filters to initialize them so I don't see a reason why to set it to `true` any other time we call `loadCustomFilters()`.
https://codereview.adblockplus.org/29712599/diff/29712600/js/desktop-options.js File js/desktop-options.js (right): https://codereview.adblockplus.org/29712599/diff/29712600/js/desktop-options.... js/desktop-options.js:1161: isCustomFiltersLoaded = false; On 2018/03/02 11:43:57, Thomas Greiner wrote: > Why not instead just move `isCustomFiltersLoaded = true` to after > `loadCustomFilters()` here? > > Because this is the only location where we load custom filters to initialize > them so I don't see a reason why to set it to `true` any other time we call > `loadCustomFilters()`. Agree, done.
Also note the typo in the commit message for which I'd suggest replacing "poping" with "popping". https://codereview.adblockplus.org/29712599/diff/29712600/js/desktop-options.js File js/desktop-options.js (right): https://codereview.adblockplus.org/29712599/diff/29712600/js/desktop-options.... js/desktop-options.js:1161: isCustomFiltersLoaded = false; On 2018/03/02 15:59:14, saroyanm wrote: > Agree, done. What about the code you initially added?
https://codereview.adblockplus.org/29712599/diff/29712600/js/desktop-options.js File js/desktop-options.js (right): https://codereview.adblockplus.org/29712599/diff/29712600/js/desktop-options.... js/desktop-options.js:1161: isCustomFiltersLoaded = false; On 2018/03/02 16:40:43, Thomas Greiner wrote: > On 2018/03/02 15:59:14, saroyanm wrote: > > Agree, done. > > What about the code you initially added? Which code ?
https://codereview.adblockplus.org/29712599/diff/29712600/js/desktop-options.js File js/desktop-options.js (right): https://codereview.adblockplus.org/29712599/diff/29712600/js/desktop-options.... js/desktop-options.js:1161: isCustomFiltersLoaded = false; On 2018/03/02 16:43:46, saroyanm wrote: > Which code ? The new code from the first patch set.
https://codereview.adblockplus.org/29712599/diff/29712600/js/desktop-options.js File js/desktop-options.js (right): https://codereview.adblockplus.org/29712599/diff/29712600/js/desktop-options.... js/desktop-options.js:1161: isCustomFiltersLoaded = false; On 2018/03/02 17:09:04, Thomas Greiner wrote: > On 2018/03/02 16:43:46, saroyanm wrote: > > Which code ? > > The new code from the first patch set. You mean the current one, right ? I don't remember adding any other patch sets. I can see current one and that small change you have mentioned here as part of patchset 2 -> https://codereview.adblockplus.org/29712599/diff/29713620/js/desktop-options.js
https://codereview.adblockplus.org/29712599/diff/29712600/js/desktop-options.js File js/desktop-options.js (right): https://codereview.adblockplus.org/29712599/diff/29712600/js/desktop-options.... js/desktop-options.js:1161: isCustomFiltersLoaded = false; On 2018/03/02 17:16:51, saroyanm wrote: > You mean the current one, right ? > I don't remember adding any other patch sets. > > I can see current one and that small change you have mentioned here as part of > patchset 2 -> > https://codereview.adblockplus.org/29712599/diff/29713620/js/desktop-options.js > I mean this one: https://codereview.adblockplus.org/29712599/diff/29712600/js/desktop-options.js
Notes, from the discussion with Thomas https://codereview.adblockplus.org/29712599/diff/29712600/js/desktop-options.js File js/desktop-options.js (right): https://codereview.adblockplus.org/29712599/diff/29712600/js/desktop-options.... js/desktop-options.js:1161: isCustomFiltersLoaded = false; On 2018/03/02 17:24:16, Thomas Greiner wrote: > On 2018/03/02 17:16:51, saroyanm wrote: > > You mean the current one, right ? > > I don't remember adding any other patch sets. > > > > I can see current one and that small change you have mentioned here as part of > > patchset 2 -> > > > https://codereview.adblockplus.org/29712599/diff/29713620/js/desktop-options.js > > > > I mean this one: > https://codereview.adblockplus.org/29712599/diff/29712600/js/desktop-options.js We are using loop on top, but now we decided to refactor the code and use promises.
Ready for the review. https://codereview.adblockplus.org/29712599/diff/29712600/js/desktop-options.js File js/desktop-options.js (right): https://codereview.adblockplus.org/29712599/diff/29712600/js/desktop-options.... js/desktop-options.js:1161: isCustomFiltersLoaded = false; On 2018/03/05 12:14:52, saroyanm wrote: > On 2018/03/02 17:24:16, Thomas Greiner wrote: > > On 2018/03/02 17:16:51, saroyanm wrote: > > > You mean the current one, right ? > > > I don't remember adding any other patch sets. > > > > > > I can see current one and that small change you have mentioned here as part > of > > > patchset 2 -> > > > > > > https://codereview.adblockplus.org/29712599/diff/29713620/js/desktop-options.js > > > > > > > I mean this one: > > > https://codereview.adblockplus.org/29712599/diff/29712600/js/desktop-options.js > > We are using loop on top, but now we decided to refactor the code and use > promises. Done.
https://codereview.adblockplus.org/29712599/diff/29714581/js/desktop-options.js File js/desktop-options.js (right): https://codereview.adblockplus.org/29712599/diff/29714581/js/desktop-options.... js/desktop-options.js:1150: getSubscriptionFilters(subscription)); Detail: This arrow function is redundant because you can write the same code as: let customFilterPromises = subscriptions.map(getSubscriptionFilters); https://codereview.adblockplus.org/29712599/diff/29714581/js/desktop-options.... js/desktop-options.js:1154: loadCustomFilters([].concat(...filters)); What is this concatenation for? https://codereview.adblockplus.org/29712599/diff/29714581/js/desktop-options.... js/desktop-options.js:1337: browser.runtime.sendMessage({ `browser.runtime.sendMessage()` already returns a promise because our abstraction layer also supports how Firefox implements it those APIs. Therefore we don't need to wrap it inside our own promise here. See also https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/runtime/sendMes...
https://codereview.adblockplus.org/29712599/diff/29714581/js/desktop-options.js File js/desktop-options.js (right): https://codereview.adblockplus.org/29712599/diff/29714581/js/desktop-options.... js/desktop-options.js:1150: getSubscriptionFilters(subscription)); On 2018/03/05 16:43:33, Thomas Greiner wrote: > Detail: This arrow function is redundant because you can write the same code as: > > let customFilterPromises = subscriptions.map(getSubscriptionFilters); I like this, will change. https://codereview.adblockplus.org/29712599/diff/29714581/js/desktop-options.... js/desktop-options.js:1154: loadCustomFilters([].concat(...filters)); On 2018/03/05 16:43:33, Thomas Greiner wrote: > What is this concatenation for? It's for flattening the array. Promise.all returns a matrix in this case, because getSubscriptionFilters promises an array. https://codereview.adblockplus.org/29712599/diff/29714581/js/desktop-options.... js/desktop-options.js:1337: browser.runtime.sendMessage({ On 2018/03/05 16:43:33, Thomas Greiner wrote: > `browser.runtime.sendMessage()` already returns a promise because our > abstraction layer also supports how Firefox implements it those APIs. Therefore > we don't need to wrap it inside our own promise here. > > See also > https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/runtime/sendMes... Didn't know that we already return a promise, will update this. THanks.
https://codereview.adblockplus.org/29712599/diff/29714581/js/desktop-options.js File js/desktop-options.js (right): https://codereview.adblockplus.org/29712599/diff/29714581/js/desktop-options.... js/desktop-options.js:1337: browser.runtime.sendMessage({ On 2018/03/05 16:51:13, saroyanm wrote: > On 2018/03/05 16:43:33, Thomas Greiner wrote: > > `browser.runtime.sendMessage()` already returns a promise because our > > abstraction layer also supports how Firefox implements it those APIs. > Therefore > > we don't need to wrap it inside our own promise here. > > > > See also > > > https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/runtime/sendMes... > > Didn't know that we already return a promise, will update this. > THanks. Hmm, Apparently the polyfill doesn't return Promise -> https://hg.adblockplus.org/adblockplusui/file/tip/ext/content.js#l67
Thanks Thomas Ready for review https://codereview.adblockplus.org/29712599/diff/29714617/ext/content.js File ext/content.js (right): https://codereview.adblockplus.org/29712599/diff/29714617/ext/content.js#newc... ext/content.js:77: let resolvePromise = null; I noticed that it took me longer than needed to implement the Error handling in current Mock, if you would like me to do that in current review please let me know, but otherwise this should work for now.
LGTM https://codereview.adblockplus.org/29712599/diff/29714581/js/desktop-options.js File js/desktop-options.js (right): https://codereview.adblockplus.org/29712599/diff/29714581/js/desktop-options.... js/desktop-options.js:1154: loadCustomFilters([].concat(...filters)); On 2018/03/05 16:51:13, saroyanm wrote: > It's for flattening the array. > > Promise.all returns a matrix in this case, because getSubscriptionFilters > promises an array. Sorry, I missed that. Maybe because I'm used to writing this as `Array.prototype.concat.apply([], filters)`. Your approach is actually cleaner so looks good. https://codereview.adblockplus.org/29712599/diff/29714581/js/desktop-options.... js/desktop-options.js:1337: browser.runtime.sendMessage({ On 2018/03/05 17:18:39, saroyanm wrote: > Hmm, Apparently the polyfill doesn't return Promise -> > https://hg.adblockplus.org/adblockplusui/file/tip/ext/content.js#l67 It's not actually a polyfill and more of a mock so I don't know why we label it as such in ext/content.js. But yes, we need to update that. https://codereview.adblockplus.org/29712599/diff/29714617/ext/content.js File ext/content.js (right): https://codereview.adblockplus.org/29712599/diff/29714617/ext/content.js#newc... ext/content.js:77: let resolvePromise = null; On 2018/03/05 20:28:33, saroyanm wrote: > I noticed that it took me longer than needed to implement the Error handling in > current Mock, if you would like me to do that in current review please let me > know, but otherwise this should work for now. We're not handling errors yet so we don't need to worry about adding that capability to the mock at this point either. 
|
