Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(30)

Issue 5330039625220096: Issue 1162 - Cache notification URL matcher

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 2 months ago by Thomas Greiner
Modified:
1 week ago
Reviewers:
kzar
CC:
sergei
Visibility:
Public.

Description

Issue 1162 - Cache notification URL matcher

Patch Set 1 #

Total comments: 20
Unified diffs Side-by-side diffs Delta from patch set Stats (+29 lines, -10 lines) Patch
M lib/notification.js View 6 chunks +29 lines, -10 lines 20 comments Download

Messages

Total messages: 5
Thomas Greiner
3 years, 2 months ago (2014-10-23 14:11:28 UTC) #1
sergei
Good, just a couple of thoughts. I have added Dave as the reviewer. https://codereview.adblockplus.org/5330039625220096/diff/5629499534213120/lib/notification.js File ...
2 weeks, 2 days ago (2018-01-03 11:12:23 UTC) #2
kzar
On 2018/01/03 11:12:23, sergei wrote: > I have added Dave as the reviewer. Thanks? Since ...
2 weeks, 1 day ago (2018-01-04 17:04:09 UTC) #3
Thomas Greiner
Thanks guys for bringing this up again. Since this file has been moved from adblockplus ...
1 week, 1 day ago (2018-01-11 18:18:20 UTC) #4
kzar
1 week ago (2018-01-12 12:27:18 UTC) #5
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5