Issue 5330039625220096: Issue 1162 - Cache notification URL matcher
|
Created:
Oct. 23, 2014, 2:09 p.m. by Thomas Greiner Modified:
Feb. 5, 2018, 12:49 p.m. Visibility:
Public. |
DescriptionIssue 1162 - Cache notification URL matcher
Patch Set 1 #
Total comments: 20
Patch Set 2 : Rebased to e2203d4fd258 #
Total comments: 1
Patch Set 3 : Addressed comments #Patch Set 4 : Fixed regression: Error if notification data not yet initialized #
Total comments: 9
MessagesTotal messages: 13
Good, just a couple of thoughts. I have added Dave as the reviewer. https://codereview.adblockplus.org/5330039625220096/diff/5629499534213120/lib... File lib/notification.js (right): https://codereview.adblockplus.org/5330039625220096/diff/5629499534213120/lib... lib/notification.js:73: matcher.toJSON = () => {}; This is not exactly how it's said in the issue "Notifications should remain serializable (replace JSON.serialize() to drop all properties starting with "_" if necessary)" but for me the current variant is fine. https://codereview.adblockplus.org/5330039625220096/diff/5629499534213120/lib... lib/notification.js:83: let remoteData = []; Could you please add a comment that localData and remoteData are arrays of notification objects and their role is to cache temporary and persistent notifications respectively. https://codereview.adblockplus.org/5330039625220096/diff/5629499534213120/lib... lib/notification.js:157: remoteData = data.notifications; It's a bit theoretical but IMO it can be a technical debt. The difference from the previous approach is that earlier in getNextToShow we read Prefs.notificationdata.data, which can be theoretically set by several ways, but now in getNextToShow we are reading remoteData which is set only from _onDownloadSuccess. IMO, although currently it does not change anything, we should rather listen to the changes of Prefs and when Prefs.notificationdata.data is changed remoteData should be updated. https://codereview.adblockplus.org/5330039625220096/diff/5629499534213120/lib... lib/notification.js:158: Prefs.notificationdata.data = data; Despite it's not directly a part of this issue, should new notifications actually be added to already existing ones instead of overriding? If there are no new notifications then it will remove all previously stored ones.
On 2018/01/03 11:12:23, sergei wrote: > I have added Dave as the reviewer. Thanks? Since this review is 3 years old it will need to be rebased, but I've added some comments. https://codereview.adblockplus.org/5330039625220096/diff/5629499534213120/lib... File lib/notification.js (left): https://codereview.adblockplus.org/5330039625220096/diff/5629499534213120/lib... lib/notification.js:162: getNextToShow: function(url) This function is confusing, each section needs a comment explaining the logic. E.g. "If the notification is restricted to certain URLs we must check that the current URL (if any) matches." I also think it's too long and could do with being split apart. https://codereview.adblockplus.org/5330039625220096/diff/5629499534213120/lib... lib/notification.js:192: && Prefs.notificationdata.shown.indexOf(notification.id) !== -1) We can use .includes(notification.id) these days (same elsewhere). https://codereview.adblockplus.org/5330039625220096/diff/5629499534213120/lib... lib/notification.js:195: if (typeof url === "string" || notification.urlFilters instanceof Array) IMO this code should instead be replaced with a call to a function. Something like urlFiltersMatchUrl(url, urlFilters). That function should worry about caching the matcher and it can do so using a WeakMap instead of mutating the notification itself. The function should return true if the url matches, false if it doesn't and undefined if there's no URL / urlFilters to match. https://codereview.adblockplus.org/5330039625220096/diff/5629499534213120/lib... File lib/notification.js (right): https://codereview.adblockplus.org/5330039625220096/diff/5629499534213120/lib... lib/notification.js:118: _getDownloadables: function() Why are we exporting functions like this and prefixing them with an underscore, instead of simply not exporting them? https://codereview.adblockplus.org/5330039625220096/diff/5629499534213120/lib... lib/notification.js:157: remoteData = data.notifications; On 2018/01/03 11:12:22, sergei wrote: > It's a bit theoretical but IMO it can be a technical debt. > The difference from the previous approach is that earlier in getNextToShow we > read Prefs.notificationdata.data, which can be theoretically set by several > ways, but now in getNextToShow we are reading remoteData which is set only from > _onDownloadSuccess. IMO, although currently it does not change anything, we > should rather listen to the changes of Prefs and when > Prefs.notificationdata.data is changed remoteData should be updated. Yea, I don't think these changes are necessary now with my other suggestions. https://codereview.adblockplus.org/5330039625220096/diff/5629499534213120/lib... lib/notification.js:158: Prefs.notificationdata.data = data; On 2018/01/03 11:12:22, sergei wrote: > Despite it's not directly a part of this issue, should new notifications > actually be added to already existing ones instead of overriding? If there are > no new notifications then it will remove all previously stored ones. Hmm, I'm not too familiar with this code but I think you might be right.
Thanks guys for bringing this up again. Since this file has been moved from adblockplus to adblockpluscore in the meantime, it might be a bit trickier to rebase so I thought I'd reply to your comments before working on the new patch. https://codereview.adblockplus.org/5330039625220096/diff/5629499534213120/lib... File lib/notification.js (left): https://codereview.adblockplus.org/5330039625220096/diff/5629499534213120/lib... lib/notification.js:162: getNextToShow: function(url) On 2018/01/04 17:04:09, kzar wrote: > This function is confusing, each section needs a comment explaining the logic. > E.g. "If the notification is restricted to certain URLs we must check that the > current URL (if any) matches." > > I also think it's too long and could do with being split apart. Agreed but I'd suggest not touching any code that's unrelated to this issue and instead tackling it separately. The reason for it being that we may break some stuff which would then require backing out the entire change. https://codereview.adblockplus.org/5330039625220096/diff/5629499534213120/lib... lib/notification.js:195: if (typeof url === "string" || notification.urlFilters instanceof Array) On 2018/01/04 17:04:09, kzar wrote: > IMO this code should instead be replaced with a call to a function. Something > like urlFiltersMatchUrl(url, urlFilters). That function should worry about > caching the matcher and it can do so using a WeakMap instead of mutating the > notification itself. The function should return true if the url matches, false > if it doesn't and undefined if there's no URL / urlFilters to match. I agree that splitting that off into its own function is a good idea to make it more readable and easier to maintain. However, using `WeakMap` seems pretty equal to using `Symbol` (see https://codereview.adblockplus.org/5330039625220096/diff/5629499534213120/lib...). Therefore I'd be fine with either so just wanted to point that out. https://codereview.adblockplus.org/5330039625220096/diff/5629499534213120/lib... File lib/notification.js (right): https://codereview.adblockplus.org/5330039625220096/diff/5629499534213120/lib... lib/notification.js:73: matcher.toJSON = () => {}; On 2018/01/03 11:12:22, sergei wrote: > This is not exactly how it's said in the issue "Notifications should remain > serializable (replace JSON.serialize() to drop all properties starting with "_" > if necessary)" but for me the current variant is fine. Fortunately, we can now use `Symbol` instead so that the matcher property won't be in the enumeration when running `JSON.stringify()` so I'd change it to use that instead so that we can avoid this workaround altogether. https://codereview.adblockplus.org/5330039625220096/diff/5629499534213120/lib... lib/notification.js:83: let remoteData = []; On 2018/01/03 11:12:22, sergei wrote: > Could you please add a comment that localData and remoteData are arrays of > notification objects and their role is to cache temporary and persistent > notifications respectively. Yep, will do. https://codereview.adblockplus.org/5330039625220096/diff/5629499534213120/lib... lib/notification.js:118: _getDownloadables: function() On 2018/01/04 17:04:09, kzar wrote: > Why are we exporting functions like this and prefixing them with an underscore, > instead of simply not exporting them? I don't think it's meant to be exported because it's also not used anywhere. It was probably just a way to group related functions. But since I haven't created this I can't say for sure but would think that we could move some functions which don't refer to `this`, like `_getDownloadables()`, outside of the `Notification` prototype. https://codereview.adblockplus.org/5330039625220096/diff/5629499534213120/lib... lib/notification.js:157: remoteData = data.notifications; On 2018/01/03 11:12:22, sergei wrote: > It's a bit theoretical but IMO it can be a technical debt. > The difference from the previous approach is that earlier in getNextToShow we > read Prefs.notificationdata.data, which can be theoretically set by several > ways, but now in getNextToShow we are reading remoteData which is set only from > _onDownloadSuccess. IMO, although currently it does not change anything, we > should rather listen to the changes of Prefs and when > Prefs.notificationdata.data is changed remoteData should be updated. Good point. I don't remember why I made this change back when I created this review. It could've been just a leftover from development. Anyway, I agree that this variable is redundant. https://codereview.adblockplus.org/5330039625220096/diff/5629499534213120/lib... lib/notification.js:158: Prefs.notificationdata.data = data; On 2018/01/03 11:12:22, sergei wrote: > Despite it's not directly a part of this issue, should new notifications > actually be added to already existing ones instead of overriding? If there are > no new notifications then it will remove all previously stored ones. By overriding existing notifications we can control which notifications should be active on which day. That's probably important for calculating conversion rates. But again, I haven't implemented the initial logic so I can't say for sure.
https://codereview.adblockplus.org/5330039625220096/diff/5629499534213120/lib... File lib/notification.js (left): https://codereview.adblockplus.org/5330039625220096/diff/5629499534213120/lib... lib/notification.js:162: getNextToShow: function(url) On 2018/01/11 18:18:19, Thomas Greiner wrote: > On 2018/01/04 17:04:09, kzar wrote: > > This function is confusing, each section needs a comment explaining the logic. > > E.g. "If the notification is restricted to certain URLs we must check that the > > current URL (if any) matches." > > > > I also think it's too long and could do with being split apart. > > Agreed but I'd suggest not touching any code that's unrelated to this issue and > instead tackling it separately. The reason for it being that we may break some > stuff which would then require backing out the entire change. IMO I think we should tidy this code up while addressing the issue, but sure split the changes up into separate commits (and codereviews). https://codereview.adblockplus.org/5330039625220096/diff/5629499534213120/lib... lib/notification.js:195: if (typeof url === "string" || notification.urlFilters instanceof Array) On 2018/01/11 18:18:19, Thomas Greiner wrote: > On 2018/01/04 17:04:09, kzar wrote: > > IMO this code should instead be replaced with a call to a function. Something > > like urlFiltersMatchUrl(url, urlFilters). That function should worry about > > caching the matcher and it can do so using a WeakMap instead of mutating the > > notification itself. The function should return true if the url matches, false > > if it doesn't and undefined if there's no URL / urlFilters to match. > > I agree that splitting that off into its own function is a good idea to make it > more readable and easier to maintain. > > However, using `WeakMap` seems pretty equal to using `Symbol` (see > https://codereview.adblockplus.org/5330039625220096/diff/5629499534213120/lib...). > Therefore I'd be fine with either so just wanted to point that out. Acknowledged. https://codereview.adblockplus.org/5330039625220096/diff/5629499534213120/lib... File lib/notification.js (right): https://codereview.adblockplus.org/5330039625220096/diff/5629499534213120/lib... lib/notification.js:73: matcher.toJSON = () => {}; On 2018/01/11 18:18:19, Thomas Greiner wrote: > On 2018/01/03 11:12:22, sergei wrote: > > This is not exactly how it's said in the issue "Notifications should remain > > serializable (replace JSON.serialize() to drop all properties starting with > "_" > > if necessary)" but for me the current variant is fine. > > Fortunately, we can now use `Symbol` instead so that the matcher property won't > be in the enumeration when running `JSON.stringify()` so I'd change it to use > that instead so that we can avoid this workaround altogether. Acknowledged.
I've rebased the review, applied the requested changes and did some quick testing to verify that the anti-adblock notification works as expected. Please let me know if I missed something. https://codereview.adblockplus.org/5330039625220096/diff/29683611/lib/notific... File lib/notification.js (right): https://codereview.adblockplus.org/5330039625220096/diff/29683611/lib/notific... lib/notification.js:317: if (typeof url === "string" || "_matcher" in notification) This block contains relevant changes that were made during the rebase.
This LGTM now, what do you think Sergei?
https://codereview.adblockplus.org/5330039625220096/diff/29683629/lib/notific... File lib/notification.js (right): https://codereview.adblockplus.org/5330039625220096/diff/29683629/lib/notific... lib/notification.js:141: return false; It somehow conflicts with above if (typeof url !== "string" && !(MATCHER in notification)) return true; Should we actually return `true` when the type of `url` parameter is actually unexpected? https://codereview.adblockplus.org/5330039625220096/diff/29683629/lib/notific... lib/notification.js:190: let {data} = Prefs.notificationdata; Prefs' properties can be not ready yet, I would recommend to move it into a function and check whether Prefs are already loaded then call it, otherwise add the function as a listener for load event, Prefs in parent projects should be adjusted too.
https://codereview.adblockplus.org/5330039625220096/diff/29683629/lib/notific... File lib/notification.js (right): https://codereview.adblockplus.org/5330039625220096/diff/29683629/lib/notific... lib/notification.js:190: let {data} = Prefs.notificationdata; On 2018/01/30 17:25:05, sergei wrote: > Prefs' properties can be not ready yet, I would recommend to move it into a > function and check whether Prefs are already loaded then call it, otherwise add > the function as a listener for load event, Prefs in parent projects should be > adjusted too. Perhaps alternatively we could do it in `matchesUrl`.
https://codereview.adblockplus.org/5330039625220096/diff/29683629/lib/notific... File lib/notification.js (right): https://codereview.adblockplus.org/5330039625220096/diff/29683629/lib/notific... lib/notification.js:141: return false; On 2018/01/30 17:25:05, sergei wrote: > It somehow conflicts with above > if (typeof url !== "string" && !(MATCHER in notification)) > return true; > > Should we actually return `true` when the type of `url` parameter is actually > unexpected? The logic behind it is that if the notification doesn't target URLs then we should just continue with checking whether it should be shown or not. However, if one of them is missing it means that - either the notification doesn't target URLs but we're only checking for notifications which target URLs - or that the notification targets URLs but that there's no URL to match against In both situations there's a mismatch which means that the notification shouldn't be shown. --- If you think it'd be helpful, I can split this condition up into two separate ones to add an individual comment to each of them. Alternatively, I could introduce variables to clarify what each part stands for and thereby also reduce duplication. e.g. let shouldCheck = (typeof url !== "string"); let targetsUrls = (MATCHER in notification); if (!shouldCheck && !targetsUrls) return true; if (!shouldCheck || !targetsUrls) return false; https://codereview.adblockplus.org/5330039625220096/diff/29683629/lib/notific... lib/notification.js:190: let {data} = Prefs.notificationdata; On 2018/01/30 17:28:24, sergei wrote: > On 2018/01/30 17:25:05, sergei wrote: > > Prefs' properties can be not ready yet, I would recommend to move it into a > > function and check whether Prefs are already loaded then call it, otherwise > add > > the function as a listener for load event, Prefs in parent projects should be > > adjusted too. > > Perhaps alternatively we could do it in `matchesUrl`. In theory that's correct but in practice this is not an issue since the preference is initialized as `{}` (at least in adblockpluschrome). Anyway, I do agree that it wouldn't be a bad idea to catch it just in case, because it's only an implicit dependency that we have here which makes it quite unreliable. Besides that, the same issue also applies to `Prefs.notifications_ignoredcategories` as well as pretty much every other use of `Prefs.notificationdata` so we may need to rethink our approach on a more fundamental level. One option I could think of would be to introduce a lib/prefs.js module in adblockpluscore so that it can define and initialize its own preferences. Would you prefer if I add a workaround for this particular case or should I create a ticket so that we can tackle the underlying issue separately?
https://codereview.adblockplus.org/5330039625220096/diff/29683629/lib/notific... File lib/notification.js (right): https://codereview.adblockplus.org/5330039625220096/diff/29683629/lib/notific... lib/notification.js:141: return false; On 2018/01/30 19:17:53, Thomas Greiner wrote: > On 2018/01/30 17:25:05, sergei wrote: > > It somehow conflicts with above > > if (typeof url !== "string" && !(MATCHER in notification)) > > return true; > > > > Should we actually return `true` when the type of `url` parameter is actually > > unexpected? > > The logic behind it is that if the notification doesn't target URLs then we > should just continue with checking whether it should be shown or not. > However, if one of them is missing it means that > - either the notification doesn't target URLs but we're only checking for > notifications which target URLs > - or that the notification targets URLs but that there's no URL to match against > > In both situations there's a mismatch which means that the notification > shouldn't be shown. > > --- > > If you think it'd be helpful, I can split this condition up into two separate > ones to add an individual comment to each of them. > > Alternatively, I could introduce variables to clarify what each part stands for > and thereby also reduce duplication. > > e.g. > let shouldCheck = (typeof url !== "string"); > let targetsUrls = (MATCHER in notification); > > if (!shouldCheck && !targetsUrls) > return true; > > if (!shouldCheck || !targetsUrls) > return false; Acknowledged. https://codereview.adblockplus.org/5330039625220096/diff/29683629/lib/notific... lib/notification.js:190: let {data} = Prefs.notificationdata; On 2018/01/30 19:17:53, Thomas Greiner wrote: > On 2018/01/30 17:28:24, sergei wrote: > > On 2018/01/30 17:25:05, sergei wrote: > > > Prefs' properties can be not ready yet, I would recommend to move it into a > > > function and check whether Prefs are already loaded then call it, otherwise > > add > > > the function as a listener for load event, Prefs in parent projects should > be > > > adjusted too. > > > > Perhaps alternatively we could do it in `matchesUrl`. > > In theory that's correct but in practice this is not an issue since the > preference is initialized as `{}` (at least in adblockpluschrome). Anyway, I do > agree that it wouldn't be a bad idea to catch it just in case, because it's only > an implicit dependency that we have here which makes it quite unreliable. It does happen on practice (at least I observed the same issue in libadblockplus). Let's consider how it works: there is some moral equivalent of "main.js" which just loads all our js files, including notification.js and let's say prefs.js is loaded by it too (it's equivalent when prefs.js is first time loaded by notification.js and notification.js is loaded as a result of series of calls of `require` BTW), so CURRENTLY RUNNING JS -- BEGIN -- require("...") require("prefs") init settings to default values and load previously stored settings asynchronously at some point later schedule the call of "done" callback after finishing currently running JS code require("...") require("notification") Notification.init() here we have only default values for Prefs require("...") CURRENTLY RUNNING JS -- END -- .... CURRENTLY RUNNING JS -- BEGIN -- "done" callback set Prefs now one may use Prefs. CURRENTLY RUNNING JS -- END -- BTW, in some test of libadblockplus there were a couple of such cases and for the sake of simplicity and in order to not bother adblockpluscore we just called "done" callback for preferences immediately (synchronously), so Prefs are ready right after `require("prefs")` :) > > Besides that, the same issue also applies to > `Prefs.notifications_ignoredcategories` as well as pretty much every other use > of `Prefs.notificationdata` so we may need to rethink our approach on a more > fundamental level. You are right, currently no one observed the problem here because the probability of the race condition with Prefs.notifications_ignoredcategories in particular is too low because it accessed first time after a huge delay, but for that new code, Prefs will not be populated by asynchronously loaded values yet. In general of course it would be good to address it at another level, e.g. either even load the rest of the modules or at least call their `init` functions after Prefs are loaded. However, it can cause undesired delays, so I think such complexity (fiddling with the "load" event) looks good for me. Actually, for the sake of simplicity we can just add a method to Prefs which contains the logic and either calls a callback immediately or adds it as a listener for some "load" event. > One option I could think of would be to introduce a lib/prefs.js module in > adblockpluscore so that it can define and initialize its own preferences. > > Would you prefer if I add a workaround for this particular case or should I > create a ticket so that we can tackle the underlying issue separately? Maybe it's a good idea to make the decision even about the approach to deal with such case in an issue, feel free to file as many as you think there should be :)
https://codereview.adblockplus.org/5330039625220096/diff/29683629/lib/notific... File lib/notification.js (right): https://codereview.adblockplus.org/5330039625220096/diff/29683629/lib/notific... lib/notification.js:190: let {data} = Prefs.notificationdata; On 2018/01/30 19:17:53, Thomas Greiner wrote: > Besides that, the same issue also applies to > `Prefs.notifications_ignoredcategories` as well as pretty much every other use > of `Prefs.notificationdata` so we may need to rethink our approach on a more > fundamental level. > One option I could think of would be to introduce a lib/prefs.js module in > adblockpluscore so that it can define and initialize its own preferences. > > Would you prefer if I add a workaround for this particular case or should I > create a ticket so that we can tackle the underlying issue separately? How about using the Prefs.untilLoaded Promise?
https://codereview.adblockplus.org/5330039625220096/diff/29683629/lib/notific... File lib/notification.js (right): https://codereview.adblockplus.org/5330039625220096/diff/29683629/lib/notific... lib/notification.js:190: let {data} = Prefs.notificationdata; On 2018/01/31 11:06:27, kzar wrote: > On 2018/01/30 19:17:53, Thomas Greiner wrote: > > > Besides that, the same issue also applies to > > `Prefs.notifications_ignoredcategories` as well as pretty much every other use > > of `Prefs.notificationdata` so we may need to rethink our approach on a more > > fundamental level. > > One option I could think of would be to introduce a lib/prefs.js module in > > adblockpluscore so that it can define and initialize its own preferences. > > > > Would you prefer if I add a workaround for this particular case or should I > > create a ticket so that we can tackle the underlying issue separately? > > How about using the Prefs.untilLoaded Promise? It seems the best option. 
|
