Issue 29339287: Issue 3885 - Disable the checkbox to toggle the Acceptable Ads list on the options page (Closed)
|
Created:
April 1, 2016, 3:40 p.m. by Sebastian Noack Modified:
April 15, 2016, 4:54 p.m. Reviewers:
Thomas Greiner Visibility:
Public. |
DescriptionIssue 3885 - Disable the checkbox to toggle the Acceptable Ads list on the options page
Patch Set 1 #
Total comments: 4
Patch Set 2 : Put disabled items to the bottom of the list #
Total comments: 2
Patch Set 3 : Simplified logic, got rid of getAcceptableAdsURL() #Patch Set 4 : Rebased #
Total comments: 4
Patch Set 5 : Addressed comment #MessagesTotal messages: 7
https://codereview.adblockplus.org/29339287/diff/29339288/options.js File options.js (right): https://codereview.adblockplus.org/29339287/diff/29339288/options.js#newcode908 options.js:908: getPref("subscriptions_exceptionsurl", callback); Unrelated, but I couldn't resits. WTF?
https://codereview.adblockplus.org/29339287/diff/29339293/locale/en-US/option... File locale/en-US/options.json (left): https://codereview.adblockplus.org/29339287/diff/29339293/locale/en-US/option... locale/en-US/options.json:143: "description": "Show block Element option in Advanced tab", This is only due to rebasing. https://codereview.adblockplus.org/29339287/diff/29339293/options.js File options.js (right): https://codereview.adblockplus.org/29339287/diff/29339293/options.js#newcode89 options.js:89: var aDisabled = this._isControlDisabled(a, 0); This makes sure that items with disabled checkboxes (i.e. the Acceptable Ads list on the Advanced tab) goes last. Given that we already have the the "Own filter list" item below, which checkbox is disabled too, it makes sense and looks much nicer if we group those special cases. Moreover, these items are the least likely the user want to mess with here. https://codereview.adblockplus.org/29339287/diff/29339293/options.js#newcode94 options.js:94: var aTitle = this._getItemTitle(a, 0).toLowerCase(); This was missed on the previous review, where we started to show the original subscription title on the Advanced tab. Since items are sorted by their shown title, we have to consider the original title here as well.
https://codereview.adblockplus.org/29339287/diff/29339296/options.js File options.js (right): https://codereview.adblockplus.org/29339287/diff/29339296/options.js#newcode89 options.js:89: var aDisabled = this._isControlDisabled(a, 0); This makes sure that items with disabled checkboxes (i.e. the Acceptable Ads list on the Advanced tab) goes last. Given that we already have the the "Own filter list" item below, which checkbox is disabled too, it makes sense and looks much nicer if we group those special cases. Moreover, these items are the least likely the user want to mess with here. https://codereview.adblockplus.org/29339287/diff/29339296/options.js#newcode94 options.js:94: var aTitle = this._getItemTitle(a, 0).toLowerCase(); This was missed on the previous review, where we started to show the original subscription title on the Advanced tab. Since items are sorted by their shown title, we have to consider the original title here as well.
https://codereview.adblockplus.org/29339287/diff/29339288/options.js File options.js (right): https://codereview.adblockplus.org/29339287/diff/29339288/options.js#newcode908 options.js:908: getPref("subscriptions_exceptionsurl", callback); On 2016/04/01 15:41:56, Sebastian Noack wrote: > Unrelated, but I couldn't resits. WTF? Ahh, now I understand. The old code impelemted some caching by overriding the global getAcceptableAdsURL() function. However, removing the caching caused some weird side effect when adding custom subscriptions. But anyway, why don't we simply put the Acceptable Ads subscription URL in a global variable when initially populating the options page? I uploaded a new patch doing that. The code is much simpler now, and seems to work great.
https://codereview.adblockplus.org/29339287/diff/29339288/options.js File options.js (right): https://codereview.adblockplus.org/29339287/diff/29339288/options.js#newcode908 options.js:908: getPref("subscriptions_exceptionsurl", callback); On 2016/04/06 17:15:41, Sebastian Noack wrote: > On 2016/04/01 15:41:56, Sebastian Noack wrote: > > Unrelated, but I couldn't resits. WTF? > > Ahh, now I understand. The old code impelemted some caching by overriding the > global getAcceptableAdsURL() function. However, removing the caching caused some > weird side effect when adding custom subscriptions. But anyway, why don't we > simply put the Acceptable Ads subscription URL in a global variable when > initially populating the options page? I uploaded a new patch doing that. The > code is much simpler now, and seems to work great. Yeah, the idea here was to avoid messaging the background page each time when requesting the filter list URL but the implementation is a bit flawed in that it doesn't queue up calls that are made between the first message and the first message response. Using a global variable instead is perfectly fine with me. Alternatively, we could've also used Promises for that. https://codereview.adblockplus.org/29339287/diff/29339538/options.js File options.js (right): https://codereview.adblockplus.org/29339287/diff/29339538/options.js#newcode91 options.js:91: var bDisabled = this._isControlDisabled(b, 0); It's a nice idea to group together all disabled checkboxes but it's not ideal that we have to hardcode the table index since this special ordering is only useful for Acceptable Ads and only for this one table. That is especially since we intend to get rid of disabled checkboxes later on. What about instead checking specifically for the whitelist here to ensure that it's at the end of the list? e.g. this.items.sort(function(a, b) { if (a.url == acceptableAdsUrl) return 1; ... }); https://codereview.adblockplus.org/29339287/diff/29339538/options.js#newcode95 options.js:95: var aTitle = this._getItemTitle(a, 0).toLowerCase(); Detail: Hardcoding the table index here might cause items to be not alphabetically ordered. However, I assume that fixing this will require major changes in the way we keep the UI in sync with the data so feel free to ignore it for this review.
https://codereview.adblockplus.org/29339287/diff/29339288/options.js File options.js (right): https://codereview.adblockplus.org/29339287/diff/29339288/options.js#newcode908 options.js:908: getPref("subscriptions_exceptionsurl", callback); On 2016/04/07 17:21:15, Thomas Greiner wrote: > On 2016/04/06 17:15:41, Sebastian Noack wrote: > > On 2016/04/01 15:41:56, Sebastian Noack wrote: > > > Unrelated, but I couldn't resits. WTF? > > > > Ahh, now I understand. The old code impelemted some caching by overriding the > > global getAcceptableAdsURL() function. However, removing the caching caused > some > > weird side effect when adding custom subscriptions. But anyway, why don't we > > simply put the Acceptable Ads subscription URL in a global variable when > > initially populating the options page? I uploaded a new patch doing that. The > > code is much simpler now, and seems to work great. > > Yeah, the idea here was to avoid messaging the background page each time when > requesting the filter list URL but the implementation is a bit flawed in that it > doesn't queue up calls that are made between the first message and the first > message response. > > Using a global variable instead is perfectly fine with me. Alternatively, we > could've also used Promises for that. Well, a promise still requires a callback. Caching it into a global seems to be the simplest solution. https://codereview.adblockplus.org/29339287/diff/29339538/options.js File options.js (right): https://codereview.adblockplus.org/29339287/diff/29339538/options.js#newcode91 options.js:91: var bDisabled = this._isControlDisabled(b, 0); On 2016/04/07 17:21:15, Thomas Greiner wrote: > It's a nice idea to group together all disabled checkboxes but it's not ideal > that we have to hardcode the table index since this special ordering is only > useful for Acceptable Ads and only for this one table. That is especially since > we intend to get rid of disabled checkboxes later on. What about instead > checking specifically for the whitelist here to ensure that it's at the end of > the list? > > e.g. > this.items.sort(function(a, b) > { > if (a.url == acceptableAdsUrl) > return 1; > > ... > }); Yeah, hard-coding the index isn't great, but we have to do so for _getItemTitle() anyway. And the way I did it it was more obvious that we are grouping disabled/enabled items respectively. Now it need a comment to explain what that is about. But fair enough. Done. https://codereview.adblockplus.org/29339287/diff/29339538/options.js#newcode95 options.js:95: var aTitle = this._getItemTitle(a, 0).toLowerCase(); On 2016/04/07 17:21:15, Thomas Greiner wrote: > Detail: Hardcoding the table index here might cause items to be not > alphabetically ordered. However, I assume that fixing this will require major > changes in the way we keep the UI in sync with the data so feel free to ignore > it for this review. I know. This is kinda a hack. But at least more correct than previously.
LGTM 
|
