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

Issue 5330039625220096: Issue 1162 - Cache notification URL matcher

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

Description

Issue 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
Unified diffs Side-by-side diffs Delta from patch set Stats (+82 lines, -34 lines) Patch
M lib/notification.js View 1 2 3 7 chunks +82 lines, -34 lines 9 comments Download

Messages

Total messages: 13
Thomas Greiner
3 years, 12 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 ...
9 months, 2 weeks 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 ...
9 months, 2 weeks 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 ...
9 months, 1 week ago (2018-01-11 18:18:20 UTC) #4
kzar
https://codereview.adblockplus.org/5330039625220096/diff/5629499534213120/lib/notification.js File lib/notification.js (left): https://codereview.adblockplus.org/5330039625220096/diff/5629499534213120/lib/notification.js#oldcode162 lib/notification.js:162: getNextToShow: function(url) On 2018/01/11 18:18:19, Thomas Greiner wrote: > ...
9 months, 1 week ago (2018-01-12 12:27:18 UTC) #5
Thomas Greiner
I've rebased the review, applied the requested changes and did some quick testing to verify ...
8 months, 3 weeks ago (2018-01-29 15:39:21 UTC) #6
kzar
This LGTM now, what do you think Sergei?
8 months, 3 weeks ago (2018-01-30 16:57:46 UTC) #7
sergei
https://codereview.adblockplus.org/5330039625220096/diff/29683629/lib/notification.js File lib/notification.js (right): https://codereview.adblockplus.org/5330039625220096/diff/29683629/lib/notification.js#newcode141 lib/notification.js:141: return false; It somehow conflicts with above if (typeof ...
8 months, 3 weeks ago (2018-01-30 17:25:05 UTC) #8
sergei
https://codereview.adblockplus.org/5330039625220096/diff/29683629/lib/notification.js File lib/notification.js (right): https://codereview.adblockplus.org/5330039625220096/diff/29683629/lib/notification.js#newcode190 lib/notification.js:190: let {data} = Prefs.notificationdata; On 2018/01/30 17:25:05, sergei wrote: ...
8 months, 3 weeks ago (2018-01-30 17:28:24 UTC) #9
Thomas Greiner
https://codereview.adblockplus.org/5330039625220096/diff/29683629/lib/notification.js File lib/notification.js (right): https://codereview.adblockplus.org/5330039625220096/diff/29683629/lib/notification.js#newcode141 lib/notification.js:141: return false; On 2018/01/30 17:25:05, sergei wrote: > It ...
8 months, 3 weeks ago (2018-01-30 19:17:53 UTC) #10
sergei
https://codereview.adblockplus.org/5330039625220096/diff/29683629/lib/notification.js File lib/notification.js (right): https://codereview.adblockplus.org/5330039625220096/diff/29683629/lib/notification.js#newcode141 lib/notification.js:141: return false; On 2018/01/30 19:17:53, Thomas Greiner wrote: > ...
8 months, 3 weeks ago (2018-01-30 21:06:48 UTC) #11
kzar
https://codereview.adblockplus.org/5330039625220096/diff/29683629/lib/notification.js File lib/notification.js (right): https://codereview.adblockplus.org/5330039625220096/diff/29683629/lib/notification.js#newcode190 lib/notification.js:190: let {data} = Prefs.notificationdata; On 2018/01/30 19:17:53, Thomas Greiner ...
8 months, 3 weeks ago (2018-01-31 11:06:27 UTC) #12
sergei
8 months, 2 weeks ago (2018-02-05 12:49:34 UTC) #13
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.
Sign in to reply to this message.

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