Issue 29615620: issue 6075 - Hide Acceptable Ads notification when corresponding subscription is removed (Closed)
|
Created:
Nov. 22, 2017, 1:27 p.m. by saroyanm Modified:
Nov. 24, 2017, 12:23 p.m. Reviewers:
Thomas Greiner Visibility:
Public. |
Descriptionissue 6075 - Hide Acceptable Ads notification when corresponding subscription is removed
Patch Set 1 #
Total comments: 8
Patch Set 2 : Addressed Thomas comments #
Total comments: 4
Patch Set 3 : Addressed Thomas comments #
Total comments: 2
Patch Set 4 : Fixed characters limit #MessagesTotal messages: 8
Thomas can you please have a look, I think it's a quick one.
https://codereview.adblockplus.org/29615620/diff/29615621/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29615620/diff/29615621/desktop-options.js#... desktop-options.js:1311: case "removed": It's a shame we missed that in our last review so it's great that we're tackling this now. However, note that the conflict can also be resolved by disabling/enabling one of the filter lists so we also need to react to those messages. https://codereview.adblockplus.org/29615620/diff/29615621/desktop-options.js#... desktop-options.js:1332: } Why not instead introduce a function similar to `setAcceptableAds()` where we simply update the state of the warning? Thereby we don't have to duplicate the element's ID, its class name and this logic throughout the code.
New patch is ready for review. https://codereview.adblockplus.org/29615620/diff/29615621/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29615620/diff/29615621/desktop-options.js#... desktop-options.js:1311: case "removed": On 2017/11/23 12:53:39, Thomas Greiner wrote: > It's a shame we missed that in our last review so it's great that we're tackling > this now. Right. > However, note that the conflict can also be resolved by disabling/enabling one > of the filter lists so we also need to react to those messages. Good point, done. https://codereview.adblockplus.org/29615620/diff/29615621/desktop-options.js#... desktop-options.js:1332: } On 2017/11/23 12:53:39, Thomas Greiner wrote: > Why not instead introduce a function similar to `setAcceptableAds()` where we > simply update the state of the warning? Thereby we don't have to duplicate the > element's ID, its class name and this logic throughout the code. Good point, done.
https://codereview.adblockplus.org/29615620/diff/29615621/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29615620/diff/29615621/desktop-options.js#... desktop-options.js:1106: return subscription.url == acceptableAdsUrl || Suggestion: If you want you can pass an optional `subscription` parameter to `setPrivacyConflict()` if you want to keep this logic around. https://codereview.adblockplus.org/29615620/diff/29615621/desktop-options.js#... desktop-options.js:1110: function hasPrivacyConflict() I wouldn't mind keeping the privacy conflict check separate to make `setPrivacyConflict()` easier to read. Because right now there's quite a lot happening in there. https://codereview.adblockplus.org/29615620/diff/29616574/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29615620/diff/29616574/desktop-options.js#... desktop-options.js:1117: E("acceptable-ads").classList.remove("show-warning"); Detail: We can avoid this unnecessary call by moving it into the "else" case and using `Element.classList.toggle()` instead of `Element.classList.add()`. https://codereview.adblockplus.org/29615620/diff/29616574/desktop-options.js#... desktop-options.js:1277: setPrivacyConflict(); Detail: Let's do this after we've updated the UI. Because in case an error occurs in either we probably care more about the UI being up-to-date rather than the warning being shown. Same applies to the other places where we call `setPrivacyConflict()`.
Thanks Thomas. New patch is ready for the review. https://codereview.adblockplus.org/29615620/diff/29615621/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29615620/diff/29615621/desktop-options.js#... desktop-options.js:1106: return subscription.url == acceptableAdsUrl || On 2017/11/23 14:46:12, Thomas Greiner wrote: > Suggestion: If you want you can pass an optional `subscription` parameter to > `setPrivacyConflict()` if you want to keep this logic around. Not sure if it will help much and it might complicate the logic IMO as we will have both logic there the one that extract from "known" subscriptions and the other one which check which subscription was trigger (we might need another argument to know what triggered the subscription addition/removal to make use of it). https://codereview.adblockplus.org/29615620/diff/29615621/desktop-options.js#... desktop-options.js:1110: function hasPrivacyConflict() On 2017/11/23 14:46:12, Thomas Greiner wrote: > I wouldn't mind keeping the privacy conflict check separate to make > `setPrivacyConflict()` easier to read. Because right now there's quite a lot > happening in there. Agree, done. https://codereview.adblockplus.org/29615620/diff/29616574/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29615620/diff/29616574/desktop-options.js#... desktop-options.js:1117: E("acceptable-ads").classList.remove("show-warning"); On 2017/11/23 14:46:12, Thomas Greiner wrote: > Detail: We can avoid this unnecessary call by moving it into the "else" case and > using `Element.classList.toggle()` instead of `Element.classList.add()`. I agree, well spotted, done. https://codereview.adblockplus.org/29615620/diff/29616574/desktop-options.js#... desktop-options.js:1277: setPrivacyConflict(); On 2017/11/23 14:46:12, Thomas Greiner wrote: > Detail: Let's do this after we've updated the UI. Because in case an error > occurs in either we probably care more about the UI being up-to-date rather than > the warning being shown. > > Same applies to the other places where we call `setPrivacyConflict()`. DOne.
Just a coding style issue left with a suggestion. https://codereview.adblockplus.org/29615620/diff/29616587/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29615620/diff/29616587/desktop-options.js#... desktop-options.js:1127: E("acceptable-ads").classList.toggle("show-warning", showTrackingWarning); Coding style: "Line length: 80 characters or less" I'd suggest resolving this issue by putting `let warning = E("acceptable-ads");` at the beginning of the function since we need to call `E("acceptable-ads")` no matter what so thereby we'd get rid of the duplicated call.
New patch uploaded. https://codereview.adblockplus.org/29615620/diff/29616587/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29615620/diff/29616587/desktop-options.js#... desktop-options.js:1127: E("acceptable-ads").classList.toggle("show-warning", showTrackingWarning); On 2017/11/23 16:51:30, Thomas Greiner wrote: > Coding style: "Line length: 80 characters or less" > > I'd suggest resolving this issue by putting `let warning = E("acceptable-ads");` > at the beginning of the function since we need to call `E("acceptable-ads")` no > matter what so thereby we'd get rid of the duplicated call. Agree, we used "acceptableAdsForm" below so I'll use it here as well, alternatively I can update all instances to "acceptableAdsSection", "acceptableAdsControl", but both will exceed the limit :/
LGTM 
|
