Issue 29346555: Issue 4156 - Adblocking filter only being removed in advanced tab fix (Closed)
|
Created:
June 15, 2016, 5:35 p.m. by saroyanm Modified:
Aug. 23, 2016, 1:15 p.m. Reviewers:
Thomas Greiner Visibility:
Public. |
DescriptionIssue 4156 - Adblocking filter only being removed in advanced tab fix
Patch Set 1 #
Total comments: 2
Patch Set 2 : Implementation simplification #
Total comments: 4
Patch Set 3 : Addressed Thomas comments #
Total comments: 14
Patch Set 4 : Merged updateLanguageCollections function #
Total comments: 2
Patch Set 5 : Reverted updateLanguageCollections function #Patch Set 6 : Rebased to changeset #85 #Patch Set 7 : Rebased to changeset #87 #Patch Set 8 : Restricted duplications #
Total comments: 6
Patch Set 9 : Changed addItems method to only accept 1 parameter and changed recommendations initial status #MessagesTotal messages: 19
Thomas can you please have a look, when you have time.
https://codereview.adblockplus.org/29346555/diff/29346556/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29346555/diff/29346556/new-options.js#newc... new-options.js:921: var knownSubscription = subscriptionsMap[subscription.url]; I'd prefer not having to deal with two different versions of the same subscription so I'd propose something along this line: if (subscription.url in subscriptionsMap) { subscription = subscriptionsMap[subscription.url]; ... } With that change the rest of the listener should be a bit cleaner since you no longer have to think about which subscription object you're supposed to use.
https://codereview.adblockplus.org/29346555/diff/29346556/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29346555/diff/29346556/new-options.js#newc... new-options.js:921: var knownSubscription = subscriptionsMap[subscription.url]; On 2016/06/16 11:09:33, Thomas Greiner wrote: > I'd prefer not having to deal with two different versions of the same > subscription so I'd propose something along this line: > > if (subscription.url in subscriptionsMap) > { > subscription = subscriptionsMap[subscription.url]; > ... > } > > With that change the rest of the listener should be a bit cleaner since you no > longer have to think about which subscription object you're supposed to use. Very good point!
https://codereview.adblockplus.org/29346555/diff/29346564/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29346555/diff/29346564/new-options.js#newc... new-options.js:947: if (subscription) This will always be `true`. Compare it with the base revision and you'll find that this check worked before. https://codereview.adblockplus.org/29346555/diff/29346564/new-options.js#newc... new-options.js:952: updateSubscription(subscription); I noticed that those two function calls swapped places. How is that related to the issue?
https://codereview.adblockplus.org/29346555/diff/29346564/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29346555/diff/29346564/new-options.js#newc... new-options.js:947: if (subscription) On 2016/06/16 16:10:34, Thomas Greiner wrote: > This will always be `true`. Compare it with the base revision and you'll find > that this check worked before. Well spotted, thanks, Done. https://codereview.adblockplus.org/29346555/diff/29346564/new-options.js#newc... new-options.js:952: updateSubscription(subscription); On 2016/06/16 16:10:34, Thomas Greiner wrote: > I noticed that those two function calls swapped places. How is that related to > the issue? Logic here is bit confusing and yes swiping the function calls doesn't make any easier or in this case sense. I've update the implementation, so currently I'm checking if the Subscription is of type=="ads" only then I'm calling the "updateLanguageCollections" method, which we were calling for most of the subscriptions which are not type of "ads" which making the logic confusing. Let me know if current implementation makes more sense from your point of view and if we can make keep this fix in this review as well, to make the code more understandable.
https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js File new-options.js (left): https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js#oldc... new-options.js:396: if (subscription.recommended == "ads") Why did you move this check out of here? This function only makes sense for "ad"-type subscriptions. https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js#newc... new-options.js:947: else if (subscription.url in subscriptionsMap) This else-if means that ad blocking subscriptions will only be updated in the language collections and therefore not in other ones such as the one in the Advanced tab… unless I'm misinterpreting this. Anyway, why not just call `updateLanguageCollections()` inside `updateSubscription()` and `addSubscription()`? https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js#newc... new-options.js:951: Coding style: Trailing white space.
https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js File new-options.js (left): https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js#oldc... new-options.js:396: if (subscription.recommended == "ads") On 2016/06/16 17:08:38, Thomas Greiner wrote: > Why did you move this check out of here? This function only makes sense for > "ad"-type subscriptions. For code readability, we were calling this function for every subscription with prev. implementation and it were not making sense IMO, the name of the function make it obvious that the method is updating LanguageCollection, so it should expect subscription to be of "ads" type, but let me know if you think that this is not making code more readable. https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js#newc... new-options.js:947: else if (subscription.url in subscriptionsMap) On 2016/06/16 17:08:38, Thomas Greiner wrote: > This else-if means that ad blocking subscriptions will only be updated in the > language collections and therefore not in other ones such as the one in the > Advanced tab… unless I'm misinterpreting this. Yes the Advanced tab will be updated by the call below: collections.filterLists.addItems(subscription); > Anyway, why not just call `updateLanguageCollections()` inside > `updateSubscription()` and `addSubscription()`? I see two reasons: 1. We call updateSubscription during "title", "lastDownload", "homepage", "downloadStatus" and "downloading" change events and this will trigger the call for the function which in the end will remoe or add the language. 2. I think as in the comment above, this is bit more readable/maintainable that we have different methods called for different subscriptions.
https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js File new-options.js (left): https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js#oldc... new-options.js:396: if (subscription.recommended == "ads") On 2016/06/16 17:42:11, saroyanm wrote: > On 2016/06/16 17:08:38, Thomas Greiner wrote: > > Why did you move this check out of here? This function only makes sense for > > "ad"-type subscriptions. > > For code readability, we were calling this function for every subscription with > prev. implementation and it were not making sense IMO, the name of the function > make it obvious that the method is updating LanguageCollection, so it should > expect subscription to be of "ads" type, but let me know if you think that this > is not making code more readable. Ok, what about getting rid of this function then altogether? There's not much in it anyway and it's used only in `onSubscriptionMessage()` - same for `updateSubscription()`. One way to achieve this would be as follows: switch (action) { case "added": case "disabled": case "downloading": ... case "title": if (subscription.url in subscriptionMap) // merge updateSubscription() here else addSubscription(subscription); if (subscription.recommended == "ads") // merge updateLanguageCollections() here break; case "removed": ... } Alternatively, we could stop using the switch-statement and do something more flexible like: if (action == "removed") { if (subscription.url == acceptableAdsUrl || subscription.recommended) { action = "disabled"; subscription.disabled = true; } else ... ... } if (action != "removed") { if (action == "added") addSubscription(subscription); else // merge updateSubscription() here if (subscription.recommended == "ads") // merge updateLanguageCollections() here } if (action == "added" || action == "disabled" || action == "removed") updateShareLinks();
https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js File new-options.js (left): https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js#oldc... new-options.js:396: if (subscription.recommended == "ads") On 2016/06/17 13:48:03, Thomas Greiner wrote: > On 2016/06/16 17:42:11, saroyanm wrote: > > On 2016/06/16 17:08:38, Thomas Greiner wrote: > > > Why did you move this check out of here? This function only makes sense for > > > "ad"-type subscriptions. > > > > For code readability, we were calling this function for every subscription > with > > prev. implementation and it were not making sense IMO, the name of the > function > > make it obvious that the method is updating LanguageCollection, so it should > > expect subscription to be of "ads" type, but let me know if you think that > this > > is not making code more readable. > > Ok, what about getting rid of this function then altogether? There's not much in > it anyway and it's used only in `onSubscriptionMessage()` - same for > `updateSubscription()`. > > One way to achieve this would be as follows: > > switch (action) > { > case "added": > case "disabled": > case "downloading": > ... > case "title": > if (subscription.url in subscriptionMap) > // merge updateSubscription() here > else > addSubscription(subscription); > > if (subscription.recommended == "ads") > // merge updateLanguageCollections() here > break; > case "removed": > ... > } > > Alternatively, we could stop using the switch-statement and do something more > flexible like: > > if (action == "removed") > { > if (subscription.url == acceptableAdsUrl || subscription.recommended) > { > action = "disabled"; > subscription.disabled = true; > } > else > ... > ... > } > > if (action != "removed") > { > if (action == "added") > addSubscription(subscription); > else > // merge updateSubscription() here > > if (subscription.recommended == "ads") > // merge updateLanguageCollections() here > } > > if (action == "added" || action == "disabled" || action == "removed") > updateShareLinks(); I think it's bit trickier than we thought,I've merged "updateLanguageCollections", but with "updateSubscription" as you suggested is bit trickier, because we can't merge action "disabled" and "added" without checking for that actions as well, because imagine the scenario when the title, or "downloadingStatus" will be changed in that case we will again remove and add items to "languages" related tables, that why we only need to add/delete items from that tables only if the item is being added/deleted/changed the "disabled" property. I've uploaded a new patch, so please let me know if changes in the patch are making the code more readable. If you think having separate "updateLanguageCollections" is better I'll revert the changes and keep it as it was initially.
https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js File new-options.js (left): https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js#oldc... new-options.js:396: if (subscription.recommended == "ads") On 2016/06/21 12:53:07, saroyanm wrote: > On 2016/06/17 13:48:03, Thomas Greiner wrote: > > On 2016/06/16 17:42:11, saroyanm wrote: > > > On 2016/06/16 17:08:38, Thomas Greiner wrote: > > > > Why did you move this check out of here? This function only makes sense > for > > > > "ad"-type subscriptions. > > > > > > For code readability, we were calling this function for every subscription > > with > > > prev. implementation and it were not making sense IMO, the name of the > > function > > > make it obvious that the method is updating LanguageCollection, so it should > > > expect subscription to be of "ads" type, but let me know if you think that > > this > > > is not making code more readable. > > > > Ok, what about getting rid of this function then altogether? There's not much > in > > it anyway and it's used only in `onSubscriptionMessage()` - same for > > `updateSubscription()`. > > > > One way to achieve this would be as follows: > > > > switch (action) > > { > > case "added": > > case "disabled": > > case "downloading": > > ... > > case "title": > > if (subscription.url in subscriptionMap) > > // merge updateSubscription() here > > else > > addSubscription(subscription); > > > > if (subscription.recommended == "ads") > > // merge updateLanguageCollections() here > > break; > > case "removed": > > ... > > } > > > > Alternatively, we could stop using the switch-statement and do something more > > flexible like: > > > > if (action == "removed") > > { > > if (subscription.url == acceptableAdsUrl || subscription.recommended) > > { > > action = "disabled"; > > subscription.disabled = true; > > } > > else > > ... > > ... > > } > > > > if (action != "removed") > > { > > if (action == "added") > > addSubscription(subscription); > > else > > // merge updateSubscription() here > > > > if (subscription.recommended == "ads") > > // merge updateLanguageCollections() here > > } > > > > if (action == "added" || action == "disabled" || action == "removed") > > updateShareLinks(); > > I think it's bit trickier than we thought,I've merged > "updateLanguageCollections", but with "updateSubscription" as you suggested is > bit trickier, because we can't merge action "disabled" and "added" without > checking for that actions as well, because imagine the scenario when the title, > or "downloadingStatus" will be changed in that case we will again remove and add > items to "languages" related tables, that why we only need to add/delete items > from that tables only if the item is being added/deleted/changed the "disabled" > property. > I've uploaded a new patch, so please let me know if changes in the patch are > making the code more readable. If you think having separate > "updateLanguageCollections" is better I'll revert the changes and keep it as it > was initially. Ok, I guess I assumed that `addItems()` would only add the subscription if it's not already in there but apparently it doesn't care about duplicates. Maybe we should fix that at some point but for now we can revert it to leave it as it is. https://codereview.adblockplus.org/29346555/diff/29346905/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29346555/diff/29346905/new-options.js#newc... new-options.js:944: collections.allLangs.removeItem(subscription); The subscription might be disabled when it's added so don't assume that it isn't. Reverting the merge of `updateLanguageCollections()` should fix that regression though.
https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js File new-options.js (left): https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js#oldc... new-options.js:396: if (subscription.recommended == "ads") On 2016/06/22 10:08:04, Thomas Greiner wrote: > On 2016/06/21 12:53:07, saroyanm wrote: > > On 2016/06/17 13:48:03, Thomas Greiner wrote: > > > On 2016/06/16 17:42:11, saroyanm wrote: > > > > On 2016/06/16 17:08:38, Thomas Greiner wrote: > > > > > Why did you move this check out of here? This function only makes sense > > for > > > > > "ad"-type subscriptions. > > > > > > > > For code readability, we were calling this function for every subscription > > > with > > > > prev. implementation and it were not making sense IMO, the name of the > > > function > > > > make it obvious that the method is updating LanguageCollection, so it > should > > > > expect subscription to be of "ads" type, but let me know if you think that > > > this > > > > is not making code more readable. > > > > > > Ok, what about getting rid of this function then altogether? There's not > much > > in > > > it anyway and it's used only in `onSubscriptionMessage()` - same for > > > `updateSubscription()`. > > > > > > One way to achieve this would be as follows: > > > > > > switch (action) > > > { > > > case "added": > > > case "disabled": > > > case "downloading": > > > ... > > > case "title": > > > if (subscription.url in subscriptionMap) > > > // merge updateSubscription() here > > > else > > > addSubscription(subscription); > > > > > > if (subscription.recommended == "ads") > > > // merge updateLanguageCollections() here > > > break; > > > case "removed": > > > ... > > > } > > > > > > Alternatively, we could stop using the switch-statement and do something > more > > > flexible like: > > > > > > if (action == "removed") > > > { > > > if (subscription.url == acceptableAdsUrl || subscription.recommended) > > > { > > > action = "disabled"; > > > subscription.disabled = true; > > > } > > > else > > > ... > > > ... > > > } > > > > > > if (action != "removed") > > > { > > > if (action == "added") > > > addSubscription(subscription); > > > else > > > // merge updateSubscription() here > > > > > > if (subscription.recommended == "ads") > > > // merge updateLanguageCollections() here > > > } > > > > > > if (action == "added" || action == "disabled" || action == "removed") > > > updateShareLinks(); > > > > I think it's bit trickier than we thought,I've merged > > "updateLanguageCollections", but with "updateSubscription" as you suggested is > > bit trickier, because we can't merge action "disabled" and "added" without > > checking for that actions as well, because imagine the scenario when the > title, > > or "downloadingStatus" will be changed in that case we will again remove and > add > > items to "languages" related tables, that why we only need to add/delete items > > from that tables only if the item is being added/deleted/changed the > "disabled" > > property. > > I've uploaded a new patch, so please let me know if changes in the patch are > > making the code more readable. If you think having separate > > "updateLanguageCollections" is better I'll revert the changes and keep it as > it > > was initially. > > Ok, I guess I assumed that `addItems()` would only add the subscription if it's > not already in there but apparently it doesn't care about duplicates. I agree, but if we only fix that we will fix simptome of the current duplication, when the item is being added, so first I'll suggest find reason of why the duplication taking place in first place in the corresponding ticket, but I wonder do we expect the subscription to be added twice ? If not I think we should worry about that. > Maybe we should fix that at some point but for now we can revert it to leave it > as it is. https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js#newc... new-options.js:951: On 2016/06/16 17:08:38, Thomas Greiner wrote: > Coding style: Trailing white space. Done. https://codereview.adblockplus.org/29346555/diff/29346905/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29346555/diff/29346905/new-options.js#newc... new-options.js:944: collections.allLangs.removeItem(subscription); On 2016/06/22 10:08:04, Thomas Greiner wrote: > The subscription might be disabled when it's added so don't assume that it > isn't. Reverting the merge of `updateLanguageCollections()` should fix that > regression though. Well spotted, done
https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js#newc... new-options.js:947: else if (subscription.url in subscriptionsMap) On 2016/06/16 17:42:11, saroyanm wrote: > On 2016/06/16 17:08:38, Thomas Greiner wrote: > > This else-if means that ad blocking subscriptions will only be updated in the > > language collections and therefore not in other ones such as the one in the > > Advanced tab… unless I'm misinterpreting this. > Yes the Advanced tab will be updated by the call below: > collections.filterLists.addItems(subscription); Why isn't it updated by `addSubscription()` and `updateSubscription()`? Seems like those are the ones responsible for adding and updating subscriptions in collections.
https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js#newc... new-options.js:947: else if (subscription.url in subscriptionsMap) On 2016/06/23 14:54:29, Thomas Greiner wrote: > On 2016/06/16 17:42:11, saroyanm wrote: > > On 2016/06/16 17:08:38, Thomas Greiner wrote: > > > This else-if means that ad blocking subscriptions will only be updated in > the > > > language collections and therefore not in other ones such as the one in the > > > Advanced tab… unless I'm misinterpreting this. > > Yes the Advanced tab will be updated by the call below: > > collections.filterLists.addItems(subscription); > > Why isn't it updated by `addSubscription()` and `updateSubscription()`? Seems > like those are the ones responsible for adding and updating subscriptions in > collections. We are also adding recommendations using "addSubscription", the recommendations are only being shown in the General tab initially. With updateSubscription is bit different, it's updating the subscription, as well it's updates the list in advanced tab, but it's not adding items, so this was created I assume to remove code duplications from the "onSubscriptionMessage". So we have several solutions IMO: 1. Add second "action" parameter to "updateLanguageCollections", "updateSubscription" and "addSubscription", check for "added" action that has been passed to the "onSubscriptionMessage" function. 2. Move "updateLanguageCollections", "updateSubscription" and "addSubscription" functions back to the "onSubscriptionMessage" provide code duplications. 3. Just rename "addSubscription" functions to "addGeneralTabSubscription", so we will have 3 functions: "updateLanguageCollections" - which obviously updating 2 collections in General tab, because we do not have other Language collections "updateSubscription" - this function is updating, but not adding item "addGeneralTabSubscription" - self explanatory. What you think ?
Rebased to changeset #85
https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js#newc... new-options.js:947: else if (subscription.url in subscriptionsMap) On 2016/06/23 16:27:12, saroyanm wrote: > We are also adding recommendations using "addSubscription", the recommendations > are only being shown in the General tab initially. > With updateSubscription is bit different, it's updating the subscription, as > well it's updates the list in advanced tab, but it's not adding items, so this > was created I assume to remove code duplications from the > "onSubscriptionMessage". > So we have several solutions IMO: > 1. Add second "action" parameter to "updateLanguageCollections", > "updateSubscription" and "addSubscription", check for "added" action that has > been passed to the "onSubscriptionMessage" function. > 2. Move "updateLanguageCollections", "updateSubscription" and "addSubscription" > functions back to the "onSubscriptionMessage" provide code duplications. > 3. Just rename "addSubscription" functions to "addGeneralTabSubscription", so we > will have 3 functions: > "updateLanguageCollections" - which obviously updating 2 collections in General > tab, because we do not have other Language collections > "updateSubscription" - this function is updating, but not adding item > "addGeneralTabSubscription" - self explanatory. > > What you think ? Ok, so the current behavior is: - addSubscription: adds subscription - updateSubscription: updates subscription - updateLanguageCollections: adds and updates language collections So what I'd go for is the following: - addSubscription: adds subscription + calls `toggleShowLanguage(subscription, true)` - updateSubscription: updates subscription + calls `toggleShowLanguage(subscription)` For that you just need to change `updateLanguageCollections(Subscription subscription)` to `toggleShowLanguage(Subscription subscription, boolean force)` like in `Element.classList.toggle()` and `Notification.toggleIgnoreCategory()` (see https://hg.adblockplus.org/adblockpluscore/file/tip/lib/notification.js#l391). Thereby you could avoid code duplication, ensure consistency and clarify what the code does.
Patch uploaded. https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js#newc... new-options.js:947: else if (subscription.url in subscriptionsMap) On 2016/07/13 11:35:18, Thomas Greiner wrote: > On 2016/06/23 16:27:12, saroyanm wrote: > > We are also adding recommendations using "addSubscription", the > recommendations > > are only being shown in the General tab initially. > > With updateSubscription is bit different, it's updating the subscription, as > > well it's updates the list in advanced tab, but it's not adding items, so this > > was created I assume to remove code duplications from the > > "onSubscriptionMessage". > > So we have several solutions IMO: > > 1. Add second "action" parameter to "updateLanguageCollections", > > "updateSubscription" and "addSubscription", check for "added" action that has > > been passed to the "onSubscriptionMessage" function. > > 2. Move "updateLanguageCollections", "updateSubscription" and > "addSubscription" > > functions back to the "onSubscriptionMessage" provide code duplications. > > 3. Just rename "addSubscription" functions to "addGeneralTabSubscription", so > we > > will have 3 functions: > > "updateLanguageCollections" - which obviously updating 2 collections in > General > > tab, because we do not have other Language collections > > "updateSubscription" - this function is updating, but not adding item > > "addGeneralTabSubscription" - self explanatory. > > > > What you think ? > > Ok, so the current behavior is: > - addSubscription: adds subscription > - updateSubscription: updates subscription > - updateLanguageCollections: adds and updates language collections > > So what I'd go for is the following: > - addSubscription: adds subscription + calls `toggleShowLanguage(subscription, > true)` > - updateSubscription: updates subscription + calls > `toggleShowLanguage(subscription)` > > For that you just need to change `updateLanguageCollections(Subscription > subscription)` to `toggleShowLanguage(Subscription subscription, boolean force)` > like in `Element.classList.toggle()` and `Notification.toggleIgnoreCategory()` > (see > https://hg.adblockplus.org/adblockpluscore/file/tip/lib/notification.js#l391). > > Thereby you could avoid code duplication, ensure consistency and clarify what > the code does. 1. I've moved the call for updateLanguageCollection to "addSubscription" and "updateSubscription" functions, 2. reverted old check for the ads type in "updateLanguageCollection" 3. Renamed updateLanguageCollections to toggleShowLanguage 4. restricted duplications in the collection's "add" method, to avoid duplications caused by multiple calls of "toggleShowLanguage" method
Looks good now. Only two minor things I noted. https://codereview.adblockplus.org/29346555/diff/29348942/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29346555/diff/29348942/new-options.js#newc... new-options.js:73: Collection.prototype.addItems = function() Note: We should really change this function to only accept a single argument because that's the only way we're currently using it. That should make the content of this function simpler and avoid this unnecessary duplication that we now introduced. Anyway, that can be tackled in a separate ticket. Just wanted to mention it. https://codereview.adblockplus.org/29346555/diff/29348942/new-options.js#newc... new-options.js:76: for (var i = 0; i < arguments.length; i++) Detail: I don't mind omitting the braces for a single line but, to avoid confusion, please use them when the content stretches across multiple lines. https://codereview.adblockplus.org/29346555/diff/29348942/new-options.js#newc... new-options.js:347: if (subscription.disabled == false) Detail: Do we need to explicitly check for `false`? Because that's the only thing that's changed about this function when comparing it with the base revision. Also note that it's a double negation which would be great to avoid to make it easier to understand.
https://codereview.adblockplus.org/29346555/diff/29348942/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29346555/diff/29348942/new-options.js#newc... new-options.js:73: Collection.prototype.addItems = function() On 2016/08/04 15:54:02, Thomas Greiner wrote: > Note: We should really change this function to only accept a single argument > because that's the only way we're currently using it. That should make the > content of this function simpler and avoid this unnecessary duplication that we > now introduced. > > Anyway, that can be tackled in a separate ticket. Just wanted to mention it. Done. https://codereview.adblockplus.org/29346555/diff/29348942/new-options.js#newc... new-options.js:76: for (var i = 0; i < arguments.length; i++) On 2016/08/04 15:54:02, Thomas Greiner wrote: > Detail: I don't mind omitting the braces for a single line but, to avoid > confusion, please use them when the content stretches across multiple lines. This was fixed during previous comment implementation https://codereview.adblockplus.org/29346555/diff/29348942/new-options.js#newc... new-options.js:347: if (subscription.disabled == false) On 2016/08/04 15:54:03, Thomas Greiner wrote: > Detail: Do we need to explicitly check for `false`? Because that's the only > thing that's changed about this function when comparing it with the base > revision. > > Also note that it's a double negation which would be great to avoid to make it > easier to understand. This change was required because with latest changes we were also calling this function during recommendation initialization. The problem is that when we are creating recommendation object we are assigning "null" to the disabled status of the recommendation, but I can't see any reason why this needs to be done that way, so I changed to assign disable=true during initial initialization of the recommendations.
LGTM 
|
