Issue 2368 - Update adblockplus dependency to revision feb391127fe4

Issue 2368 - Update adblockplus dependency to revision feb391127fe4

[Sebastian Noack, 2015-06-12 09:29:24]

LGTM, but too late to land before the 1.9 release. Please push afterwards.

[Sebastian Noack, 2015-06-12 09:42:06]

http://codereview.adblockplus.org/5733084272001024/diff/6270652252160000/lib/...
File lib/notificationHelper.js (right):

http://codereview.adblockplus.org/5733084272001024/diff/6270652252160000/lib/...
lib/notificationHelper.js:229: NotificationStorage.showNext(url &&
stringifyURL(url));
Oh wait, I were too quick. The case where url isn't given here doesn't exist
anymore. So maybe making the argument rudimentary renaming the function to
showNextNotificationForURL?

[Sebastian Noack, 2015-06-12 09:42:42]

http://codereview.adblockplus.org/5733084272001024/diff/6270652252160000/lib/...
File lib/notificationHelper.js (right):

http://codereview.adblockplus.org/5733084272001024/diff/6270652252160000/lib/...
lib/notificationHelper.js:229: NotificationStorage.showNext(url &&
stringifyURL(url));
On 2015/06/12 09:42:06, Sebastian Noack wrote:
> Oh wait, I were too quick. The case where url isn't given here doesn't exist
> anymore. So maybe making the argument rudimentary renaming the function to
> showNextNotificationForURL?

s/rudimentary/mandatory/

[Felix Dahlke, 2015-06-12 17:01:34]

New patch set.

http://codereview.adblockplus.org/5733084272001024/diff/6270652252160000/lib/...
File lib/notificationHelper.js (right):

http://codereview.adblockplus.org/5733084272001024/diff/6270652252160000/lib/...
lib/notificationHelper.js:229: NotificationStorage.showNext(url &&
stringifyURL(url));
On 2015/06/12 09:42:42, Sebastian Noack wrote:
> On 2015/06/12 09:42:06, Sebastian Noack wrote:
> > Oh wait, I were too quick. The case where url isn't given here doesn't exist
> > anymore. So maybe making the argument rudimentary renaming the function to
> > showNextNotificationForURL?
> 
> s/rudimentary/mandatory/

Somehow presumed it can be null. But judging from the code, it's always created
using new URL, so I've made it mandatory.

[Sebastian Noack, 2015-06-12 17:14:19]

http://codereview.adblockplus.org/5733084272001024/diff/5639274879778816/lib/...
File lib/notificationHelper.js (right):

http://codereview.adblockplus.org/5733084272001024/diff/5639274879778816/lib/...
lib/notificationHelper.js:223: * Shows the next notification for the supplied
URL, if any.
Mind removing ", if any", since its mandatory now?

http://codereview.adblockplus.org/5733084272001024/diff/5639274879778816/lib/...
lib/notificationHelper.js:225: * @param {URL} [url] URL to match notifications
to
Square brackets mean optional, which it isn't anymore.

[Felix Dahlke, 2015-06-12 17:56:10]

New patch set is up.

http://codereview.adblockplus.org/5733084272001024/diff/5639274879778816/lib/...
File lib/notificationHelper.js (right):

http://codereview.adblockplus.org/5733084272001024/diff/5639274879778816/lib/...
lib/notificationHelper.js:223: * Shows the next notification for the supplied
URL, if any.
On 2015/06/12 17:14:19, Sebastian Noack wrote:
> Mind removing ", if any", since its mandatory now?

The "if any" was targeted at the notification, since there might not be one. But
it doesn't really matter to the caller here, removed the part to make things
less ambiguous.

http://codereview.adblockplus.org/5733084272001024/diff/5639274879778816/lib/...
lib/notificationHelper.js:225: * @param {URL} [url] URL to match notifications
to
On 2015/06/12 17:14:19, Sebastian Noack wrote:
> Square brackets mean optional, which it isn't anymore.

Done.

[Sebastian Noack, 2015-06-12 18:12:50]

http://codereview.adblockplus.org/5733084272001024/diff/5639274879778816/lib/...
File lib/notificationHelper.js (right):

http://codereview.adblockplus.org/5733084272001024/diff/5639274879778816/lib/...
lib/notificationHelper.js:223: * Shows the next notification for the supplied
URL, if any.
On 2015/06/12 17:56:11, Felix H. Dahlke wrote:
> On 2015/06/12 17:14:19, Sebastian Noack wrote:
> > Mind removing ", if any", since its mandatory now?
> 
> The "if any" was targeted at the notification, since there might not be one.
But
> it doesn't really matter to the caller here, removed the part to make things
> less ambiguous.

Argh, you are right. I misread it. I think it makes sense to leave the "if any"
there. It's more accurate. And might be unexpected otherwise, if no notification
is shown.

[Felix Dahlke, 2015-06-12 18:24:08]

http://codereview.adblockplus.org/5733084272001024/diff/5639274879778816/lib/...
File lib/notificationHelper.js (right):

http://codereview.adblockplus.org/5733084272001024/diff/5639274879778816/lib/...
lib/notificationHelper.js:223: * Shows the next notification for the supplied
URL, if any.
On 2015/06/12 18:12:51, Sebastian Noack wrote:
> On 2015/06/12 17:56:11, Felix H. Dahlke wrote:
> > On 2015/06/12 17:14:19, Sebastian Noack wrote:
> > > Mind removing ", if any", since its mandatory now?
> > 
> > The "if any" was targeted at the notification, since there might not be one.
> But
> > it doesn't really matter to the caller here, removed the part to make things
> > less ambiguous.
> 
> Argh, you are right. I misread it. I think it makes sense to leave the "if
any"
> there. It's more accurate. And might be unexpected otherwise, if no
notification
> is shown.

Since the caller is not doing anything with the notification itself, I thought
it doesn't matter really. Anyway, think I've found a way to include this in an
unambiguous way now :)

[Sebastian Noack, 2015-06-12 18:49:43]

LGTM