Issue 3829 - Merge add-subscriptions and subscriptions.add messages

Issue 3829 - Merge add-subscriptions and subscriptions.add messages

[Sebastian Noack, 2016-03-17 21:41:18]

https://codereview.adblockplus.org/29338595/diff/29338596/messageResponder.js
File messageResponder.js (left):

https://codereview.adblockplus.org/29338595/diff/29338596/messageResponder.js...
messageResponder.js:302: if (message.url in FilterStorage.knownSubscriptions)
This check seems like a bug to me. If I understand the FilterStorage code
correctly the only thing that it does is ignoring subscriptions that have
previously been added, even if they got removed again. Also note that we removed
a similar check from the subscription initialization code.

https://codereview.adblockplus.org/29338595/diff/29338596/messageResponder.js...
messageResponder.js:306: if (!subscription)
Subscription.fromURL() never returns null or similar. Similar checks also have
been removed from the subscription initialization code.

[Thomas Greiner, 2016-03-18 10:48:20]

https://codereview.adblockplus.org/29338595/diff/29338596/background.js
File background.js (right):

https://codereview.adblockplus.org/29338595/diff/29338596/background.js#newco...
background.js:341: type: "subscription.add"
The message type is "subscriptions.add".

https://codereview.adblockplus.org/29338595/diff/29338596/messageResponder.js
File messageResponder.js (left):

https://codereview.adblockplus.org/29338595/diff/29338596/messageResponder.js...
messageResponder.js:302: if (message.url in FilterStorage.knownSubscriptions)
On 2016/03/17 21:41:17, Sebastian Noack wrote:
> This check seems like a bug to me. If I understand the FilterStorage code
> correctly the only thing that it does is ignoring subscriptions that have
> previously been added, even if they got removed again. Also note that we
removed
> a similar check from the subscription initialization code.

No, you're probably thinking about `Filter.knownFilters` or
`ElemHide.knownSubscriptions` which are never being cleared (see #3468). But
according to
https://hg.adblockplus.org/adblockpluscore/file/tip/lib/filterStorage.js#l171
`FilterStorage.knownSubscriptions` is cleared properly.

https://codereview.adblockplus.org/29338595/diff/29338596/messageResponder.js...
messageResponder.js:306: if (!subscription)
On 2016/03/17 21:41:17, Sebastian Noack wrote:
> Subscription.fromURL() never returns null or similar. Similar checks also have
> been removed from the subscription initialization code.

You're right, nice catch.

https://codereview.adblockplus.org/29338595/diff/29338596/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29338595/diff/29338596/messageResponder.js...
messageResponder.js:300: if (message.ask)
Detail: Just a personal preference but what do you think about "confirm" instead
since our dialog is equivalent in its purpose to `window.confirm()`?

https://codereview.adblockplus.org/29338595/diff/29338596/messageResponder.js...
messageResponder.js:302: ext.showOptions(onFilterChange.bind(null,
"addSubscription", subscription));
Coding style: Mind the maximum line length.

[Sebastian Noack, 2016-03-18 11:35:07]

https://codereview.adblockplus.org/29338595/diff/29338596/background.js
File background.js (right):

https://codereview.adblockplus.org/29338595/diff/29338596/background.js#newco...
background.js:341: type: "subscription.add"
On 2016/03/18 10:48:20, Thomas Greiner wrote:
> The message type is "subscriptions.add".

Argh, right.

https://codereview.adblockplus.org/29338595/diff/29338596/messageResponder.js
File messageResponder.js (left):

https://codereview.adblockplus.org/29338595/diff/29338596/messageResponder.js...
messageResponder.js:302: if (message.url in FilterStorage.knownSubscriptions)
On 2016/03/18 10:48:20, Thomas Greiner wrote:
> On 2016/03/17 21:41:17, Sebastian Noack wrote:
> > This check seems like a bug to me. If I understand the FilterStorage code
> > correctly the only thing that it does is ignoring subscriptions that have
> > previously been added, even if they got removed again. Also note that we
> removed
> > a similar check from the subscription initialization code.
> 
> No, you're probably thinking about `Filter.knownFilters` or
> `ElemHide.knownSubscriptions` which are never being cleared (see #3468). But
> according to
> https://hg.adblockplus.org/adblockpluscore/file/tip/lib/filterStorage.js#l171
> `FilterStorage.knownSubscriptions` is cleared properly.

You're right. However, FilterStorage.addSubscription already bails out if the
subscription is in FilterStorage.knownSubscriptions. So this check is still
redundant.

https://codereview.adblockplus.org/29338595/diff/29338596/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29338595/diff/29338596/messageResponder.js...
messageResponder.js:300: if (message.ask)
On 2016/03/18 10:48:20, Thomas Greiner wrote:
> Detail: Just a personal preference but what do you think about "confirm"
instead
> since our dialog is equivalent in its purpose to `window.confirm()`?

Fine with me.

https://codereview.adblockplus.org/29338595/diff/29338596/messageResponder.js...
messageResponder.js:302: ext.showOptions(onFilterChange.bind(null,
"addSubscription", subscription));
On 2016/03/18 10:48:20, Thomas Greiner wrote:
> Coding style: Mind the maximum line length.

Done.

[Thomas Greiner, 2016-03-18 13:20:39]

LGTM

https://codereview.adblockplus.org/29338595/diff/29338596/messageResponder.js
File messageResponder.js (left):

https://codereview.adblockplus.org/29338595/diff/29338596/messageResponder.js...
messageResponder.js:302: if (message.url in FilterStorage.knownSubscriptions)
On 2016/03/18 11:35:07, Sebastian Noack wrote:
> On 2016/03/18 10:48:20, Thomas Greiner wrote:
> > On 2016/03/17 21:41:17, Sebastian Noack wrote:
> > > This check seems like a bug to me. If I understand the FilterStorage code
> > > correctly the only thing that it does is ignoring subscriptions that have
> > > previously been added, even if they got removed again. Also note that we
> > removed
> > > a similar check from the subscription initialization code.
> > 
> > No, you're probably thinking about `Filter.knownFilters` or
> > `ElemHide.knownSubscriptions` which are never being cleared (see #3468). But
> > according to
> >
https://hg.adblockplus.org/adblockpluscore/file/tip/lib/filterStorage.js#l171
> > `FilterStorage.knownSubscriptions` is cleared properly.
> 
> You're right. However, FilterStorage.addSubscription already bails out if the
> subscription is in FilterStorage.knownSubscriptions. So this check is still
> redundant.

I see what you mean. Seems to be more of a design decision whether or not we'd
want the confirmation dialog to show up if a subscription has already been
added.
Currently, there's no feedback for the user that this is the case so it
shouldn't make too much of a difference whether the dialog is shown or not, I
assume.