Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

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.
Reviewers:
sergei, kzar
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
Oct. 23, 2014, 2:11 p.m. (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 ...
Jan. 3, 2018, 11:12 a.m. (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 ...
Jan. 4, 2018, 5:04 p.m. (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 ...
Jan. 11, 2018, 6:18 p.m. (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: > ...
Jan. 12, 2018, 12:27 p.m. (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 ...
Jan. 29, 2018, 3:39 p.m. (2018-01-29 15:39:21 UTC) #6
kzar
This LGTM now, what do you think Sergei?
Jan. 30, 2018, 4:57 p.m. (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 ...
Jan. 30, 2018, 5:25 p.m. (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: ...
Jan. 30, 2018, 5:28 p.m. (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 ...
Jan. 30, 2018, 7:17 p.m. (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: > ...
Jan. 30, 2018, 9:06 p.m. (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 ...
Jan. 31, 2018, 11:06 a.m. (2018-01-31 11:06:27 UTC) #12
sergei
Feb. 5, 2018, 12:49 p.m. (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.

Powered by Google App Engine
This is Rietveld