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

Issue 29338595: Issue 3829 - Merge add-subscriptions and subscriptions.add messages (Closed)

Created:
March 17, 2016, 9:20 p.m. by Sebastian Noack
Modified:
March 18, 2016, 1:32 p.m.
Reviewers:
Thomas Greiner
CC:
kzar
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 12

Patch Set 2 : Addressed comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+17 lines, -19 lines) Patch
M background.js View 1 1 chunk +2 lines, -1 line 0 comments Download
M messageResponder.js View 1 2 chunks +15 lines, -18 lines 0 comments Download

Messages

Total messages: 4
Sebastian Noack
https://codereview.adblockplus.org/29338595/diff/29338596/messageResponder.js File messageResponder.js (left): https://codereview.adblockplus.org/29338595/diff/29338596/messageResponder.js#oldcode302 messageResponder.js:302: if (message.url in FilterStorage.knownSubscriptions) This check seems like a ...
March 17, 2016, 9:41 p.m. (2016-03-17 21:41:18 UTC) #1
Thomas Greiner
https://codereview.adblockplus.org/29338595/diff/29338596/background.js File background.js (right): https://codereview.adblockplus.org/29338595/diff/29338596/background.js#newcode341 background.js:341: type: "subscription.add" The message type is "subscriptions.add". https://codereview.adblockplus.org/29338595/diff/29338596/messageResponder.js File ...
March 18, 2016, 10:48 a.m. (2016-03-18 10:48:20 UTC) #2
Sebastian Noack
https://codereview.adblockplus.org/29338595/diff/29338596/background.js File background.js (right): https://codereview.adblockplus.org/29338595/diff/29338596/background.js#newcode341 background.js:341: type: "subscription.add" On 2016/03/18 10:48:20, Thomas Greiner wrote: > ...
March 18, 2016, 11:35 a.m. (2016-03-18 11:35:07 UTC) #3
Thomas Greiner
March 18, 2016, 1:20 p.m. (2016-03-18 13:20:39 UTC) #4
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.

Powered by Google App Engine
This is Rietveld