Issue 6088024630755328: issue 1526 - Implement new options page design for Chrome/Opera/Safari (Closed)
|
Created:
Jan. 9, 2015, 5:11 p.m. by saroyanm Modified:
June 22, 2015, 12:57 p.m. CC:
sven, Wladimir Palant Visibility:
Public. |
DescriptionThis review is related to current ticket:
https://issues.adblockplus.org/ticket/1526
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 78
Patch Set 4 : #
Total comments: 15
Patch Set 5 : #Patch Set 6 : #
Total comments: 31
Patch Set 7 : #
Total comments: 69
Patch Set 8 : #
Total comments: 124
Patch Set 9 : #
Total comments: 4
Patch Set 10 : #
Total comments: 86
Patch Set 11 : #
Total comments: 6
Patch Set 12 : #
Total comments: 62
Patch Set 13 : Adderessed Felix initial comments #Patch Set 14 : Composition over inheritance #
Total comments: 1
Patch Set 15 : array declaration #Patch Set 16 : Comment about solution being temprorary is added to subscriptions.xml #
MessagesTotal messages: 38
Initial review uploaded. It's not complete, but you can see how the implementation looks now.
The front end part for general tab is ready to be reviewed, currently it don't remove the subscription when it's unchecked because we still need to discuss if we need disable subscription when unchecking from general tab or remove it, for now missing also implementation for "read more" popup and also share button functionality, but this can discuss on separate review I guess. For now we have mostly functional general tab, after this review things will go much smoother and faster as this is suppose to be biggest review so far.
Just updated the patch with some fixes. Acceptable Ads checkbox missbehave were fixed.
Here are my comments on options.json and options.html so that you can already get started with those. I still need to review the other files, however. Note that I didn't look at the Advanced tab at all, of course, but I did review the Help tab except for the links since they haven't been implemented yet so I just focused on the design here for now. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/loca... File locale/ar/options.json (right): http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/loca... locale/ar/options.json:1: { Is this file really necessary? http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/loca... File locale/en-US/options.json (right): http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/loca... locale/en-US/options.json:1: { Please add a description to translations to provide some context to translators. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/loca... locale/en-US/options.json:36: "message": "for websites in..." Don't separate the above two. Rather merge them into "<strong>Adblocking</strong> for websites in..." http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/loca... locale/en-US/options.json:48: "message": "Exception" Nit: "Exceptions" http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/loca... locale/en-US/options.json:69: "message": "Block Element" Again, the order is important for the two texts above so would be better to merge them into "Show <a>Block Element</a> right-click menu item" http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/loca... locale/en-US/options.json:75: "message": "Hide whitespace" The above two are not going to be part of the options page since we removed that checkbox from the existing one. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/loca... locale/en-US/options.json:111: "message": "You only need to refresh your blocking list in \"Advanced\" very often, but there are also other known problems." We need a better text here. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/loca... locale/en-US/options.json:120: "message": "If the frequently asked questions helped you can reach us in our forum." We need a better text here. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/loca... locale/en-US/options.json:129: "message": "If there are major problems with Adblock Plus which affects all of our users, we tell you in our social media channels." We need a better text here. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/loca... locale/en-US/options.json:153: "message": "Added Languages" Nit: "Added languages" http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/loca... locale/en-US/options.json:201: "message": "Ruassian and Ukrainian" Nit: "Russian" http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... File options.html (right): http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:1: <!DOCTYPE html> This page contains a couple of subpages so I'd find it helpful to have comments to mark when the code for the General, Advanced and Help tabs begin. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:3: - This file is part of Adblock Plus <http://adblockplus.org/>, Nit: Change to "https" http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:24: <script type="text/javascript" src="ext/common.js"></script> No need to specify the <script> type http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:34: <h1>Adblock <b>Plus</b></h1> This text needs to be taken from "locale/en-US/options.json" http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:37: <li id="tab-general" data-show="content-general" class="active"><span class="i18n_options_tab_general"></span><span class="icon"></span></li> Don't put every list item in one single line to improve legibility. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:41: <p class="nav-blue"> Don't include colors in class names because the color might change. Rather use more generic names like "nav-link". http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:42: <span class="i18n_options_version"></span> <span id="version">2.5.10</span> Don't hardcode the version number. Request it from the background page using: ext.backgroundPage.sendMessage({ method: "app.get", what: "addonVersion" }, function(addonVersion) { ... }); I'll add the required backend code for that. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:54: <div> You're wrapping everything inside a <div> but don't indent it… that raises the question: Is this <div> really necessary? http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:56: <hr> You could use "border-bottom" instead of <hr> here and for any other occurence. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:58: <div id="blocking-languages" class="left"> Don't use "left" or "right" in class names since we're building a page that will also work in an RTL environment so "left" could actually be "right" and vice versa. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:59: <p class="option-name"><b class="i18n_options_language_adblocking"></b> <span class="i18n_options_language_title"></span> <a class="i18n_options_readMore dotted" href="#" target="_blank"></a></p> Generally, we avoid using HTML elements that convey styling. So please use <strong> instead of <b>. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:63: <div class="button" id="add-website-language" data-show="blocking-languages-modal"> It'd be great if you could use standard HTML elements like <button> for this to keep it accessible. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:69: <p class="option-name"><b class="i18n_options_furtherBlocking_title"></b> <a class="i18n_options_readMore dotted" href="#" target="_blank"></a></p> Again, don't use implementation specific details like "dotted" in class names. Those might change at any point. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:88: <li><input type='checkbox'/><label id="allow-whitelist-cb"></label><span>Allow some non-intrusive advertising</span></li> Use " instead of ' for consistency http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:88: <li><input type='checkbox'/><label id="allow-whitelist-cb"></label><span>Allow some non-intrusive advertising</span></li> Actually, the <span> should be the label for the checkbox. That's at least what <label> is intended for. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:100: <div style="display: flex;"> All styles should be inside a CSS file to separate structure and styles. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:101: <button class="addbtn" id="whitelisting-add-btn">+<span class="i18n_options_btn_add"></span></button> Why not "button-add"? That should be more consistent with the other class names you're using. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:177: <h1>1. <span class="i18n_options_faq"></span></h1> No need to do manual enumeration. Just use <ol> or CSS counters (see https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Counters). http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:181: <span class="icon icon-arrow"></span> This can be added using the "::before" pseudo element to ensure consistency for all similar links. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:206: <div> We use two spaces for indentation. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:207: <div class="header"> Use the <header> element instead to stick to standard markup. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:210: <span id="modal-title"></span> This structure is quite confusing and looks somewhat redundant. Can't you trim it down? http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:213: <span class="separator"></span> I guess this can be achieved using a border. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:221: <div class="content" id="modal-content"> What about the dialog for "abp:subscribe" links? I know that we had a separate mockup for that one to prevent editing of the filterlist title and URL. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:222: <div style="display: none;" id="blocking-languages-modal" data-title="options_modal_language_title"> What do you need the "data-title" attribute here for? http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... File options.js (right): http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.js:2: * This file is part of Adblock Plus <http://adblockplus.org/>, Nit: Change to "https" http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/skin... File skin/options.css (right): http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/skin... skin/options.css:1: body License header is missing.
Thanks for having look Thomas, I've updated the patch, please note that in options.json I didn't added descriptions yet, will provide them with next patch, so in case you can work with updated HTML file if you will start to work on CSS part. Also didn't updated text in help tab yet where you mentioned "We need a better text here". http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/loca... File locale/ar/options.json (right): http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/loca... locale/ar/options.json:1: { On 2015/01/22 17:56:30, Thomas Greiner wrote: > Is this file really necessary? No, it's not necessary. It just will shows Error when testing RTL direction with "ar" locale, but still testable. Removed http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/loca... File locale/en-US/options.json (right): http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/loca... locale/en-US/options.json:1: { On 2015/01/22 17:56:30, Thomas Greiner wrote: > Please add a description to translations to provide some context to translators. Will provide description with next patch. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/loca... locale/en-US/options.json:36: "message": "for websites in..." On 2015/01/22 17:56:30, Thomas Greiner wrote: > Don't separate the above two. Rather merge them into > "<strong>Adblocking</strong> for websites in..." Done. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/loca... locale/en-US/options.json:48: "message": "Exception" On 2015/01/22 17:56:30, Thomas Greiner wrote: > Nit: "Exceptions" Done. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/loca... locale/en-US/options.json:69: "message": "Block Element" On 2015/01/22 17:56:30, Thomas Greiner wrote: > Again, the order is important for the two texts above so would be better to > merge them into "Show <a>Block Element</a> right-click menu item" Done. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/loca... locale/en-US/options.json:75: "message": "Hide whitespace" On 2015/01/22 17:56:30, Thomas Greiner wrote: > The above two are not going to be part of the options page since we removed that > checkbox from the existing one. Done. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/loca... locale/en-US/options.json:153: "message": "Added Languages" On 2015/01/22 17:56:30, Thomas Greiner wrote: > Nit: "Added languages" Done. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/loca... locale/en-US/options.json:201: "message": "Ruassian and Ukrainian" On 2015/01/22 17:56:30, Thomas Greiner wrote: > Nit: "Russian" Done. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... File options.html (right): http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:1: <!DOCTYPE html> On 2015/01/22 17:56:30, Thomas Greiner wrote: > This page contains a couple of subpages so I'd find it helpful to have comments > to mark when the code for the General, Advanced and Help tabs begin. Done. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:3: - This file is part of Adblock Plus <http://adblockplus.org/>, On 2015/01/22 17:56:30, Thomas Greiner wrote: > Nit: Change to "https" Done. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:24: <script type="text/javascript" src="ext/common.js"></script> On 2015/01/22 17:56:30, Thomas Greiner wrote: > No need to specify the <script> type Done. We are using type attribute also in other HTML files, should we also remove them for consistency ? http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:34: <h1>Adblock <b>Plus</b></h1> On 2015/01/22 17:56:30, Thomas Greiner wrote: > This text needs to be taken from "locale/en-US/options.json" Done. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:37: <li id="tab-general" data-show="content-general" class="active"><span class="i18n_options_tab_general"></span><span class="icon"></span></li> On 2015/01/22 17:56:30, Thomas Greiner wrote: > Don't put every list item in one single line to improve legibility. Done. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:41: <p class="nav-blue"> On 2015/01/22 17:56:30, Thomas Greiner wrote: > Don't include colors in class names because the color might change. Rather use > more generic names like "nav-link". Done. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:42: <span class="i18n_options_version"></span> <span id="version">2.5.10</span> On 2015/01/22 17:56:30, Thomas Greiner wrote: > Don't hardcode the version number. Request it from the background page using: > > ext.backgroundPage.sendMessage({ > method: "app.get", > what: "addonVersion" > }, function(addonVersion) > { > ... > }); > > I'll add the required backend code for that. Done. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:54: <div> On 2015/01/22 17:56:30, Thomas Greiner wrote: > You're wrapping everything inside a <div> but don't indent it… that raises the > question: Is this <div> really necessary? Thanks, indentation was missing it wraps Blocking and Exception sections, for consistency. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:56: <hr> On 2015/01/22 17:56:30, Thomas Greiner wrote: > You could use "border-bottom" instead of <hr> here and for any other occurence. Done. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:58: <div id="blocking-languages" class="left"> On 2015/01/22 17:56:30, Thomas Greiner wrote: > Don't use "left" or "right" in class names since we're building a page that will > also work in an RTL environment so "left" could actually be "right" and vice > versa. Done. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:59: <p class="option-name"><b class="i18n_options_language_adblocking"></b> <span class="i18n_options_language_title"></span> <a class="i18n_options_readMore dotted" href="#" target="_blank"></a></p> On 2015/01/22 17:56:30, Thomas Greiner wrote: > Generally, we avoid using HTML elements that convey styling. So please use > <strong> instead of <b>. Done. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:63: <div class="button" id="add-website-language" data-show="blocking-languages-modal"> On 2015/01/22 17:56:30, Thomas Greiner wrote: > It'd be great if you could use standard HTML elements like <button> for this to > keep it accessible. Done. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:69: <p class="option-name"><b class="i18n_options_furtherBlocking_title"></b> <a class="i18n_options_readMore dotted" href="#" target="_blank"></a></p> On 2015/01/22 17:56:30, Thomas Greiner wrote: > Again, don't use implementation specific details like "dotted" in class names. > Those might change at any point. Done. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:88: <li><input type='checkbox'/><label id="allow-whitelist-cb"></label><span>Allow some non-intrusive advertising</span></li> On 2015/01/22 17:56:30, Thomas Greiner wrote: > Use " instead of ' for consistency Done. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:88: <li><input type='checkbox'/><label id="allow-whitelist-cb"></label><span>Allow some non-intrusive advertising</span></li> On 2015/01/22 17:56:30, Thomas Greiner wrote: > Actually, the <span> should be the label for the checkbox. That's at least what > <label> is intended for. Yes for now, It make sense to change with span, beforehand were using for labeled control. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:100: <div style="display: flex;"> On 2015/01/22 17:56:30, Thomas Greiner wrote: > All styles should be inside a CSS file to separate structure and styles. Done. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:101: <button class="addbtn" id="whitelisting-add-btn">+<span class="i18n_options_btn_add"></span></button> On 2015/01/22 17:56:30, Thomas Greiner wrote: > Why not "button-add"? That should be more consistent with the other class names > you're using. Done. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:177: <h1>1. <span class="i18n_options_faq"></span></h1> On 2015/01/22 17:56:30, Thomas Greiner wrote: > No need to do manual enumeration. Just use <ol> or CSS counters (see > https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Counters). Done. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:181: <span class="icon icon-arrow"></span> On 2015/01/22 17:56:30, Thomas Greiner wrote: > This can be added using the "::before" pseudo element to ensure consistency for > all similar links. Done. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:206: <div> On 2015/01/22 17:56:30, Thomas Greiner wrote: > We use two spaces for indentation. Done. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:207: <div class="header"> On 2015/01/22 17:56:30, Thomas Greiner wrote: > Use the <header> element instead to stick to standard markup. Done. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:210: <span id="modal-title"></span> On 2015/01/22 17:56:30, Thomas Greiner wrote: > This structure is quite confusing and looks somewhat redundant. Can't you trim > it down? Thanks, should be better now. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:213: <span class="separator"></span> On 2015/01/22 17:56:30, Thomas Greiner wrote: > I guess this can be achieved using a border. Done. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:221: <div class="content" id="modal-content"> On 2015/01/22 17:56:30, Thomas Greiner wrote: > What about the dialog for "abp:subscribe" links? I know that we had a separate > mockup for that one to prevent editing of the filterlist title and URL. I thought it should be same dialog. Can you please provide link to that mockup ? http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:222: <div style="display: none;" id="blocking-languages-modal" data-title="options_modal_language_title"> On 2015/01/22 17:56:30, Thomas Greiner wrote: > What do you need the "data-title" attribute here for? I'm using that attribute to change the title of modal window in options.js line 85: E("modal-title").innerHTML = ext.i18n.getMessage(content.dataset.title); http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... File options.js (right): http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.js:2: * This file is part of Adblock Plus <http://adblockplus.org/>, On 2015/01/22 17:56:30, Thomas Greiner wrote: > Nit: Change to "https" Done. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/skin... File skin/options.css (right): http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/skin... skin/options.css:1: body On 2015/01/22 17:56:30, Thomas Greiner wrote: > License header is missing. Done.
Again, only comments regarding HTML and locale files. I'll look at the rest next. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/loca... File locale/ar/options.json (right): http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/loca... locale/ar/options.json:1: { On 2015/01/23 18:18:39, saroyanm wrote: > On 2015/01/22 17:56:30, Thomas Greiner wrote: > > Is this file really necessary? > > No, it's not necessary. It just will shows Error when testing RTL direction with > "ar" locale, but still testable. > Removed FYI: As soon as we have arabic translations of the texts in the options page this error should no longer appear. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... File options.html (right): http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:24: <script type="text/javascript" src="ext/common.js"></script> On 2015/01/23 18:18:39, saroyanm wrote: > On 2015/01/22 17:56:30, Thomas Greiner wrote: > > No need to specify the <script> type > > Done. > We are using type attribute also in other HTML files, should we also remove them > for consistency ? No, we usually take care of inconsistencies when someone's working at a specific file in a review. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:88: <li><input type='checkbox'/><label id="allow-whitelist-cb"></label><span>Allow some non-intrusive advertising</span></li> On 2015/01/23 18:18:39, saroyanm wrote: > On 2015/01/22 17:56:30, Thomas Greiner wrote: > > Actually, the <span> should be the label for the checkbox. That's at least > what > > <label> is intended for. > > Yes for now, It make sense to change with span, beforehand were using for > labeled control. That's not what I meant. I thought more of something along the lines of: <li> <label id="allow-whitelist"> <input type="checkbox"> Allow some non-intrusive advertising </label> </li> - <input> will be hidden anyway - <label> used as markup for <input> - icon can be added using "::before" CSS pseudo element (this technique can also be used for other icons on that page like you did with the link icons for the help tab) http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:221: <div class="content" id="modal-content"> On 2015/01/23 18:18:39, saroyanm wrote: > On 2015/01/22 17:56:30, Thomas Greiner wrote: > > What about the dialog for "abp:subscribe" links? I know that we had a separate > > mockup for that one to prevent editing of the filterlist title and URL. > > I thought it should be same dialog. > Can you please provide link to that mockup ? I asked Sven to attach it to the issue description. http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti... options.html:222: <div style="display: none;" id="blocking-languages-modal" data-title="options_modal_language_title"> On 2015/01/23 18:18:39, saroyanm wrote: > On 2015/01/22 17:56:30, Thomas Greiner wrote: > > What do you need the "data-title" attribute here for? > > I'm using that attribute to change the title of modal window in options.js line > 85: > E("modal-title").innerHTML = ext.i18n.getMessage(content.dataset.title); You could achieve the same effect using a CSS class on the div#modal element which would allow you to define via CSS rules which elements/texts are visible and which ones aren't. That class can be set by the JavaScript to show a specific variant of the dialog. http://codereview.adblockplus.org/6088024630755328/diff/5129251808346112/opti... File options.html (right): http://codereview.adblockplus.org/6088024630755328/diff/5129251808346112/opti... options.html:74: <p class="option-name"><span class="i18n_options_language_title"></span> <a class="i18n_options_readMore highlighted" href="#" target="_blank"></a></p> Is "highlighted" the right name here? What about "tooltip" since links with dotted lines underneath are meant for showing a tooltip when hovering over them? http://codereview.adblockplus.org/6088024630755328/diff/5129251808346112/opti... options.html:74: <p class="option-name"><span class="i18n_options_language_title"></span> <a class="i18n_options_readMore highlighted" href="#" target="_blank"></a></p> Again, no need to put every element in a single line. This also applies to other code parts on this page. http://codereview.adblockplus.org/6088024630755328/diff/5129251808346112/opti... options.html:76: <hr> As noted in other comments please remove any style-related HTML tags and instead use CSS styles. http://codereview.adblockplus.org/6088024630755328/diff/5129251808346112/opti... options.html:182: <button> Why do you need a <div> around the button? You didn't need one in the previous revision. http://codereview.adblockplus.org/6088024630755328/diff/5129251808346112/opti... options.html:231: <input type="text" placeholder="find language" id="find-language" /> The placeholder text has to be translatable. http://codereview.adblockplus.org/6088024630755328/diff/5129251808346112/opti... options.html:241: <input id="blockingList-textbox" type="text" placeholder="www.yourURL.com/blockinglist.txt" /> Personal preference: Usually, we use www.example.com for example URLs so I'd prefer to use it here as well instead of www.yourURL.com http://codereview.adblockplus.org/6088024630755328/diff/5129251808346112/opti... options.html:245: <span class="i18n_options_modal_blocklist_import text-blue"></span> Here's another class that contains style information in its name.
Updated http://codereview.adblockplus.org/6088024630755328/diff/5129251808346112/opti... File options.html (right): http://codereview.adblockplus.org/6088024630755328/diff/5129251808346112/opti... options.html:74: <p class="option-name"><span class="i18n_options_language_title"></span> <a class="i18n_options_readMore highlighted" href="#" target="_blank"></a></p> On 2015/01/26 18:29:17, Thomas Greiner wrote: > Is "highlighted" the right name here? What about "tooltip" since links with > dotted lines underneath are meant for showing a tooltip when hovering over them? Done. http://codereview.adblockplus.org/6088024630755328/diff/5129251808346112/opti... options.html:74: <p class="option-name"><span class="i18n_options_language_title"></span> <a class="i18n_options_readMore highlighted" href="#" target="_blank"></a></p> On 2015/01/26 18:29:17, Thomas Greiner wrote: > Again, no need to put every element in a single line. This also applies to other > code parts on this page. Done. http://codereview.adblockplus.org/6088024630755328/diff/5129251808346112/opti... options.html:76: <hr> On 2015/01/26 18:29:17, Thomas Greiner wrote: > As noted in other comments please remove any style-related HTML tags and instead > use CSS styles. Done. http://codereview.adblockplus.org/6088024630755328/diff/5129251808346112/opti... options.html:182: <button> On 2015/01/26 18:29:17, Thomas Greiner wrote: > Why do you need a <div> around the button? You didn't need one in the previous > revision. Divs with class name "controls" are wrappers for table control elements like: Add item, edit and etc. http://codereview.adblockplus.org/6088024630755328/diff/5129251808346112/opti... options.html:231: <input type="text" placeholder="find language" id="find-language" /> On 2015/01/26 18:29:17, Thomas Greiner wrote: > The placeholder text has to be translatable. Done. http://codereview.adblockplus.org/6088024630755328/diff/5129251808346112/opti... options.html:241: <input id="blockingList-textbox" type="text" placeholder="www.yourURL.com/blockinglist.txt" /> On 2015/01/26 18:29:17, Thomas Greiner wrote: > Personal preference: Usually, we use http://www.example.com for example URLs so I'd > prefer to use it here as well instead of http://www.yourURL.com Done. http://codereview.adblockplus.org/6088024630755328/diff/5129251808346112/opti... options.html:245: <span class="i18n_options_modal_blocklist_import text-blue"></span> On 2015/01/26 18:29:17, Thomas Greiner wrote: > Here's another class that contains style information in its name. Done.
Just updated options.js after separating the elements, neither than having on same line.
@Thomas I've uploaded new patch according our discussion: 1. Use Pseudo element for checkboxes (don't overlay using "span"). 2. Use parent's class to show and hide content.
Here's my review of options.js http://codereview.adblockplus.org/6088024630755328/diff/5129251808346112/opti... File options.html (right): http://codereview.adblockplus.org/6088024630755328/diff/5129251808346112/opti... options.html:182: <button> On 2015/01/27 11:19:11, saroyanm wrote: > On 2015/01/26 18:29:17, Thomas Greiner wrote: > > Why do you need a <div> around the button? You didn't need one in the previous > > revision. > > Divs with class name "controls" are wrappers for table control elements like: > Add item, edit and etc. I see, fine then. http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... File options.html (right): http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... options.html:247: <input type="text" id="find-language" /> This could be `type="search"`. See https://developer.mozilla.org/en/docs/Web/HTML/Element/Input for documentation of its features (e.g. "search" event) http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... File options.js (right): http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... options.js:23: var acceptableAdsUrl = null; Don't save that URL as a separate variable or you'll have a race condition if you access this variable before calling `getAcceptableAdsURL()`. Therefore always call `getAcceptableAdsURL()` to ensure that the variable exists. Same for `optionSubscriptions` but for that the `loadRecommendations()` function needs to be changed to cache the result like `getAcceptableAdsURL()` does. http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... options.js:33: setLinks("block-element-explanation", "#"); No need to set the `src` attribute of anchor tags if we don't intend to use them as hyperlinks. http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... options.js:35: E("add-blocking-list").addEventListener("click", Modal.open, false); I noticed you have a ton of event listeners. Instead of assigning an event listener to each element individually, you could react to clicks on a higher level in the DOM hierarchy and check which element it comes from to determine which action to take. http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... options.js:41: E("whitelisting-textbox").addEventListener("keypress", function(e) { Nit: Opening braces should be in next line. http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... options.js:41: E("whitelisting-textbox").addEventListener("keypress", function(e) { Here you're using `e` as the name for an Event whereas further down you've chosen `ev`. Please be consistent and use `ev`. http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... options.js:42: if (e.keyCode == 13) Please avoid using magical numbers. Unfortunately, the current state of keyboard events is suboptimal (see https://developer.mozilla.org/en-US/docs/Web/Events/keypress) so you need to (1) make sure that it works on different operating systems and (2) add a comment explaining which key this key code is supposed to correspond to. http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... options.js:45: E("whitelisting-cancel-btn").addEventListener("click", function(){ Nit: Opening braces should be in next line. http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... options.js:49: E("import-blockingList-btn").addEventListener("click", importListBtnCLick, false); Calling this function `importList` should be more than sufficient. No need to reference anything about the UI. http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... options.js:50: E("edit-ownBlockingList-btn").addEventListener("click", editOwnRulsBtnClick, false); Calling this function `editCustomFilters` should be more than sufficient. No need to reference anything about the UI. Please also stick to common terminology in the code which might differ from how we call something in the UI (e.g. blocking lists -> subscription, own rules -> custom filters, etc.). http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... options.js:80: open: function (content) This function is being called inconsistently. Sometimes you call it with `Modal.open(String)` and sometimes with `Modal.open(Event)` by defining it as an event listener. I assume that's why you're checking for `this` a few lines below. http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... options.js:89: modal.style.marginTop = -(modal.clientHeight/2)+"px"; Calculating layout is the job of the browser so stick with CSS here. Flexbox makes this much easier than it used to be (example: http://philipwalton.github.io/solved-by-flexbox/demos/vertical-centering/). http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... options.js:145: loadRecommendations(function(recommends) You're calling it "recommends" here but "recommendations" below. Please make the variable name consistent. http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... options.js:165: recommends[subscriptions[i].url] = subscriptions[i]; Please keep both lists separate since they contain two different kinds of subscriptions data. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... File options.html (right): http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.html:144: <h1><span class="i18n_options_tweaks"></span><a class="i18n_options_readMore tooltip" href="#"></a></h1> Don't put everything in one line. Not only here but also in the rest of the file. Otherwise it is easy to lose track of the page structure. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... File options.js (right): http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:60: E(tab.dataset.show).classList.add("active"); Using CSS classes instead of modifying the style attribute is already an improvement but here's an example of what I meant: When clicking on the "General" tab you could set the "data-tab" attribute of the body element to "general" to show the content of the general tab. You can achieve that using CSS to hide elements that should not be shown in the general tab, show those that should be shown and highlight the tab in the sidebar. e.g. body[data-tab="general"] #tab-general { ... } body[data-tab="general"] #content-general { ... } Using that technique you could simply write `document.body.dataset.tab = "general";` instead of the entire `initTabs()` function. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:85: document.body.classList.add("modal-active"); You could get rid of this entire function by setting the "data-modal" attribute on the body by using the same technique I outlined above for `initTabs()`. e.g. To show the "add-subscription" dialog: `document.body.dataset.modal = "add-subscription"` To hide the dialog: `delete document.body.dataset.modal` http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:182: request.open("GET", "subscriptions.xml"); This function call is missing the third parameter which indicates whether it should be a synchronous or asynchronous XHR. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:190: var subscription = {}; Use `var subscription = Object.create(null);` instead since this object doesn't need a prototype. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:199: subscription.display = ext.i18n.getMessage("options_language_"+subscription.prefixes.replace(/,/g, '_')); This line doesn't need to be that long. I'd suggest splitting it up into two lines: prefix = prefixes.replace(/,/g, "_"); subscription.display = ext.i18n.getMessage("subscriptions_langauge_" + prefix); http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:204: var popular = element.getAttribute("popular"); It's safer to convert this string into a boolean. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:223: var language = item.getElementsByTagName("span")[1].innerHTML; Don't rely on the content of an element because it might change (e.g. if we decide to add child nodes). Rather use an approach similar to this one: http://www.redotheweb.com/2013/05/15/client-side-full-text-search-in-css.html http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:233: var display = subscription.display ? subscription.display : subscription.title; You can reduce this down to `var display = subscriptions.display || subscription.title;` http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:234: var getPossition = function(elements, subscription) This variable name is spelled incorrectly. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:234: var getPossition = function(elements, subscription) You were able to use a much nicer approach for populating the list of whitelisted domains by using the data structure to sort the elements instead of going through the DOM. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:277: var possition = getPossition(elements, elem); This variable name is spelled incorrectly. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:322: for (var i = 0; i < elems.length; i++) Why are you removing elements when adding a subscription? The list of language subscriptions in the dialog can be populated everytime you load it. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:337: function addFurtherList(subscription) We tend to call them "custom subscriptions" in our code. "Further blocking lists" is used only in the texts. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:357: for (var i = 0; i < elem.length; i++) A subscription should only be shown once on the page, right? http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:361: checkbox.checked = state; Might make sense to implement some simple data-binding for that to make sure that model and view are always kept in sync. Unfortunately, `Object.observe()` is not widely supported yet so we'd have to implement that ourselves. I'd like to hear Wladimir's thoughts on that though. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:449: function showAddSubscriptionDialog(action, subscription) The `action` parameter is unused. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:483: var acceptableCheckbox = this; This variable is not necessary. You can access the element on which the event was dispatched on using the event's `target` property. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:487: var title = "Allow non-intrusive advertising"; Those two values are only used once so no need to define variables for them. if (!ev.target.checked) removeSubscription(url); else addSubscription(url, "Allow non-intrusive advertising"); http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:539: if (match[1]) `match` could be null which can cause an exception to be thrown here. Therefore `if (match)` would be a safer approach. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:546: domains.push(items[i].innerHTML); The content of an HTML element might change so better not to rely on that and take the data directly from an array. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:565: if (match[1]) As mentioned above, `match` could be null so better check `if (match)`. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:567: var elem = document.querySelector("[data-domain='"+match[1]+"']"); We're not restricted by bandwidth so you don't need to use abbreviated variable names. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:567: var elem = document.querySelector("[data-domain='"+match[1]+"']"); Nit: Missing spaces around `+`s http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:625: method: "app.get", Nit: Adjust indentation level http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:629: E("abp-version").innerHTML = addonVersion; You're only changing the text content so use `textContent` instead of `innerHTML`. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:642: function setLinks(id) Seems like you should be able to get rid of this function after addressing my comment regarding line 33. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:679: message.args.unshift(message.action); You don't even use `message.action` in `showAddSubscriptionDialog` because you're already checking for it here so no need to add it to `message.args`. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:680: showAddSubscriptionDialog.apply(null, message.args); `showAddSubscription` only requires the first argument so let's limit it to that: `showAddSubscriptionDialog(message.args[0]);` http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:685: onFilterMessage.apply(null, message.args); `onFilterMessage` only expects two parameters so let's limit it to that: `onFilterMessage(message.action, message.args[0]);` Same applies to `onSubscriptionMessage` below.
http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/skin... File skin/options.css (right): http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/skin... skin/options.css:242: bottom: 0px; Nit: You omitted unit for left/right but not for top/bottom. This is inconsistent. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/skin... skin/options.css:333: padding-left: 38px; This should be rigth for right-to-left languages. So use -webkit-padding-start and -moz-padding-start. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/skin... skin/options.css:344: padding-right: 10px; This should be left for right-to-left languages. So use -webkit-padding-end and -moz-padding-end. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/skin... skin/options.css:590: padding-left: 10px; This should be rigth for right-to-left languages. So use -webkit-padding-start and -moz-padding-start. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/skin... skin/options.css:607: right: 10px; This should be left for right-to-left languages. So use -webkit-padding-end and -moz-padding-end. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/skin... skin/options.css:613: border-right: 1px solid #CDCDCD; This should be left for right-to-left languages. So use -webkit-borde-end and -moz-border-end. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/skin... skin/options.css:679: float: right; The whole point of using flexbox was to not use float, which won't behave correctly for right-to-left languages. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/skin... skin/options.css:701: padding-left: 0px !important; This should be rigth for right-to-left languages. So use -webkit-padding-start and -moz-padding-start. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/skin... skin/options.css:712: margin-left:-200px; This hack is obosolete when using flexbox. In order to center and element just use "margin: auto", and make the parent element use "display: flex". http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/skin... skin/options.css:792: margin-right: 16px !important; This should be left for right-to-left languages. So use -webkit-margin-end and -moz-margin-end.
Thanks for your comments guys, Sorry for late update, but it took little bit more time than expected. @Thomas Can you please review options.js again, the implementation is changed hope this implementation is better than the one before. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... File options.js (right): http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:60: E(tab.dataset.show).classList.add("active"); On 2015/01/30 13:55:52, Thomas Greiner wrote: > Using CSS classes instead of modifying the style attribute is already an > improvement but here's an example of what I meant: > > When clicking on the "General" tab you could set the "data-tab" attribute of the > body element to "general" to show the content of the general tab. You can > achieve that using CSS to hide elements that should not be shown in the general > tab, show those that should be shown and highlight the tab in the sidebar. > > e.g. > > body[data-tab="general"] #tab-general { > ... > } > > body[data-tab="general"] #content-general { > ... > } > > Using that technique you could simply write `document.body.dataset.tab = > "general";` instead of the entire `initTabs()` function. Yes, good point. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:85: document.body.classList.add("modal-active"); On 2015/01/30 13:55:52, Thomas Greiner wrote: > You could get rid of this entire function by setting the "data-modal" attribute > on the body by using the same technique I outlined above for `initTabs()`. > > e.g. > > To show the "add-subscription" dialog: `document.body.dataset.modal = > "add-subscription"` > To hide the dialog: `delete document.body.dataset.modal` Done. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:182: request.open("GET", "subscriptions.xml"); On 2015/01/30 13:55:52, Thomas Greiner wrote: > This function call is missing the third parameter which indicates whether it > should be a synchronous or asynchronous XHR. Done. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:190: var subscription = {}; On 2015/01/30 13:55:52, Thomas Greiner wrote: > Use `var subscription = Object.create(null);` instead since this object doesn't > need a prototype. Done. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:199: subscription.display = ext.i18n.getMessage("options_language_"+subscription.prefixes.replace(/,/g, '_')); On 2015/01/30 13:55:52, Thomas Greiner wrote: > This line doesn't need to be that long. I'd suggest splitting it up into two > lines: > > prefix = prefixes.replace(/,/g, "_"); > subscription.display = ext.i18n.getMessage("subscriptions_langauge_" + prefix); Done. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:204: var popular = element.getAttribute("popular"); On 2015/01/30 13:55:52, Thomas Greiner wrote: > It's safer to convert this string into a boolean. Done. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:234: var getPossition = function(elements, subscription) On 2015/01/30 13:55:52, Thomas Greiner wrote: > You were able to use a much nicer approach for populating the list of > whitelisted domains by using the data structure to sort the elements instead of > going through the DOM. Changed to use arrays for sorting. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:322: for (var i = 0; i < elems.length; i++) On 2015/01/30 13:55:52, Thomas Greiner wrote: > Why are you removing elements when adding a subscription? The list of language > subscriptions in the dialog can be populated everytime you load it. The item will stay there when it's added while we don't need that item in all languages list anymore not to confuse the user. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:337: function addFurtherList(subscription) On 2015/01/30 13:55:52, Thomas Greiner wrote: > We tend to call them "custom subscriptions" in our code. "Further blocking > lists" is used only in the texts. Done. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:357: for (var i = 0; i < elem.length; i++) On 2015/01/30 13:55:52, Thomas Greiner wrote: > A subscription should only be shown once on the page, right? if you mean tab, then Language subscriptions shown in modal also. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:449: function showAddSubscriptionDialog(action, subscription) On 2015/01/30 13:55:52, Thomas Greiner wrote: > The `action` parameter is unused. Done. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:539: if (match[1]) On 2015/01/30 13:55:52, Thomas Greiner wrote: > `match` could be null which can cause an exception to be thrown here. Therefore > `if (match)` would be a safer approach. Done. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:546: domains.push(items[i].innerHTML); On 2015/01/30 13:55:52, Thomas Greiner wrote: > The content of an HTML element might change so better not to rely on that and > take the data directly from an array. Done. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:565: if (match[1]) On 2015/01/30 13:55:52, Thomas Greiner wrote: > As mentioned above, `match` could be null so better check `if (match)`. Done. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:625: method: "app.get", On 2015/01/30 13:55:52, Thomas Greiner wrote: > Nit: Adjust indentation level Done. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:629: E("abp-version").innerHTML = addonVersion; On 2015/01/30 13:55:52, Thomas Greiner wrote: > You're only changing the text content so use `textContent` instead of > `innerHTML`. Done. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:679: message.args.unshift(message.action); On 2015/01/30 13:55:52, Thomas Greiner wrote: > You don't even use `message.action` in `showAddSubscriptionDialog` because > you're already checking for it here so no need to add it to `message.args`. Done. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:680: showAddSubscriptionDialog.apply(null, message.args); On 2015/01/30 13:55:52, Thomas Greiner wrote: > `showAddSubscription` only requires the first argument so let's limit it to > that: > > `showAddSubscriptionDialog(message.args[0]);` Done. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/opti... options.js:685: onFilterMessage.apply(null, message.args); On 2015/01/30 13:55:52, Thomas Greiner wrote: > `onFilterMessage` only expects two parameters so let's limit it to that: > > `onFilterMessage(message.action, message.args[0]);` > > Same applies to `onSubscriptionMessage` below. Done. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/skin... File skin/options.css (right): http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/skin... skin/options.css:242: bottom: 0px; On 2015/02/11 07:44:07, Sebastian Noack wrote: > Nit: You omitted unit for left/right but not for top/bottom. This is > inconsistent. Done. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/skin... skin/options.css:333: padding-left: 38px; On 2015/02/11 07:44:07, Sebastian Noack wrote: > This should be rigth for right-to-left languages. So use -webkit-padding-start > and -moz-padding-start. Done. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/skin... skin/options.css:344: padding-right: 10px; On 2015/02/11 07:44:07, Sebastian Noack wrote: > This should be left for right-to-left languages. So use -webkit-padding-end and > -moz-padding-end. Done. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/skin... skin/options.css:590: padding-left: 10px; On 2015/02/11 07:44:07, Sebastian Noack wrote: > This should be rigth for right-to-left languages. So use -webkit-padding-start > and -moz-padding-start. Done. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/skin... skin/options.css:607: right: 10px; On 2015/02/11 07:44:07, Sebastian Noack wrote: > This should be left for right-to-left languages. So use -webkit-padding-end and > -moz-padding-end. Done. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/skin... skin/options.css:613: border-right: 1px solid #CDCDCD; On 2015/02/11 07:44:07, Sebastian Noack wrote: > This should be left for right-to-left languages. So use -webkit-borde-end and > -moz-border-end. Done. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/skin... skin/options.css:679: float: right; On 2015/02/11 07:44:07, Sebastian Noack wrote: > The whole point of using flexbox was to not use float, which won't behave > correctly for right-to-left languages. Done. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/skin... skin/options.css:701: padding-left: 0px !important; On 2015/02/11 07:44:07, Sebastian Noack wrote: > This should be rigth for right-to-left languages. So use -webkit-padding-start > and -moz-padding-start. Done. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/skin... skin/options.css:712: margin-left:-200px; On 2015/02/11 07:44:07, Sebastian Noack wrote: > This hack is obosolete when using flexbox. In order to center and element just > use "margin: auto", and make the parent element use "display: flex". Done. http://codereview.adblockplus.org/6088024630755328/diff/5113375461736448/skin... skin/options.css:792: margin-right: 16px !important; On 2015/02/11 07:44:07, Sebastian Noack wrote: > This should be left for right-to-left languages. So use -webkit-margin-end and > -moz-margin-end. Done.
LGTM for the CSS. I didn't really look into the other code, leaving it up to Thomas to review it. But please consider the filter validation there. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... File options.js (right): http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:496: ext.backgroundPage.sendMessage( Note that custom filters are validated. So filters.add might respond with a validation error that must be shown to the user.
http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... File options.js (right): http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:496: ext.backgroundPage.sendMessage( On 2015/02/13 15:12:51, Sebastian Noack wrote: > Note that custom filters are validated. So filters.add might respond with a > validation error that must be shown to the user. Good point. I think that can be handled in separate review.
Mostly consistency issues but there were quite a bunch of those in there which is why there are so many comments. Please also be careful with the naming of variables and functions and with code style. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/loca... File locale/en-US/options.json (right): http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/loca... locale/en-US/options.json:3: "description": "Content of <title> tag", This is an implementation detail and it doesn't mention that it's for the options page (same issue with other descriptions in this file). My suggestion: "Options page title" http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/loca... locale/en-US/options.json:7: "description": "Shown in navigation sidebar", This is not describing what the text is, only where it is shown. Same issue with other descriptions in this file. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/loca... locale/en-US/options.json:71: "message": "Allow some non-intrusive advertising" Please add a description for this one. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/loca... locale/en-US/options.json:194: "description": "Grey placeholder text in add language modal dialog", The style of an element is an implementation detail and should therefore not appear in a description for a text. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... File options.html (right): http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.html:24: <style id="search_style"></style> All other ID values in this file use "-" as word separator so I'd suggest replacing "_" with "-". http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.html:24: <style id="search_style"></style> Add `type="text/css"` for consistency. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... File options.js (right): http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:22: var acceptableAdsUrl = null; See http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:24: var optionObj = This variable name is too generic and therefore doesn't express what its value is. Furthermore there's no need to introduce this namespace. If you want to group functions together, usually, a simple comment is enough for that. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:26: collections: Collections of what? Again, this name is too generic. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:28: push: function() push/remove do not fit together since it's push/pop or add/remove. Therefore I'd sugget using addItems/removeItem. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:41: for (var i=0; i < arguments.length; i++) Nit: Missing spaces around "=" character (also in other for loops) http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:53: var list = document.createElement("li"); It's not a list, it's a list item. Again, be careful with the naming of variables since this could lead to confusion as soon as others are operating on this code. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:56: list.getElementsByClassName("display")[0].textContent = text; `list.querySelector(".display")` seems more appropriate for this use case. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:58: list.dataset.value = text.toLowerCase(); The name `value` is too generic and therefore doesn't describe what this values is meant for. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:88: getArray: function(subscription) This code is only used once so no need to make it its own function. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:105: var optionSubscription = this[subscription.url]; It's not clear to me why it is called `optionSubscription`. I'd recommend calling this variable `knownSubscription` to describe that it is a subscription we might already know. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:135: chboxListener: function(e) There's no reason to abbreviate this name. I'd suggest `toggleSubscription` since this is what it does, checkbox is an implementation detail. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:135: chboxListener: function(e) It's better to define those event listeners where they're used. Otherwise it's going to be difficult to maintain this code. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:138: var target = e.target; This variable is not necessary, especially since you're accessing `e.target` two lines below anyway. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:146: acceptableAdsListener: function(e) It's more helpful to describe what it does, e.g. `toggleWhitelist` http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:192: observe: function(obj, props) This functionality is only used at one part in the code so not necessary to make it its own function. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:192: observe: function(obj, props) I would've prefered a polyfill for `Object.observe()` (see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...) because this is just a workaround since it's not too widespread yet. e.g. if (!Object.observe) { Object.observe = ... } http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:196: obj["__"+property] = obj[property]; The convention is to use "$" as prefix for dynamically created properties. "_" is usually used for "private" properties and "__" looks too much like "_" and will lead to maintenance issues. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:210: var elem = elements[i]; There's no need to shorten this variable name. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:212: if (control.tagName == "INPUT") Use `localName` instead of `tagName`. Note that the values are lower-case. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:226: function createCollArray(tables) Seems like what you'd want here is a "class-like" construct instead of the factory pattern. You also wouldn't need to assign functions like you currently do in line 112. e.g. function Collection() { ... } Collection.prototype = { addItem: ..., removeItem: ..., ... }; collections.popular = new Collection(); See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Inheritance_and_the_p... for more information about prototype inheritance. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:239: template: "<input type='checkbox' class='control' /><span class='display'></span><span class='popular'>popular</span>", Don't put HTML code into JavaScript. This can easily be avoided using HTML templates (see http://www.html5rocks.com/en/tutorials/webcomponents/template/) http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:239: template: "<input type='checkbox' class='control' /><span class='display'></span><span class='popular'>popular</span>", Don't hardcode text or otherwise it can't be translated. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:285: load: function() A namespace with only one function? What's wrong with having a `loadRecommendations()` function? http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:305: var prefix = element.getAttribute("prefixes").replace(/,/g, '_'); You really need to keep an eye on style consistency. We don't use `'` but `"` for strings. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:315: }.bind(this); What do you need to bind `this` for? http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:316: request.send(); Always set the first parameter. In this case `null` would be sufficient. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:322: updateShareLink(); This also needs to be called everytime filters change (subscription disabled, subscription added, filter removed, etc.) http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:326: for (var i = 0; i < tabList.length; ++i) Unless you need it at some point I'd stick with consistency here and write `i++` instead of `++i`. Note that in some situations it does make a difference whether you use one or the other. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:328: tabList[i].addEventListener("click", function(ev) Here the event variable is called `ev` and in other parts of the code it's called `e`. That is inconsistent. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:330: document.body.dataset.tab = this.id.substr(4); I thought you added `data-show` for determining what should be hidden so I'd suggest making use of it. Anyway, don't misuse the ID attribute or otherwise it could easily break. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:330: document.body.dataset.tab = this.id.substr(4); Usually, we use `ev.target` to reference the event dispatching element so I'd recommend using it instead. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:334: var searchLanguage = function() `function searchLanguage()` please to keep it consistent. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:336: var searchStyle = document.getElementById('search_style'); You're using `document.getElementById()` here but `E()` a few lines below. Again, watch out for inconsistencies. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:337: var searchVal = this.value; This variable is not really effective since a few lines below you're using `this.value` again, anyway. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:348: }, function(addonVersion) The indentation here is inconsistent. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:353: var whitelistDomainBtnClick = function() The name is implementation-specific. Rather describe what the function does. e.g. `function addWhitelistedDomain()` http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:355: var domain = E("whitelisting-textbox").value; Here it would make sense to have a variable for `E("whitelisting-textbox")` http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:384: if (e.keyCode == 13) See http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:403: E("modal-title").textContent = ext.i18n.getMessage(title); All titles can already be in the HTML and made in-/visible using CSS. This way you wouldn't need to rely on the correct message ID format. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:406: function populateLists() I didn't find any part in your code where you're emptying the lists before populating them. This could lead to inconsistencies between UI representation and stored data. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:456: var SendMessage = This is not the name of a "class-like" function (e.g. `new Example()`) and therefore should start with a lower-case letter. But I don't see why you'd even need to distinguish between functions that send a message to the background page and functions that don't. I don't mind namespaces but in this case this one's of no use. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:512: var MessageListeners = Again, this variable name should start with a lower-case letter. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:548: optionObj.subscriptions.remove(subscription); Note that this would also remove popular subscriptions. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:555: showAddSubscriptionDialog: function(subscription) This function is not listening for messages so if you want to have a "messageListeners" namespace then this one doesn't belong in it. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:565: { In this line you're putting "{" in a separate line whereas in line 571 you don't. (See https://hg.adblockplus.org/adblockpluschrome/file/72785e0948d0/block.js#l42 for a similar example) http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/skin... File skin/options.css (right): http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/skin... skin/options.css:356: width: 395px; This wasn't necessary in the previous version so why is it now? http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/skin... skin/options.css:363: -moz-padding-end: 10px; Seems like this will be WebKit only since there doesn't appear to be a Gecko equivalent. Either remove custom styles for scrollbars (after consulting with Sven) or remove `-moz-padding-end` from here. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/skin... skin/options.css:722: -webkit-padding-start: 0px !important; Why is it necessary to use `!important` here?
Thomas I've replied on the part of comments more is coming. I've also tried to describe the idea behind the namespaces I've used, would really appreciate if you can find time to reply on that, some of the comments which are not yet replied are also rely on whether we should stick with the namespace solution or not. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... File options.js (right): http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:24: var optionObj = On 2015/02/18 17:19:27, Thomas Greiner wrote: > This variable name is too generic and therefore doesn't express what its value > is. Furthermore there's no need to introduce this namespace. If you want to > group functions together, usually, a simple comment is enough for that. I thought it would be handy to use this namespace to manage the view, so this namespace consists of current objects: * Collections * Subscriptions * Filters Collections contains arrays that are bind to view with it's methods, so after pushing item to array it's being sorted and modified in the view as well when item is being removed. Collections array consists of Subscriptions (Which I called optionSubscription) or Filters (optionFilters). Arrays in Collections also have property named "table" which contains information about the IDs, templates and listeners which is used by array to update the view. Subscriptions are subscriptionUrl to "optionSubscription" map with methods for managing the subscriptions with current methods: * update: Gets the "optionSubscription" from map and update it or adding the optionSubscription to appropriate Collection and starts to observ. * remove: Manages the removal of optionSubscription from appropriate Collection. * and subscriptions related listeners Filters are Filtet.text to "optionFilter" map, with current methods: * update: Currently it's checking the type of the filter whether it's whitelist type filter and push to appropriate collection in this case "collections.whitelist". * remove: Currently just remove from whitelist collection. * and filter related listeners So I used this namespace to combine this 3 objects that are responsible for view. Please let me know if you still think this usage of namespace here are redundant. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:41: for (var i=0; i < arguments.length; i++) On 2015/02/18 17:19:27, Thomas Greiner wrote: > Nit: Missing spaces around "=" character (also in other for loops) Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:53: var list = document.createElement("li"); On 2015/02/18 17:19:27, Thomas Greiner wrote: > It's not a list, it's a list item. Again, be careful with the naming of > variables since this could lead to confusion as soon as others are operating on > this code. Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:56: list.getElementsByClassName("display")[0].textContent = text; On 2015/02/18 17:19:27, Thomas Greiner wrote: > `list.querySelector(".display")` seems more appropriate for this use case. Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:58: list.dataset.value = text.toLowerCase(); On 2015/02/18 17:19:27, Thomas Greiner wrote: > The name `value` is too generic and therefore doesn't describe what this values > is meant for. Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:88: getArray: function(subscription) On 2015/02/18 17:19:27, Thomas Greiner wrote: > This code is only used once so no need to make it its own function. Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:138: var target = e.target; On 2015/02/18 17:19:27, Thomas Greiner wrote: > This variable is not necessary, especially since you're accessing `e.target` two > lines below anyway. Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:146: acceptableAdsListener: function(e) On 2015/02/18 17:19:27, Thomas Greiner wrote: > It's more helpful to describe what it does, e.g. `toggleWhitelist` I'd go with toggleAcceptableAds, while we have whitelisted websites (domains) separately, for less confusion between those two. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:196: obj["__"+property] = obj[property]; On 2015/02/18 17:19:27, Thomas Greiner wrote: > The convention is to use "$" as prefix for dynamically created properties. "_" > is usually used for "private" properties and "__" looks too much like "_" and > will lead to maintenance issues. Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:210: var elem = elements[i]; On 2015/02/18 17:19:27, Thomas Greiner wrote: > There's no need to shorten this variable name. Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:212: if (control.tagName == "INPUT") On 2015/02/18 17:19:27, Thomas Greiner wrote: > Use `localName` instead of `tagName`. Note that the values are lower-case. Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:226: function createCollArray(tables) On 2015/02/18 17:19:27, Thomas Greiner wrote: > Seems like what you'd want here is a "class-like" construct instead of the > factory pattern. You also wouldn't need to assign functions like you currently > do in line 112. > > e.g. > function Collection() > { > ... > } > Collection.prototype = { > addItem: ..., > removeItem: ..., > ... > }; > > collections.popular = new Collection(); > > See > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Inheritance_and_the_p... > for more information about prototype inheritance. Actually what I wanted here is Array which is binded to view, so if I'll use collection.push(subscription) or collection.push(filter) it would sort the array and update the view and same way if I'll call collection.remove(subscription) or collection.remove(filter) it would also update the view, so the array will be binded to view. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:285: load: function() On 2015/02/18 17:19:27, Thomas Greiner wrote: > A namespace with only one function? What's wrong with having a > `loadRecommendations()` function? Yes, make sense, had several more functions beforehand. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:305: var prefix = element.getAttribute("prefixes").replace(/,/g, '_'); On 2015/02/18 17:19:27, Thomas Greiner wrote: > You really need to keep an eye on style consistency. We don't use `'` but `"` > for strings. Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:315: }.bind(this); On 2015/02/18 17:19:27, Thomas Greiner wrote: > What do you need to bind `this` for? Good point, didn't use here. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:316: request.send(); On 2015/02/18 17:19:27, Thomas Greiner wrote: > Always set the first parameter. In this case `null` would be sufficient. Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:326: for (var i = 0; i < tabList.length; ++i) On 2015/02/18 17:19:27, Thomas Greiner wrote: > Unless you need it at some point I'd stick with consistency here and write `i++` > instead of `++i`. Note that in some situations it does make a difference whether > you use one or the other. Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:328: tabList[i].addEventListener("click", function(ev) On 2015/02/18 17:19:27, Thomas Greiner wrote: > Here the event variable is called `ev` and in other parts of the code it's > called `e`. That is inconsistent. Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:330: document.body.dataset.tab = this.id.substr(4); On 2015/02/18 17:19:27, Thomas Greiner wrote: > Usually, we use `ev.target` to reference the event dispatching element so I'd > recommend using it instead. Yes good point if we bind some other object will go with currentTarget. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:330: document.body.dataset.tab = this.id.substr(4); On 2015/02/18 17:19:27, Thomas Greiner wrote: > I thought you added `data-show` for determining what should be hidden so I'd > suggest making use of it. Anyway, don't misuse the ID attribute or otherwise it > could easily break. Good point. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:336: var searchStyle = document.getElementById('search_style'); On 2015/02/18 17:19:27, Thomas Greiner wrote: > You're using `document.getElementById()` here but `E()` a few lines below. > Again, watch out for inconsistencies. Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:337: var searchVal = this.value; On 2015/02/18 17:19:27, Thomas Greiner wrote: > This variable is not really effective since a few lines below you're using > `this.value` again, anyway. Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:348: }, function(addonVersion) On 2015/02/18 17:19:27, Thomas Greiner wrote: > The indentation here is inconsistent. Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:355: var domain = E("whitelisting-textbox").value; On 2015/02/18 17:19:27, Thomas Greiner wrote: > Here it would make sense to have a variable for `E("whitelisting-textbox")` Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:456: var SendMessage = On 2015/02/18 17:19:27, Thomas Greiner wrote: > This is not the name of a "class-like" function (e.g. `new Example()`) and > therefore should start with a lower-case letter. But I don't see why you'd even > need to distinguish between functions that send a message to the background page > and functions that don't. > > I don't mind namespaces but in this case this one's of no use. from my point of view addSubscription, removeSubscriptio and etc. It's not clear whether this methods add subscription to the views or to the extension in general, I was thinking to have namespace that clearly says that methods in that namespace will send message to BG page using mesage passing. So it was really handy for me use methods in namespace, but if this leads to more confusion I can don't use the namespace for this purpose. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:512: var MessageListeners = On 2015/02/18 17:19:27, Thomas Greiner wrote: > Again, this variable name should start with a lower-case letter. Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:548: optionObj.subscriptions.remove(subscription); On 2015/02/18 17:19:27, Thomas Greiner wrote: > Note that this would also remove popular subscriptions. No it's not I'm using optionObj.subscriptions object to map to "optionSubscription" after that I can see whether it's popular subscription or not, this is handled in subscription.remove http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:555: showAddSubscriptionDialog: function(subscription) On 2015/02/18 17:19:27, Thomas Greiner wrote: > This function is not listening for messages so if you want to have a > "messageListeners" namespace then this one doesn't belong in it. Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:565: { On 2015/02/18 17:19:27, Thomas Greiner wrote: > In this line you're putting "{" in a separate line whereas in line 571 you > don't. (See > https://hg.adblockplus.org/adblockpluschrome/file/72785e0948d0/block.js#l42 for > a similar example) Done.
Comments on the latest patch. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... File options.js (right): http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:24: var optionObj = On 2015/02/19 16:58:53, saroyanm wrote: > On 2015/02/18 17:19:27, Thomas Greiner wrote: > > This variable name is too generic and therefore doesn't express what its value > > is. Furthermore there's no need to introduce this namespace. If you want to > > group functions together, usually, a simple comment is enough for that. > > I thought it would be handy to use this namespace to manage the view, so this > namespace consists of current objects: > * Collections > * Subscriptions > * Filters > > Collections contains arrays that are bind to view with it's methods, so after > pushing item to array it's being sorted and modified in the view as well when > item is being removed. > Collections array consists of Subscriptions (Which I called optionSubscription) > or Filters (optionFilters). > Arrays in Collections also have property named "table" which contains > information about the IDs, templates and listeners which is used by array to > update the view. > > > Subscriptions are subscriptionUrl to "optionSubscription" map with methods for > managing the subscriptions with current methods: > * update: Gets the "optionSubscription" from map and update it or adding the > optionSubscription to appropriate Collection and starts to observ. > * remove: Manages the removal of optionSubscription from appropriate Collection. > * and subscriptions related listeners > > Filters are Filtet.text to "optionFilter" map, with current methods: > * update: Currently it's checking the type of the filter whether it's whitelist > type filter and push to appropriate collection in this case > "collections.whitelist". > * remove: Currently just remove from whitelist collection. > * and filter related listeners > > So I used this namespace to combine this 3 objects that are responsible for > view. > Please let me know if you still think this usage of namespace here are > redundant. I do think it is based on the suggestions I made in my other comments for this patch. Those would make it possible to have all three of those be simple global objects that only contain data. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:146: acceptableAdsListener: function(e) On 2015/02/19 16:58:53, saroyanm wrote: > On 2015/02/18 17:19:27, Thomas Greiner wrote: > > It's more helpful to describe what it does, e.g. `toggleWhitelist` > > I'd go with toggleAcceptableAds, while we have whitelisted websites (domains) > separately, for less confusion between those two. That's fine with me. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:226: function createCollArray(tables) On 2015/02/19 16:58:53, saroyanm wrote: > On 2015/02/18 17:19:27, Thomas Greiner wrote: > > Seems like what you'd want here is a "class-like" construct instead of the > > factory pattern. You also wouldn't need to assign functions like you currently > > do in line 112. > > > > e.g. > > function Collection() > > { > > ... > > } > > Collection.prototype = { > > addItem: ..., > > removeItem: ..., > > ... > > }; > > > > collections.popular = new Collection(); > > > > See > > > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Inheritance_and_the_p... > > for more information about prototype inheritance. > > Actually what I wanted here is Array which is binded to view, so if I'll use > collection.push(subscription) or collection.push(filter) it would sort the array > and update the view and same way if I'll call collection.remove(subscription) or > collection.remove(filter) it would also update the view, so the array will be > binded to view. I know and it will have the same result but instead of having to assign functions from other objects you could simply inherit and override those functions to create custom object instances instead of modified arrays. Please take a look at the link that I included in my previous comment to learn more about it. Optionally, feel free to schedule a meeting and I'll explain a bit more in-depth how prototypal inheritance works and how it can be used. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:330: document.body.dataset.tab = this.id.substr(4); On 2015/02/19 16:58:53, saroyanm wrote: > On 2015/02/18 17:19:27, Thomas Greiner wrote: > > Usually, we use `ev.target` to reference the event dispatching element so I'd > > recommend using it instead. > > Yes good point if we bind some other object will go with currentTarget. That's better, I agree http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:456: var SendMessage = On 2015/02/19 16:58:53, saroyanm wrote: > On 2015/02/18 17:19:27, Thomas Greiner wrote: > > This is not the name of a "class-like" function (e.g. `new Example()`) and > > therefore should start with a lower-case letter. But I don't see why you'd > even > > need to distinguish between functions that send a message to the background > page > > and functions that don't. > > > > I don't mind namespaces but in this case this one's of no use. > > from my point of view addSubscription, removeSubscriptio and etc. It's not clear > whether this methods add subscription to the views or to the extension in > general, I was thinking to have namespace that clearly says that methods in that > namespace will send message to BG page using mesage passing. So it was really > handy for me use methods in namespace, but if this leads to more confusion I can > don't use the namespace for this purpose. There should only be one function for adding a subscription, one for removing a subscription, etc. There should only be one point where subscriptions are added/removed from the view and that's inside the filter listener. That mechanism ensures consistency because a subscription will only be added to/removed from the view as a consequence of sending a message to the background page. Note that you're currently only using `optionObj.subscriptions.remove` in one part of your code so it doesn't need to be a separate function. Same situation with `optionObj.filters.remove`. So it appears that the whole view-namespace could be reduced to an `updateSubscription(subscription)` and an `updateFilter(filter)` function. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:548: optionObj.subscriptions.remove(subscription); On 2015/02/19 16:58:53, saroyanm wrote: > On 2015/02/18 17:19:27, Thomas Greiner wrote: > > Note that this would also remove popular subscriptions. > > No it's not I'm using optionObj.subscriptions object to map to > "optionSubscription" after that I can see whether it's popular subscription or > not, this is handled in subscription.remove Ok, in that case inlining the `optionObj.subscriptions.remove` function would clarify that. Because as mentioned in my comment at http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... it's only used here anyway. http://codereview.adblockplus.org/6088024630755328/diff/6217103673655296/opti... File options.js (right): http://codereview.adblockplus.org/6088024630755328/diff/6217103673655296/opti... options.js:329: var searchStyle = E('search_style'); Review comments should not only be addressed for one specific part of the code but if you know that an issue occurs in other places as well, please also fix them there. Otherwise we'd end up with many duplicate comments about the same issue. In this case you have addressed the comment about replacing `"` instead of `'` from http://codereview.adblockplus.org/6088024630755328/diff2/5742506566221824:621... as expected but didn't address the same issue elsewhere in the code, like in this line. http://codereview.adblockplus.org/6088024630755328/diff/6217103673655296/opti... options.js:337: ext.backgroundPage.sendMessage( It's still inconsistent. As I noted in a previous comment of mine, the "{" is in its own line whereas the "}" is not. Since we don't have one style we always go with, feel free to choose one of the following: fn({foo: "bar"}, function(foo) { return null; }); or fn({ foo: "bar" }, function(foo) { return null; }); or (less common in our code base) fn( { foo: "bar" }, function(foo) { return null; } );
Thomas thanks for detailed review. Hope this patch will not s*ck as previous :) http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... File options.html (right): http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... options.html:247: <input type="text" id="find-language" /> On 2015/01/30 13:55:51, Thomas Greiner wrote: > This could be `type="search"`. See > https://developer.mozilla.org/en/docs/Web/HTML/Element/Input for documentation > of its features (e.g. "search" event) Done. http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... File options.js (right): http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... options.js:23: var acceptableAdsUrl = null; On 2015/01/30 13:55:51, Thomas Greiner wrote: > Don't save that URL as a separate variable or you'll have a race condition if > you access this variable before calling `getAcceptableAdsURL()`. Therefore > always call `getAcceptableAdsURL()` to ensure that the variable exists. > > Same for `optionSubscriptions` but for that the `loadRecommendations()` function > needs to be changed to cache the result like `getAcceptableAdsURL()` does. Done, but without caching Recommendation, now I'm populating it for scratch for current implementation consistency. http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... options.js:33: setLinks("block-element-explanation", "#"); On 2015/01/30 13:55:51, Thomas Greiner wrote: > No need to set the `src` attribute of anchor tags if we don't intend to use them > as hyperlinks. Not using setLinks in currently uploaded patch. http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... options.js:35: E("add-blocking-list").addEventListener("click", Modal.open, false); On 2015/01/30 13:55:51, Thomas Greiner wrote: > I noticed you have a ton of event listeners. Instead of assigning an event > listener to each element individually, you could react to clicks on a higher > level in the DOM hierarchy and check which element it comes from to determine > which action to take. Most of them already not exist, let me know if it's make sense to use on whitelisting event listeners. http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... options.js:41: E("whitelisting-textbox").addEventListener("keypress", function(e) { On 2015/01/30 13:55:51, Thomas Greiner wrote: > Here you're using `e` as the name for an Event whereas further down you've > chosen `ev`. Please be consistent and use `ev`. I've changed to use `e` from some point, let me know if I should change to `ev` again. http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... options.js:41: E("whitelisting-textbox").addEventListener("keypress", function(e) { On 2015/01/30 13:55:51, Thomas Greiner wrote: > Nit: Opening braces should be in next line. Done. http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... options.js:42: if (e.keyCode == 13) On 2015/01/30 13:55:51, Thomas Greiner wrote: > Please avoid using magical numbers. Unfortunately, the current state of keyboard > events is suboptimal (see > https://developer.mozilla.org/en-US/docs/Web/Events/keypress) so you need to (1) > make sure that it works on different operating systems and (2) add a comment > explaining which key this key code is supposed to correspond to. Done. http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... options.js:45: E("whitelisting-cancel-btn").addEventListener("click", function(){ On 2015/01/30 13:55:51, Thomas Greiner wrote: > Nit: Opening braces should be in next line. Done. http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... options.js:49: E("import-blockingList-btn").addEventListener("click", importListBtnCLick, false); On 2015/01/30 13:55:51, Thomas Greiner wrote: > Calling this function `importList` should be more than sufficient. No need to > reference anything about the UI. I don't have this function anymore. http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... options.js:50: E("edit-ownBlockingList-btn").addEventListener("click", editOwnRulsBtnClick, false); On 2015/01/30 13:55:51, Thomas Greiner wrote: > Calling this function `editCustomFilters` should be more than sufficient. No > need to reference anything about the UI. > > Please also stick to common terminology in the code which might differ from how > we call something in the UI (e.g. blocking lists -> subscription, own rules -> > custom filters, etc.). Done. http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... options.js:80: open: function (content) On 2015/01/30 13:55:51, Thomas Greiner wrote: > This function is being called inconsistently. Sometimes you call it with > `Modal.open(String)` and sometimes with `Modal.open(Event)` by defining it as an > event listener. > > I assume that's why you're checking for `this` a few lines below. Now we are using CSS to show the modal. http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... options.js:89: modal.style.marginTop = -(modal.clientHeight/2)+"px"; On 2015/01/30 13:55:51, Thomas Greiner wrote: > Calculating layout is the job of the browser so stick with CSS here. Flexbox > makes this much easier than it used to be (example: > http://philipwalton.github.io/solved-by-flexbox/demos/vertical-centering/). Done. http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... options.js:145: loadRecommendations(function(recommends) On 2015/01/30 13:55:51, Thomas Greiner wrote: > You're calling it "recommends" here but "recommendations" below. Please make the > variable name consistent. Done. http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... options.js:165: recommends[subscriptions[i].url] = subscriptions[i]; On 2015/01/30 13:55:51, Thomas Greiner wrote: > Please keep both lists separate since they contain two different kinds of > subscriptions data. Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/loca... File locale/en-US/options.json (right): http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/loca... locale/en-US/options.json:3: "description": "Content of <title> tag", On 2015/02/18 17:19:27, Thomas Greiner wrote: > This is an implementation detail and it doesn't mention that it's for the > options page (same issue with other descriptions in this file). > > My suggestion: "Options page title" Maybe we can have some kind of comment ? If it's not obvious from file name for Translators ? http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/loca... locale/en-US/options.json:7: "description": "Shown in navigation sidebar", On 2015/02/18 17:19:27, Thomas Greiner wrote: > This is not describing what the text is, only where it is shown. Same issue with > other descriptions in this file. Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/loca... locale/en-US/options.json:71: "message": "Allow some non-intrusive advertising" On 2015/02/18 17:19:27, Thomas Greiner wrote: > Please add a description for this one. Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/loca... locale/en-US/options.json:194: "description": "Grey placeholder text in add language modal dialog", On 2015/02/18 17:19:27, Thomas Greiner wrote: > The style of an element is an implementation detail and should therefore not > appear in a description for a text. Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... File options.html (right): http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.html:24: <style id="search_style"></style> On 2015/02/18 17:19:27, Thomas Greiner wrote: > All other ID values in this file use "-" as word separator so I'd suggest > replacing "_" with "-". Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.html:24: <style id="search_style"></style> On 2015/02/18 17:19:27, Thomas Greiner wrote: > Add `type="text/css"` for consistency. Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... File options.js (right): http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:22: var acceptableAdsUrl = null; On 2015/02/18 17:19:27, Thomas Greiner wrote: > See > http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:24: var optionObj = On 2015/02/20 14:02:07, Thomas Greiner wrote: > On 2015/02/19 16:58:53, saroyanm wrote: > > On 2015/02/18 17:19:27, Thomas Greiner wrote: > > > This variable name is too generic and therefore doesn't express what its > value > > > is. Furthermore there's no need to introduce this namespace. If you want to > > > group functions together, usually, a simple comment is enough for that. > > > > I thought it would be handy to use this namespace to manage the view, so this > > namespace consists of current objects: > > * Collections > > * Subscriptions > > * Filters > > > > Collections contains arrays that are bind to view with it's methods, so after > > pushing item to array it's being sorted and modified in the view as well when > > item is being removed. > > Collections array consists of Subscriptions (Which I called > optionSubscription) > > or Filters (optionFilters). > > Arrays in Collections also have property named "table" which contains > > information about the IDs, templates and listeners which is used by array to > > update the view. > > > > > > Subscriptions are subscriptionUrl to "optionSubscription" map with methods for > > managing the subscriptions with current methods: > > * update: Gets the "optionSubscription" from map and update it or adding the > > optionSubscription to appropriate Collection and starts to observ. > > * remove: Manages the removal of optionSubscription from appropriate > Collection. > > * and subscriptions related listeners > > > > Filters are Filtet.text to "optionFilter" map, with current methods: > > * update: Currently it's checking the type of the filter whether it's > whitelist > > type filter and push to appropriate collection in this case > > "collections.whitelist". > > * remove: Currently just remove from whitelist collection. > > * and filter related listeners > > > > So I used this namespace to combine this 3 objects that are responsible for > > view. > > Please let me know if you still think this usage of namespace here are > > redundant. > > I do think it is based on the suggestions I made in my other comments for this > patch. Those would make it possible to have all three of those be simple global > objects that only contain data. Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:26: collections: On 2015/02/18 17:19:27, Thomas Greiner wrote: > Collections of what? Again, this name is too generic. Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:28: push: function() On 2015/02/18 17:19:27, Thomas Greiner wrote: > push/remove do not fit together since it's push/pop or add/remove. Therefore I'd > sugget using addItems/removeItem. Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:105: var optionSubscription = this[subscription.url]; On 2015/02/18 17:19:27, Thomas Greiner wrote: > It's not clear to me why it is called `optionSubscription`. I'd recommend > calling this variable `knownSubscription` to describe that it is a subscription > we might already know. Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:135: chboxListener: function(e) On 2015/02/18 17:19:27, Thomas Greiner wrote: > There's no reason to abbreviate this name. I'd suggest `toggleSubscription` > since this is what it does, checkbox is an implementation detail. Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:135: chboxListener: function(e) On 2015/02/18 17:19:27, Thomas Greiner wrote: > It's better to define those event listeners where they're used. Otherwise it's > going to be difficult to maintain this code. Hope current location will be better. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:192: observe: function(obj, props) On 2015/02/18 17:19:27, Thomas Greiner wrote: > This functionality is only used at one part in the code so not necessary to make > it its own function. Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:192: observe: function(obj, props) On 2015/02/18 17:19:27, Thomas Greiner wrote: > I would've prefered a polyfill for `Object.observe()` (see > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...) > because this is just a workaround since it's not too widespread yet. > > e.g. > if (!Object.observe) > { > Object.observe = ... > } Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:226: function createCollArray(tables) On 2015/02/20 14:02:07, Thomas Greiner wrote: > On 2015/02/19 16:58:53, saroyanm wrote: > > On 2015/02/18 17:19:27, Thomas Greiner wrote: > > > Seems like what you'd want here is a "class-like" construct instead of the > > > factory pattern. You also wouldn't need to assign functions like you > currently > > > do in line 112. > > > > > > e.g. > > > function Collection() > > > { > > > ... > > > } > > > Collection.prototype = { > > > addItem: ..., > > > removeItem: ..., > > > ... > > > }; > > > > > > collections.popular = new Collection(); > > > > > > See > > > > > > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Inheritance_and_the_p... > > > for more information about prototype inheritance. > > > > Actually what I wanted here is Array which is binded to view, so if I'll use > > collection.push(subscription) or collection.push(filter) it would sort the > array > > and update the view and same way if I'll call collection.remove(subscription) > or > > collection.remove(filter) it would also update the view, so the array will be > > binded to view. > > I know and it will have the same result but instead of having to assign > functions from other objects you could simply inherit and override those > functions to create custom object instances instead of modified arrays. > > Please take a look at the link that I included in my previous comment to learn > more about it. Optionally, feel free to schedule a meeting and I'll explain a > bit more in-depth how prototypal inheritance works and how it can be used. Yes that make sense, my previous comment was unnecessary. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:239: template: "<input type='checkbox' class='control' /><span class='display'></span><span class='popular'>popular</span>", On 2015/02/18 17:19:27, Thomas Greiner wrote: > Don't put HTML code into JavaScript. This can easily be avoided using HTML > templates (see http://www.html5rocks.com/en/tutorials/webcomponents/template/) Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:239: template: "<input type='checkbox' class='control' /><span class='display'></span><span class='popular'>popular</span>", On 2015/02/18 17:19:27, Thomas Greiner wrote: > Don't hardcode text or otherwise it can't be translated. Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:322: updateShareLink(); On 2015/02/18 17:19:27, Thomas Greiner wrote: > This also needs to be called everytime filters change (subscription disabled, > subscription added, filter removed, etc.) Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:334: var searchLanguage = function() On 2015/02/18 17:19:27, Thomas Greiner wrote: > `function searchLanguage()` please to keep it consistent. Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:353: var whitelistDomainBtnClick = function() On 2015/02/18 17:19:27, Thomas Greiner wrote: > The name is implementation-specific. Rather describe what the function does. > > e.g. > `function addWhitelistedDomain()` Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:384: if (e.keyCode == 13) On 2015/02/18 17:19:27, Thomas Greiner wrote: > See > http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... Yes I've updated it to first check for e.key As you can see from here: https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent.keyCode#Non-pr... On Windows, Linux and Mac keycode 13 coressponds to "Enter", so we can go until it's removed. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:403: E("modal-title").textContent = ext.i18n.getMessage(title); On 2015/02/18 17:19:27, Thomas Greiner wrote: > All titles can already be in the HTML and made in-/visible using CSS. This way > you wouldn't need to rely on the correct message ID format. Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:406: function populateLists() On 2015/02/18 17:19:27, Thomas Greiner wrote: > I didn't find any part in your code where you're emptying the lists before > populating them. This could lead to inconsistencies between UI representation > and stored data. Thomas now I'm removing before populating them, but can you please describe, why we call this function also when filters are loaded, but not only when the DOM is loaded ? http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:456: var SendMessage = On 2015/02/20 14:02:07, Thomas Greiner wrote: > On 2015/02/19 16:58:53, saroyanm wrote: > > On 2015/02/18 17:19:27, Thomas Greiner wrote: > > > This is not the name of a "class-like" function (e.g. `new Example()`) and > > > therefore should start with a lower-case letter. But I don't see why you'd > > even > > > need to distinguish between functions that send a message to the background > > page > > > and functions that don't. > > > > > > I don't mind namespaces but in this case this one's of no use. > > > > from my point of view addSubscription, removeSubscriptio and etc. It's not > clear > > whether this methods add subscription to the views or to the extension in > > general, I was thinking to have namespace that clearly says that methods in > that > > namespace will send message to BG page using mesage passing. So it was really > > handy for me use methods in namespace, but if this leads to more confusion I > can > > don't use the namespace for this purpose. > > There should only be one function for adding a subscription, one for removing a > subscription, etc. There should only be one point where subscriptions are > added/removed from the view and that's inside the filter listener. > > That mechanism ensures consistency because a subscription will only be added > to/removed from the view as a consequence of sending a message to the background > page. > > Note that you're currently only using `optionObj.subscriptions.remove` in one > part of your code so it doesn't need to be a separate function. Same situation > with `optionObj.filters.remove`. > > So it appears that the whole view-namespace could be reduced to an > `updateSubscription(subscription)` and an `updateFilter(filter)` function. Thanks, now it's clear for me. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:548: optionObj.subscriptions.remove(subscription); On 2015/02/20 14:02:07, Thomas Greiner wrote: > On 2015/02/19 16:58:53, saroyanm wrote: > > On 2015/02/18 17:19:27, Thomas Greiner wrote: > > > Note that this would also remove popular subscriptions. > > > > No it's not I'm using optionObj.subscriptions object to map to > > "optionSubscription" after that I can see whether it's popular subscription or > > not, this is handled in subscription.remove > > Ok, in that case inlining the `optionObj.subscriptions.remove` function would > clarify that. Because as mentioned in my comment at > http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... > it's only used here anyway. Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/skin... File skin/options.css (right): http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/skin... skin/options.css:356: width: 395px; On 2015/02/18 17:19:27, Thomas Greiner wrote: > This wasn't necessary in the previous version so why is it now? Yes, in previous patch we have overflow: auto; style on table itself here we have it on container div, 5px is the minus size of righthand scroller, now in this patch we have two tables under that container: Recommedations and custm subscrpitions. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/skin... skin/options.css:363: -moz-padding-end: 10px; On 2015/02/18 17:19:27, Thomas Greiner wrote: > Seems like this will be WebKit only since there doesn't appear to be a Gecko > equivalent. Either remove custom styles for scrollbars (after consulting with > Sven) or remove `-moz-padding-end` from here. Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/skin... skin/options.css:722: -webkit-padding-start: 0px !important; On 2015/02/18 17:19:27, Thomas Greiner wrote: > Why is it necessary to use `!important` here? No need, thanks. http://codereview.adblockplus.org/6088024630755328/diff/6217103673655296/opti... File options.js (right): http://codereview.adblockplus.org/6088024630755328/diff/6217103673655296/opti... options.js:329: var searchStyle = E('search_style'); On 2015/02/20 14:02:07, Thomas Greiner wrote: > Review comments should not only be addressed for one specific part of the code > but if you know that an issue occurs in other places as well, please also fix > them there. Otherwise we'd end up with many duplicate comments about the same > issue. > > In this case you have addressed the comment about replacing `"` instead of `'` > from > http://codereview.adblockplus.org/6088024630755328/diff2/5742506566221824:621... > as expected but didn't address the same issue elsewhere in the code, like in > this line. Done. http://codereview.adblockplus.org/6088024630755328/diff/6217103673655296/opti... options.js:337: ext.backgroundPage.sendMessage( On 2015/02/20 14:02:07, Thomas Greiner wrote: > It's still inconsistent. As I noted in a previous comment of mine, the "{" is in > its own line whereas the "}" is not. Since we don't have one style we always go > with, feel free to choose one of the following: > > fn({foo: "bar"}, function(foo) > { > return null; > }); > > or > > fn({ > foo: "bar" > }, function(foo) > { > return null; > }); > > or (less common in our code base) > > fn( > { > foo: "bar" > }, > function(foo) > { > return null; > } > ); Used the one from your suggestion: https://hg.adblockplus.org/adblockpluschrome/file/72785e0948d0/block.js#l42 http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... File options.js (right): http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:28: function Collection(details) We can rethink about the name of current Class in case. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/subs... File subscriptions.xml (right): http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/subs... subscriptions.xml:112: specialization="Disable Tracking" @greiner @palant: Currently I'm using specialization attribute to display the title of recommended subscription, but we need to think about the way how to localize the title of the recommended subscription, for Ad blocking subscription we are using "prefixes" attribute to localize subscription title "options_language_"+prefix. Please let me know how you think it would be better to implement the localization for the recommended subscriptions if you have an indea.
http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... File options.js (right): http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... options.js:35: E("add-blocking-list").addEventListener("click", Modal.open, false); On 2015/02/26 12:18:32, saroyanm wrote: > On 2015/01/30 13:55:51, Thomas Greiner wrote: > > I noticed you have a ton of event listeners. Instead of assigning an event > > listener to each element individually, you could react to clicks on a higher > > level in the DOM hierarchy and check which element it comes from to determine > > which action to take. > > Most of them already not exist, let me know if it's make sense to use on > whitelisting event listeners. Yes, it definitely makes sense to combine the listeners for the controls wrapper for the domain whitelisting section. http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... options.js:41: E("whitelisting-textbox").addEventListener("keypress", function(e) { On 2015/02/26 12:18:32, saroyanm wrote: > On 2015/01/30 13:55:51, Thomas Greiner wrote: > > Here you're using `e` as the name for an Event whereas further down you've > > chosen `ev`. Please be consistent and use `ev`. > > I've changed to use `e` from some point, let me know if I should change to `ev` > again. No need to as long as it's consistent. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/loca... File locale/en-US/options.json (right): http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/loca... locale/en-US/options.json:3: "description": "Content of <title> tag", On 2015/02/26 12:18:33, saroyanm wrote: > On 2015/02/18 17:19:27, Thomas Greiner wrote: > > This is an implementation detail and it doesn't mention that it's for the > > options page (same issue with other descriptions in this file). > > > > My suggestion: "Options page title" > > Maybe we can have some kind of comment ? If it's not obvious from file name for > Translators ? You're right, the file name might be sufficient already. Still makes sense to explicitly mention it where it makes sense. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... File options.js (right): http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/opti... options.js:406: function populateLists() On 2015/02/26 12:18:33, saroyanm wrote: > On 2015/02/18 17:19:27, Thomas Greiner wrote: > > I didn't find any part in your code where you're emptying the lists before > > populating them. This could lead to inconsistencies between UI representation > > and stored data. > > Thomas now I'm removing before populating them, but can you please describe, why > we call this function also when filters are loaded, but not only when the DOM is > loaded ? We already discussed that in person so I'll just add it here for reference: The "load" event is triggered whenever the filter storage is reloaded so we should invalidate (i.e. clear) all filter-related UI elements and reinitialize them to make sure that it's in-sync again. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/skin... File skin/options.css (right): http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/skin... skin/options.css:356: width: 395px; On 2015/02/26 12:18:33, saroyanm wrote: > On 2015/02/18 17:19:27, Thomas Greiner wrote: > > This wasn't necessary in the previous version so why is it now? > > Yes, in previous patch we have overflow: auto; style on table itself here we > have it on container div, 5px is the minus size of righthand scroller, now in > this patch we have two tables under that container: Recommedations and custm > subscrpitions. Usually, you don't need to account for scrollbars. So setting the width to `100%` should do the job. If you're specifying it using exact pixel values will lead to problems, at least in non-WebKit environments because there we cannot even specify the width of the scrollbars. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/loca... File locale/en-US/options.json (right): http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/loca... locale/en-US/options.json:11: "description": "Shown in navigation sidebar", Yes, but what is the purpose of the text? "Page title in navigation sidebar" would be more descriptive. Same applies to "options_version". http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/loca... locale/en-US/options.json:15: "description": "Tab navigation name in sidebar", Duplicate descriptions are not describing a specific text. So rather use "Name in sidebar for General tab" for those. This comment applies to a couple of duplicated descriptions in this file. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/loca... locale/en-US/options.json:43: "description": "Section name in General tab", Name of which section in the general tab? http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/loca... locale/en-US/options.json:59: "description": "Popular label in recommendation table", From a user's perspective there is no "recommendations table". All they see is "further blocking options". http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... File options.html (right): http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.html:283: <span>+add</span> This string is hardcoded and therefore can't be translated. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... File options.js (right): http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:28: function Collection(details) On 2015/02/26 12:18:33, saroyanm wrote: > We can rethink about the name of current Class in case. I guess "Collection" should be fine (see Java's Collection interface http://docs.oracle.com/javase/7/docs/api/java/util/Collection.html). Otherwise "ItemList" would be another option we could go with in this case since its methods are called "addItems" and "removeItem". http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:28: function Collection(details) Why "details"? It's an array of items and not a configuration object or something similar. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:34: Collection.prototype.addItems = function() I couldn't find a case where you add multiple items at once so accepting one parameter and renaming it to "addItem" would be sufficient. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:59: var text = object.title || object.text; A subscription might not have a title. In those cases we should at least show its URL. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:60: var access = object.url || object.text; You're only using this variable once so no need to have a variable in the first place. Same goes for "index". http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:64: listItem.dataset.access = access; You need to normalize this string or otherwise `Element.querySelector("[data-access='" + access + "']")` will break for filters or subscription titles that contain `'`. Replacing `'` with `\'` should be sufficient but it needs to be done everywhere you're constructing this access variable. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:72: control.addEventListener("click", listener, false); Just FYI: I'd still prefer a single global event listener instead of adding a listener to each control element individually but let's go with that for now. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:78: popular.textContent = ext.i18n.getMessage("options_popular"); Setting that text right inside the template tag once at startup should be sufficient. No need to set it each time a new item is added to the list. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:89: Collection.prototype.removeItem = function(obj) The function is called "removeItem" but the item is called "obj" and not "item". http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:108: var isChecked = e.target.checked; Again, this variable is only used once. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:133: listener: toggleSubscription I'd rename "listener" to "onClick". Otherwise it's not clear when that function will be triggered. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:192: var control = element.getElementsByClassName("control")[0]; Usually, you use `element.querySelector(".control")` for that. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:216: subscription["$"+property] = subscription[property]; By now I'd expect you to know that operators, like `+` in this case, need to have spaces around them. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:369: E("find-language").addEventListener("search", searchLanguage, false); Turns out that the "onsearch" event listener is WebKit-only but seems like you don't need it anyway so please remove this line. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:375: // keyCode value of 13 corresponds to "Enter" key Please also explain why you need to check for both "key" and "keyCode". Example: // KeyboardEvent.keyCode has been deprecated so we use KeyboardEvent.key instead if it's available if (...) { ... } // keyCode value 13 corresponds to "Enter" key else { ... } http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:408: collection.details.forEach(function(detail) This property does not need to be exposed. It'd be cleaner to have a `Collection.clearAll` method. Furthermore using a regular for-loop would work just as fine. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:485: if (acceptableAdsUrl) This if-statement is not necessary and neither is the "acceptableAdsUrl" variable. You're replacing the function as soon as you have the value and from then on only return the value directly. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:506: function getKnownSubscription(url) The next few functions are all one-liners. There's no added value in having them. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:625: var subscriptionUrl = subscription.url; This variable is not necessary. It's only used once and it doesn't abstract any complexity. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/skin... File skin/options.css (right): http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/skin... skin/options.css:43: hr Please insert an empty line above. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/skin... skin/options.css:152: border-bottom-width: 1px !important; I don't see why you'd need to specify `!important` here. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/skin... skin/options.css:302: .table.list li .display Just a small thing: Move this block one up so that :nth-child(odd) and :nth-child(even) are right next to each other since they belong together. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/skin... skin/options.css:380: .table input[type=checkbox] I'd suggest to get used to using quotation marks for attribute selectors (e.g. `type="checkbox"`) to avoid any issues when using more complicated selectors like `title^=Won't work`. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/skin... skin/options.css:389: .table input[type=checkbox]:before "before" is a pseudo element so use "::before" instead. In CSS3 a single ":" is only used for pseudo classes like ":checked" (see https://developer.mozilla.org/en-US/docs/Web/CSS/Pseudo-elements). ":before" is CSS2 syntax so no need to use it since the only reason to have it would be if we had to support IE8. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/skin... skin/options.css:391: content:""; Missing space after ":". http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/skin... skin/options.css:517: #whitelisting .controls input[type=text]:focus This looks a bit strange. What is it for? http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/skin... skin/options.css:616: .icon-enter-blue I was about to comment on that until I noticed that this was only used in the Advanced tab which is not part of this review. Could you please add comments to indicate where the styles for the individual tabs start? (similar to how you did it in options.html) That would make it easier to maintain. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/skin... skin/options.css:643: #blocking-list-own .input-btn-text Please use "button" instead of "btn" (not only here) because it's clearer and we're not limited by bandwidth like on the Web. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/skin... skin/options.css:670: -moz-margin-start: 5px !important; If "!important" was necessary here, rather try to make the selector more specific. Preferable, the use of "!important" should be kept to an absolute minimum. Please also check other instances where you use "!important". http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/skin... skin/options.css:836: #modal .content > div > div Using class names should make it more obvious what you're targeting here. Same for the block below. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/subs... File subscriptions.xml (right): http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/subs... subscriptions.xml:125: popular="true"/> I thought you wanted to get rid of this attribute since every subscription that can be found in this file and that doesn't have prefixes should be marked as popular.
Thanks for the comments Thomas, I've uploaded the patch, hope we are closer now. http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... File options.js (right): http://codereview.adblockplus.org/6088024630755328/diff/6613180188983296/opti... options.js:35: E("add-blocking-list").addEventListener("click", Modal.open, false); On 2015/03/05 11:36:03, Thomas Greiner wrote: > On 2015/02/26 12:18:32, saroyanm wrote: > > On 2015/01/30 13:55:51, Thomas Greiner wrote: > > > I noticed you have a ton of event listeners. Instead of assigning an event > > > listener to each element individually, you could react to clicks on a higher > > > level in the DOM hierarchy and check which element it comes from to > determine > > > which action to take. > > > > Most of them already not exist, let me know if it's make sense to use on > > whitelisting event listeners. > > Yes, it definitely makes sense to combine the listeners for the controls wrapper > for the domain whitelisting section. Done. http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/skin... File skin/options.css (right): http://codereview.adblockplus.org/6088024630755328/diff/5742506566221824/skin... skin/options.css:356: width: 395px; On 2015/03/05 11:36:03, Thomas Greiner wrote: > On 2015/02/26 12:18:33, saroyanm wrote: > > On 2015/02/18 17:19:27, Thomas Greiner wrote: > > > This wasn't necessary in the previous version so why is it now? > > > > Yes, in previous patch we have overflow: auto; style on table itself here we > > have it on container div, 5px is the minus size of righthand scroller, now in > > this patch we have two tables under that container: Recommedations and custm > > subscrpitions. > > Usually, you don't need to account for scrollbars. So setting the width to > `100%` should do the job. > > If you're specifying it using exact pixel values will lead to problems, at least > in non-WebKit environments because there we cannot even specify the width of the > scrollbars. Good point, done. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/loca... File locale/en-US/options.json (right): http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/loca... locale/en-US/options.json:11: "description": "Shown in navigation sidebar", On 2015/03/05 11:36:03, Thomas Greiner wrote: > Yes, but what is the purpose of the text? "Page title in navigation sidebar" > would be more descriptive. Same applies to "options_version". Done. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/loca... locale/en-US/options.json:15: "description": "Tab navigation name in sidebar", On 2015/03/05 11:36:03, Thomas Greiner wrote: > Duplicate descriptions are not describing a specific text. So rather use "Name > in sidebar for General tab" for those. > > This comment applies to a couple of duplicated descriptions in this file. Done. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/loca... locale/en-US/options.json:43: "description": "Section name in General tab", On 2015/03/05 11:36:03, Thomas Greiner wrote: > Name of which section in the general tab? Better ? Hopefully done :) http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/loca... locale/en-US/options.json:59: "description": "Popular label in recommendation table", On 2015/03/05 11:36:03, Thomas Greiner wrote: > From a user's perspective there is no "recommendations table". All they see is > "further blocking options". Done. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... File options.html (right): http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.html:283: <span>+add</span> On 2015/03/05 11:36:03, Thomas Greiner wrote: > This string is hardcoded and therefore can't be translated. Done. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... File options.js (right): http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:28: function Collection(details) On 2015/03/05 11:36:03, Thomas Greiner wrote: > Why "details"? It's an array of items and not a configuration object or > something similar. I used details as I thought that it's corresponds to table details (tableId and item's onclick listener). Maybe naming it tablePreferences would be better ? I think naming them items can make confusion, while we are adding items using addItems and RemoveItem, what you think ? http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:34: Collection.prototype.addItems = function() On 2015/03/05 11:36:03, Thomas Greiner wrote: > I couldn't find a case where you add multiple items at once so accepting one > parameter and renaming it to "addItem" would be sufficient. I actually were thinking that this could be useful for upcoming patch when we will implement lazy loading, anyway I can change this to only add one item for now, let me know what you think about that. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:59: var text = object.title || object.text; On 2015/03/05 11:36:03, Thomas Greiner wrote: > A subscription might not have a title. In those cases we should at least show > its URL. Done. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:60: var access = object.url || object.text; On 2015/03/05 11:36:03, Thomas Greiner wrote: > You're only using this variable once so no need to have a variable in the first > place. Same goes for "index". Done, but now I'm using access, to shorten the line. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:64: listItem.dataset.access = access; On 2015/03/05 11:36:03, Thomas Greiner wrote: > You need to normalize this string or otherwise > `Element.querySelector("[data-access='" + access + "']")` will break for filters > or subscription titles that contain `'`. > > Replacing `'` with `\'` should be sufficient but it needs to be done everywhere > you're constructing this access variable. while current query throw error: document.querySelector("[data-search='test\'.com']"); This works fine: document.querySelector("[data-search='test\\'.com']"); But not sure if we can stick to that solution, what you think ? http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:78: popular.textContent = ext.i18n.getMessage("options_popular"); On 2015/03/05 11:36:03, Thomas Greiner wrote: > Setting that text right inside the template tag once at startup should be > sufficient. No need to set it each time a new item is added to the list. Done. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:89: Collection.prototype.removeItem = function(obj) On 2015/03/05 11:36:03, Thomas Greiner wrote: > The function is called "removeItem" but the item is called "obj" and not "item". Done. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:108: var isChecked = e.target.checked; On 2015/03/05 11:36:03, Thomas Greiner wrote: > Again, this variable is only used once. Done. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:133: listener: toggleSubscription On 2015/03/05 11:36:03, Thomas Greiner wrote: > I'd rename "listener" to "onClick". Otherwise it's not clear when that function > will be triggered. Done. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:192: var control = element.getElementsByClassName("control")[0]; On 2015/03/05 11:36:03, Thomas Greiner wrote: > Usually, you use `element.querySelector(".control")` for that. Done. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:216: subscription["$"+property] = subscription[property]; On 2015/03/05 11:36:03, Thomas Greiner wrote: > By now I'd expect you to know that operators, like `+` in this case, need to > have spaces around them. Done. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:369: E("find-language").addEventListener("search", searchLanguage, false); On 2015/03/05 11:36:03, Thomas Greiner wrote: > Turns out that the "onsearch" event listener is WebKit-only but seems like you > don't need it anyway so please remove this line. I'm using this event to listen for "X" button click inside the input element of type "search": http://stackoverflow.com/questions/2977023/how-do-you-detect-the-clearing-of-... http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:375: // keyCode value of 13 corresponds to "Enter" key On 2015/03/05 11:36:03, Thomas Greiner wrote: > Please also explain why you need to check for both "key" and "keyCode". > > Example: > > // KeyboardEvent.keyCode has been deprecated so we use KeyboardEvent.key instead > if it's available > if (...) > { > ... > } > // keyCode value 13 corresponds to "Enter" key > else > { > ... > } Done. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:408: collection.details.forEach(function(detail) On 2015/03/05 11:36:03, Thomas Greiner wrote: > This property does not need to be exposed. It'd be cleaner to have a > `Collection.clearAll` method. Furthermore using a regular for-loop would work > just as fine. Done. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:485: if (acceptableAdsUrl) On 2015/03/05 11:36:03, Thomas Greiner wrote: > This if-statement is not necessary and neither is the "acceptableAdsUrl" > variable. You're replacing the function as soon as you have the value and from > then on only return the value directly. Hmm :/ Totally missed your implementation, good point :) http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:506: function getKnownSubscription(url) On 2015/03/05 11:36:03, Thomas Greiner wrote: > The next few functions are all one-liners. There's no added value in having > them. Done. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:625: var subscriptionUrl = subscription.url; On 2015/03/05 11:36:03, Thomas Greiner wrote: > This variable is not necessary. It's only used once and it doesn't abstract any > complexity. Done. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/skin... File skin/options.css (right): http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/skin... skin/options.css:43: hr On 2015/03/05 11:36:03, Thomas Greiner wrote: > Please insert an empty line above. Done. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/skin... skin/options.css:152: border-bottom-width: 1px !important; On 2015/03/05 11:36:03, Thomas Greiner wrote: > I don't see why you'd need to specify `!important` here. Because the Specificity Value of `body[data-tab="advanced"] #tab-advanced + li` was higher, anyway updated after your last comment :) http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/skin... skin/options.css:302: .table.list li .display On 2015/03/05 11:36:03, Thomas Greiner wrote: > Just a small thing: Move this block one up so that :nth-child(odd) and > :nth-child(even) are right next to each other since they belong together. Done. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/skin... skin/options.css:380: .table input[type=checkbox] On 2015/03/05 11:36:03, Thomas Greiner wrote: > I'd suggest to get used to using quotation marks for attribute selectors (e.g. > `type="checkbox"`) to avoid any issues when using more complicated selectors > like `title^=Won't work`. Done. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/skin... skin/options.css:389: .table input[type=checkbox]:before On 2015/03/05 11:36:03, Thomas Greiner wrote: > "before" is a pseudo element so use "::before" instead. In CSS3 a single ":" is > only used for pseudo classes like ":checked" (see > https://developer.mozilla.org/en-US/docs/Web/CSS/Pseudo-elements). > > ":before" is CSS2 syntax so no need to use it since the only reason to have it > would be if we had to support IE8. Done. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/skin... skin/options.css:391: content:""; On 2015/03/05 11:36:03, Thomas Greiner wrote: > Missing space after ":". Done. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/skin... skin/options.css:517: #whitelisting .controls input[type=text]:focus On 2015/03/05 11:36:03, Thomas Greiner wrote: > This looks a bit strange. What is it for? What is exactly strange ? http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/skin... skin/options.css:616: .icon-enter-blue On 2015/03/05 11:36:03, Thomas Greiner wrote: > I was about to comment on that until I noticed that this was only used in the > Advanced tab which is not part of this review. > > Could you please add comments to indicate where the styles for the individual > tabs start? (similar to how you did it in options.html) That would make it > easier to maintain. Done, but for general tab couldn't find good place to collect the styles, because most of the general styles applies to General tab content. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/skin... skin/options.css:643: #blocking-list-own .input-btn-text On 2015/03/05 11:36:03, Thomas Greiner wrote: > Please use "button" instead of "btn" (not only here) because it's clearer and > we're not limited by bandwidth like on the Web. Done. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/skin... skin/options.css:670: -moz-margin-start: 5px !important; On 2015/03/05 11:36:03, Thomas Greiner wrote: > If "!important" was necessary here, rather try to make the selector more > specific. Preferable, the use of "!important" should be kept to an absolute > minimum. > > Please also check other instances where you use "!important". Done. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/skin... skin/options.css:836: #modal .content > div > div On 2015/03/05 11:36:03, Thomas Greiner wrote: > Using class names should make it more obvious what you're targeting here. Same > for the block below. Done. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/subs... File subscriptions.xml (right): http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/subs... subscriptions.xml:125: popular="true"/> On 2015/03/05 11:36:03, Thomas Greiner wrote: > I thought you wanted to get rid of this attribute since every subscription that > can be found in this file and that doesn't have prefixes should be marked as > popular. I thought maybe afterward if we decide to use prefixes also for popular subscriptions we will need some other identificator, let me know if you are okey to keep current implementation.
Only few minor things left. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/loca... File locale/en-US/options.json (right): http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/loca... locale/en-US/options.json:43: "description": "Section name in General tab", On 2015/03/06 11:54:32, saroyanm wrote: > On 2015/03/05 11:36:03, Thomas Greiner wrote: > > Name of which section in the general tab? > > Better ? Hopefully done :) Better, thank you. :) http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... File options.js (right): http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:28: function Collection(details) On 2015/03/06 11:54:32, saroyanm wrote: > On 2015/03/05 11:36:03, Thomas Greiner wrote: > > Why "details"? It's an array of items and not a configuration object or > > something similar. > > I used details as I thought that it's corresponds to table details (tableId and > item's onclick listener). > Maybe naming it tablePreferences would be better ? > I think naming them items can make confusion, while we are adding items using > addItems and RemoveItem, what you think ? Makes sense, let's stick with "details" then. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:34: Collection.prototype.addItems = function() On 2015/03/06 11:54:32, saroyanm wrote: > On 2015/03/05 11:36:03, Thomas Greiner wrote: > > I couldn't find a case where you add multiple items at once so accepting one > > parameter and renaming it to "addItem" would be sufficient. > > I actually were thinking that this could be useful for upcoming patch when we > will implement lazy loading, anyway I can change this to only add one item for > now, let me know what you think about that. Ok, then let's leave it like it is for now (no need to add that parameter then) to keep things simple. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:60: var access = object.url || object.text; On 2015/03/06 11:54:32, saroyanm wrote: > On 2015/03/05 11:36:03, Thomas Greiner wrote: > > You're only using this variable once so no need to have a variable in the > first > > place. Same goes for "index". > > Done, but now I'm using access, to shorten the line. That'd be ok but "index" is only used in that one if-case so if you want to have a separate "index" variable to make lines shorter then initialize it where you're using it and not here. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:64: listItem.dataset.access = access; On 2015/03/06 11:54:32, saroyanm wrote: > On 2015/03/05 11:36:03, Thomas Greiner wrote: > > You need to normalize this string or otherwise > > `Element.querySelector("[data-access='" + access + "']")` will break for > filters > > or subscription titles that contain `'`. > > > > Replacing `'` with `\'` should be sufficient but it needs to be done > everywhere > > you're constructing this access variable. > > while current query throw error: > document.querySelector("[data-search='test\'.com']"); > > This works fine: > document.querySelector("[data-search='test\\'.com']"); > > But not sure if we can stick to that solution, what you think ? Should be fine. Alternatively, you could use `encodeURIComponent` to normalize it. However, for that you'd need to replace table.querySelector("[data-access='" + access + "']") with table.querySelector('[data-access="' + access + '"]') http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:369: E("find-language").addEventListener("search", searchLanguage, false); On 2015/03/06 11:54:32, saroyanm wrote: > On 2015/03/05 11:36:03, Thomas Greiner wrote: > > Turns out that the "onsearch" event listener is WebKit-only but seems like you > > don't need it anyway so please remove this line. > > I'm using this event to listen for "X" button click inside the input element of > type "search": > http://stackoverflow.com/questions/2977023/how-do-you-detect-the-clearing-of-... Since that "X" is also not in the standard I'd suggest hiding it since it also doesn't appear in any of the options page designs. Example: input[type="search"]::-webkit-search-cancel-button { display: none; } According to the standard "the difference between the Text state and the Search state is primarily stylistic" (see http://www.w3.org/TR/html5/forms.html#text-(type=text)-state-and-search-state... and also https://bugzilla.mozilla.org/show_bug.cgi?id=456229#c1). http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/skin... File skin/options.css (right): http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/skin... skin/options.css:517: #whitelisting .controls input[type=text]:focus On 2015/03/06 11:54:32, saroyanm wrote: > On 2015/03/05 11:36:03, Thomas Greiner wrote: > > This looks a bit strange. What is it for? > > What is exactly strange ? That the :focus styles of that element is the same as the default ones. If, for instance, you don't want to have the outline when focused then I'd suggest splitting this up into two blocks like this: #whitelisting .controls input[type="text"] { ... styles except outline ... } #whitelisting .controls input[type="text"]:focus { outline: 0px; } http://codereview.adblockplus.org/6088024630755328/diff/5637036128075776/loca... File locale/en-US/options.json (right): http://codereview.adblockplus.org/6088024630755328/diff/5637036128075776/loca... locale/en-US/options.json:59: "description": "Popular label in further blocking option's table", Detail: I guess you meant "options" and not "option's" http://codereview.adblockplus.org/6088024630755328/diff/5637036128075776/opti... File options.js (right): http://codereview.adblockplus.org/6088024630755328/diff/5637036128075776/opti... options.js:56: var object = arguments[i]; What you're calling "object" here is the same thing you've called "obj" in "removeItems" which you renamed to "item" now so it should also be renamed here. http://codereview.adblockplus.org/6088024630755328/diff/5637036128075776/opti... options.js:330: var buttonText = "+" + ext.i18n.getMessage("options_button_add"); I suppose the "+" should be on the other side of the text for RTL languages. To achieve that you could put the "+" in a separate HTML element.
http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... File options.js (right): http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:34: Collection.prototype.addItems = function() On 2015/03/12 11:41:56, Thomas Greiner wrote: > On 2015/03/06 11:54:32, saroyanm wrote: > > On 2015/03/05 11:36:03, Thomas Greiner wrote: > > > I couldn't find a case where you add multiple items at once so accepting one > > > parameter and renaming it to "addItem" would be sufficient. > > > > I actually were thinking that this could be useful for upcoming patch when we > > will implement lazy loading, anyway I can change this to only add one item for > > now, let me know what you think about that. > > Ok, then let's leave it like it is for now (no need to add that parameter then) > to keep things simple. Done. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:60: var access = object.url || object.text; On 2015/03/12 11:41:56, Thomas Greiner wrote: > On 2015/03/06 11:54:32, saroyanm wrote: > > On 2015/03/05 11:36:03, Thomas Greiner wrote: > > > You're only using this variable once so no need to have a variable in the > > first > > > place. Same goes for "index". > > > > Done, but now I'm using access, to shorten the line. > > That'd be ok but "index" is only used in that one if-case so if you want to > have a separate "index" variable to make lines shorter then initialize it where > you're using it and not here. This is already fixed in current patch: http://codereview.adblockplus.org/6088024630755328/diff/5637036128075776/opti... http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/opti... options.js:369: E("find-language").addEventListener("search", searchLanguage, false); On 2015/03/12 11:41:56, Thomas Greiner wrote: > On 2015/03/06 11:54:32, saroyanm wrote: > > On 2015/03/05 11:36:03, Thomas Greiner wrote: > > > Turns out that the "onsearch" event listener is WebKit-only but seems like > you > > > don't need it anyway so please remove this line. > > > > I'm using this event to listen for "X" button click inside the input element > of > > type "search": > > > http://stackoverflow.com/questions/2977023/how-do-you-detect-the-clearing-of-... > > Since that "X" is also not in the standard I'd suggest hiding it since it also > doesn't appear in any of the options page designs. > > Example: > input[type="search"]::-webkit-search-cancel-button { > display: none; > } > > According to the standard "the difference between the Text state and the Search > state is primarily stylistic" (see > http://www.w3.org/TR/html5/forms.html#text-%28type=text%29-state-and-search-s...) > and also https://bugzilla.mozilla.org/show_bug.cgi?id=456229#c1). Done. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/skin... File skin/options.css (right): http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/skin... skin/options.css:517: #whitelisting .controls input[type=text]:focus On 2015/03/12 11:41:56, Thomas Greiner wrote: > On 2015/03/06 11:54:32, saroyanm wrote: > > On 2015/03/05 11:36:03, Thomas Greiner wrote: > > > This looks a bit strange. What is it for? > > > > What is exactly strange ? > > That the :focus styles of that element is the same as the default ones. > > If, for instance, you don't want to have the outline when focused then I'd > suggest splitting this up into two blocks like this: > > #whitelisting .controls input[type="text"] > { > ... styles except outline ... > } > > #whitelisting .controls input[type="text"]:focus > { > outline: 0px; > } Ahh right, Overlooked, we don't need focus here. Thanks. http://codereview.adblockplus.org/6088024630755328/diff/5637036128075776/loca... File locale/en-US/options.json (right): http://codereview.adblockplus.org/6088024630755328/diff/5637036128075776/loca... locale/en-US/options.json:59: "description": "Popular label in further blocking option's table", On 2015/03/12 11:41:56, Thomas Greiner wrote: > Detail: I guess you meant "options" and not "option's" Done. http://codereview.adblockplus.org/6088024630755328/diff/5637036128075776/opti... File options.js (right): http://codereview.adblockplus.org/6088024630755328/diff/5637036128075776/opti... options.js:56: var object = arguments[i]; On 2015/03/12 11:41:56, Thomas Greiner wrote: > What you're calling "object" here is the same thing you've called "obj" in > "removeItems" which you renamed to "item" now so it should also be renamed here. Done. http://codereview.adblockplus.org/6088024630755328/diff/5637036128075776/opti... options.js:330: var buttonText = "+" + ext.i18n.getMessage("options_button_add"); On 2015/03/12 11:41:56, Thomas Greiner wrote: > I suppose the "+" should be on the other side of the text for RTL languages. To > achieve that you could put the "+" in a separate HTML element. Should work now.
This is now something we can work with so LGTM. Just to reiterate: Smaller design issues and other missing features/changes will be addressed in a separate review. Same applies for the implementation of the Advanced and Help tab which were intentionally not addressed in this review.
On 2015/03/12 16:15:08, Thomas Greiner wrote: > This is now something we can work with so LGTM. > > Just to reiterate: Smaller design issues and other missing features/changes will > be addressed in a separate review. Same applies for the implementation of the > Advanced and Help tab which were intentionally not addressed in this review. Just to keep in mind, we still have question regarding, subscription.xml: http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/subs...
Sorry for the long wait, finally had time to start looking into this. I haven't looked into the HTML/CSS/JS yet, hoping to get around to that on Friday. But here's already some comments for the stuff I looked at. (I've tried to not post redundant comments, not sure I managed to however.) http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/subs... File subscriptions.xml (right): http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/subs... subscriptions.xml:112: specialization="Disable Tracking" On 2015/02/26 12:18:33, saroyanm wrote: > @greiner @palant: > Currently I'm using specialization attribute to display the title of recommended > subscription, but we need to think about the way how to localize the title of > the recommended subscription, for Ad blocking subscription we are using > "prefixes" attribute to localize subscription title "options_language_"+prefix. > Please let me know how you think it would be better to implement the > localization for the recommended subscriptions if you have an indea. I suggest we create a follow-up issue about how to localise that and stick to not localising it for now. Fact is: We will need it in all our products, localising it everywhere repeatedly doesn't make sense to me. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/subs... subscriptions.xml:125: popular="true"/> On 2015/03/06 11:54:32, saroyanm wrote: > On 2015/03/05 11:36:03, Thomas Greiner wrote: > > I thought you wanted to get rid of this attribute since every subscription > that > > can be found in this file and that doesn't have prefixes should be marked as > > popular. > > I thought maybe afterward if we decide to use prefixes also for popular > subscriptions we will need some other identificator, let me know if you are okey > to keep current implementation. Doesn't the very fact that the subscriptions are recommended subscriptions make them "popular" already? I, too, thought that was the logic. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/loca... File locale/en-US/options.json (right): http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/loca... locale/en-US/options.json:6: "options_abp": { How about "options_page_header_2" for this and "options_page_header_1" instead of "options_page_name", or something along those lines? http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/loca... locale/en-US/options.json:42: "options_blocking": { How about "options_blocking_title" for consistency, and likewise with the other section titles below? http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/loca... locale/en-US/options.json:146: "options_faq_link": { Do we expect this message to differ from options_faq? Otherwise I'd just use the same string and update the description to say it's used for both. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/loca... locale/en-US/options.json:210: "options_language_en": { Related to the discussion in subscriptions.xml - I don't think this is how we should localise the adblocking languages. IMO we need to find a way to localise the specialization attribute for all subscriptions in subscriptions.xml, which would include the language-specific ones. So I'd suggest we just use the specialization texts here for now, and create a follow-up issue for localising specialization in subscriptions.xml. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/subs... File subscriptions.xml (right): http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/subs... subscriptions.xml:1: <?xml version="1.0"?> So the purpose of bundling this file here is so that we temporarily don't have to change the format of the canonical one? Because this file is already bundled by the extension, and used to select the default subscriptions, so duplicating it here is not a good idea. I'm OK with using a bundled file temporarily, but I'd like: 1. A follow-up issue for getting these changes into the real subscriptions.xml file and using that one. 2. A comment in this copy of the file here, that explains why it's bundled, pointing to that issue.
Managed to review the HTML, CSS and JS still to do. One at a time :) http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... File options.html (right): http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.html:22: <title class="i18n_options_page_title"></title> Nit: Why not use dashes here for consistency? Can be a follow-up. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.html:24: <style type="text/css" id="search-style"></style> Nit: type is optional here since we're using HTML5. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.html:32: <div id="nav-sidebar"> Since we're using HTML5, I think we should use nav, header, footer et al, rather than a horde of divs. I'm completely OK with doing this in a follow-up. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.html:38: <ul class="tabs vertical" id="main-navigation-tabs"> Tiny nit: Sometimes it's id before class, sometimes the other way around, would be easier to understand if it was more consistent. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.html:170: <ul class="table" style="width: auto;"> Nit: Can we avoid using a style attribute here, and below? http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.html:256: <div id="modal"> What's a "modal"? That's rather an adjective, as in "modal window". So how about rather calling this "dialog" or something like that?
Reviewed CSS and JS as well, that should be all for now. Note that as discussed, I've done a fairly high level review, since Thomas has already reviewed everything more thoroughly, and since we want to push the first version of this soon and improve it from there. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... File options.html (right): http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.html:22: <title class="i18n_options_page_title"></title> On 2015/06/05 21:51:26, Felix H. Dahlke wrote: > Nit: Why not use dashes here for consistency? Can be a follow-up. Nevermind actually - this way it's consistent with the existing code. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... File options.js (right): http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.js:27: function Collection(details) Does details have to be an array? From what I've seen below, you're only ever passing a one element array to it. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.js:32: Collection.prototype = Object.create(Array.prototype); From what I've seen, the only member functions used on Collection instances are addItems(), removeItem() and clearAll(). In that case I believe we should prefer composition over inheritance here, i.e. just store the items in a member variable rather than inheriting from Array. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.js:43: if (aValue < bValue) Nit: Would be shorter using localeCompare: `return aValue.localeCompare(bValue);` http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.js:69: control.checked = item.disabled == false; Nit: "Do not compare x == true or x == false. Use (x) or (!x)" https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style... There are more instances of this below. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.js:72: if (table.hasChildNodes) Shouldn't this be `table.hasChildNodes()`? http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.js:109: function toggleSubscription(e) Nit: Wouldn't insist on it, but being an event handler that does something with the event, I'd prefer a name that makes this more obvious, such as `onToggleSubscriptionClick`. Same for the other control click handlers. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.js:220: ["disabled"].forEach(function(property) This could be simplified :D http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.js:239: Object.observe(subscription, function(changes) Nit: Why not just `Object.observe(subscription, onObjectChanged);`? http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.js:286: request.onload = function() Nit: Use addEventListener()? http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.js:345: function searchLanguage() Nit: Similar to above, I'd find this easier to understand if it was named to indicate it's an event handler, e.g. `onFindLanguageKeyUp`. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.js:392: // e.keyCode has been deprecated so we use e.key instead Nit: "so we attempt to use e.key"? Since we're actually falling back to keyCode. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.js:396: addWhitelistedDomain(); Nit: Seeing how the else body is identical to the if body, why not merge the conditions? http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.js:483: Nit: Seeing how we have a bunch of TODO comments here, I guess this deserves one too? http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.js:570: case "disabled": Merge with case "added" above? http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.js:627: function E(id) Not a fan of this function, made things harder to read since I first had to look up what it does. If Thomas is OK with it I can live with it too, but then we should at least name it with a lower case E, so it doesn't look like a constructor. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/skin... File skin/options.css (right): http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/skin... skin/options.css:520: General tab content Seems like there's some stuff above that belongs below this comment, e.g. #blocking-languages.
http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... File options.js (right): http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.js:69: control.checked = item.disabled == false; On 2015/06/07 21:09:59, Felix H. Dahlke wrote: > Nit: "Do not compare x == true or x == false. Use (x) or (!x)" > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style... > > There are more instances of this below. Sorry, wrong link, should have been: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style...
http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... File options.js (right): http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.js:627: function E(id) On 2015/06/07 21:09:59, Felix H. Dahlke wrote: > Not a fan of this function, made things harder to read since I first had to look > up what it does. If Thomas is OK with it I can live with it too, but then we > should at least name it with a lower case E, so it doesn't look like a > constructor. Just realised that we have E() as well in the Firefox code. Still not a fan, but for consistency's sake I'm OK with keeping it as it is here.
Thanks Felix for the review. Mostly fixed, just have few questions wrote as reply to your comments. http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/subs... File subscriptions.xml (right): http://codereview.adblockplus.org/6088024630755328/diff/5634985281191936/subs... subscriptions.xml:112: specialization="Disable Tracking" On 2015/06/03 16:20:43, Felix H. Dahlke wrote: > On 2015/02/26 12:18:33, saroyanm wrote: > > @greiner @palant: > > Currently I'm using specialization attribute to display the title of > recommended > > subscription, but we need to think about the way how to localize the title of > > the recommended subscription, for Ad blocking subscription we are using > > "prefixes" attribute to localize subscription title > "options_language_"+prefix. > > Please let me know how you think it would be better to implement the > > localization for the recommended subscriptions if you have an indea. > > I suggest we create a follow-up issue about how to localise that and stick to > not localising it for now. Fact is: We will need it in all our products, > localising it everywhere repeatedly doesn't make sense to me. https://issues.adblockplus.org/ticket/2668 http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/loca... File locale/en-US/options.json (right): http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/loca... locale/en-US/options.json:6: "options_abp": { On 2015/06/03 16:20:43, Felix H. Dahlke wrote: > How about "options_page_header_2" for this and "options_page_header_1" instead > of "options_page_name", or something along those lines? Done. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/loca... locale/en-US/options.json:42: "options_blocking": { On 2015/06/03 16:20:43, Felix H. Dahlke wrote: > How about "options_blocking_title" for consistency, and likewise with the other > section titles below? Done. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/loca... locale/en-US/options.json:146: "options_faq_link": { On 2015/06/03 16:20:43, Felix H. Dahlke wrote: > Do we expect this message to differ from options_faq? Otherwise I'd just use the > same string and update the description to say it's used for both. Good point, Done. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/loca... locale/en-US/options.json:210: "options_language_en": { On 2015/06/03 16:20:43, Felix H. Dahlke wrote: > Related to the discussion in subscriptions.xml - I don't think this is how we > should localise the adblocking languages. IMO we need to find a way to localise > the specialization attribute for all subscriptions in subscriptions.xml, which > would include the language-specific ones. So I'd suggest we just use the > specialization texts here for now, and create a follow-up issue for localising > specialization in subscriptions.xml. https://issues.adblockplus.org/ticket/2668 http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... File options.html (right): http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.html:24: <style type="text/css" id="search-style"></style> On 2015/06/05 21:51:26, Felix H. Dahlke wrote: > Nit: type is optional here since we're using HTML5. Done. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.html:32: <div id="nav-sidebar"> On 2015/06/05 21:51:26, Felix H. Dahlke wrote: > Since we're using HTML5, I think we should use nav, header, footer et al, rather > than a horde of divs. I'm completely OK with doing this in a follow-up. https://issues.adblockplus.org/ticket/2669 http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.html:38: <ul class="tabs vertical" id="main-navigation-tabs"> On 2015/06/05 21:51:26, Felix H. Dahlke wrote: > Tiny nit: Sometimes it's id before class, sometimes the other way around, would > be easier to understand if it was more consistent. Done. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.html:170: <ul class="table" style="width: auto;"> On 2015/06/05 21:51:26, Felix H. Dahlke wrote: > Nit: Can we avoid using a style attribute here, and below? Yes, please note that this is Advanced tab code we decided to handle this and Advanced tab related issues in followup reviews (#2377), I think this code can be changed and sure there is bunch of inconsistency in Advanced tab implementation. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.html:256: <div id="modal"> On 2015/06/05 21:51:26, Felix H. Dahlke wrote: > What's a "modal"? That's rather an adjective, as in "modal window". So how about > rather calling this "dialog" or something like that? Done. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... File options.js (right): http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.js:27: function Collection(details) On 2015/06/07 21:09:59, Felix H. Dahlke wrote: > Does details have to be an array? From what I've seen below, you're only ever > passing a one element array to it. on line 139 I'm passing array from 2 elements. "Adblocking for websites in.." section and "Added languages" in dialog have have same data in there and handled same way. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.js:32: Collection.prototype = Object.create(Array.prototype); On 2015/06/07 21:09:59, Felix H. Dahlke wrote: > From what I've seen, the only member functions used on Collection instances are > addItems(), removeItem() and clearAll(). In that case I believe we should prefer > composition over inheritance here, i.e. just store the items in a member > variable rather than inheriting from Array. I would like to understand why it's more preferable to use composition in this case instead of inheritance, I personally think it's more readable, for example in the example of addItems method for me it's more intuitive that I'm sorting and pushing the items into the object itself, I don't mind to use composition instead I just would like to understand whether there is a reason I'm missing or it's personal preference. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.js:43: if (aValue < bValue) On 2015/06/07 21:09:59, Felix H. Dahlke wrote: > Nit: Would be shorter using localeCompare: `return > aValue.localeCompare(bValue);` I like this :) Done. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.js:69: control.checked = item.disabled == false; On 2015/06/07 21:15:24, Felix H. Dahlke wrote: > On 2015/06/07 21:09:59, Felix H. Dahlke wrote: > > Nit: "Do not compare x == true or x == false. Use (x) or (!x)" > > > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style... > > > > There are more instances of this below. > > Sorry, wrong link, should have been: > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style... In this case is bit complicated, while the design itself is bit inconsistent with logic, we now have subscription which are disabled and aren't added which are treated differently on view, so please note the case when we need to show the subscription in the case it's "disabled" property is null or true. for this case we are using subscription.desabled == false. Or we should go with the case of !subscription.desabled && subscription.desabled != null ? It's still bit confusing. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.js:72: if (table.hasChildNodes) On 2015/06/07 21:09:59, Felix H. Dahlke wrote: > Shouldn't this be `table.hasChildNodes()`? Done. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.js:109: function toggleSubscription(e) On 2015/06/07 21:09:59, Felix H. Dahlke wrote: > Nit: Wouldn't insist on it, but being an event handler that does something with > the event, I'd prefer a name that makes this more obvious, such as > `onToggleSubscriptionClick`. Same for the other control click handlers. Done. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.js:220: ["disabled"].forEach(function(property) On 2015/06/07 21:09:59, Felix H. Dahlke wrote: > This could be simplified :D For future we are going to not only listen to "disbled" property of subscription, but also to other properties like -> title, lastDownload, homepage and etc. I've added comment regarding that, please let me know if think it make sense to modify it now. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.js:239: Object.observe(subscription, function(changes) On 2015/06/07 21:09:59, Felix H. Dahlke wrote: > Nit: Why not just `Object.observe(subscription, onObjectChanged);`? Done. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.js:286: request.onload = function() On 2015/06/07 21:09:59, Felix H. Dahlke wrote: > Nit: Use addEventListener()? Done. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.js:345: function searchLanguage() On 2015/06/07 21:09:59, Felix H. Dahlke wrote: > Nit: Similar to above, I'd find this easier to understand if it was named to > indicate it's an event handler, e.g. `onFindLanguageKeyUp`. Done. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.js:392: // e.keyCode has been deprecated so we use e.key instead On 2015/06/07 21:09:59, Felix H. Dahlke wrote: > Nit: "so we attempt to use e.key"? Since we're actually falling back to keyCode. Done. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.js:396: addWhitelistedDomain(); On 2015/06/07 21:09:59, Felix H. Dahlke wrote: > Nit: Seeing how the else body is identical to the if body, why not merge the > conditions? Done. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.js:483: On 2015/06/07 21:09:59, Felix H. Dahlke wrote: > Nit: Seeing how we have a bunch of TODO comments here, I guess this deserves one > too? Done. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.js:570: case "disabled": On 2015/06/07 21:09:59, Felix H. Dahlke wrote: > Merge with case "added" above? Done. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/skin... File skin/options.css (right): http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/skin... skin/options.css:520: General tab content On 2015/06/07 21:09:59, Felix H. Dahlke wrote: > Seems like there's some stuff above that belongs below this comment, e.g. > #blocking-languages. Done. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/subs... File subscriptions.xml (right): http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/subs... subscriptions.xml:1: <?xml version="1.0"?> On 2015/06/03 16:20:43, Felix H. Dahlke wrote: > So the purpose of bundling this file here is so that we temporarily don't have > to change the format of the canonical one? > > Because this file is already bundled by the extension, and used to select the > default subscriptions, so duplicating it here is not a good idea. > > I'm OK with using a bundled file temporarily, but I'd like: > > 1. A follow-up issue for getting these changes into the real subscriptions.xml > file and using that one. > 2. A comment in this copy of the file here, that explains why it's bundled, > pointing to that issue. But do we want to have the recommendation available in old interface ? Please note if we will replace the existing subscriptions.xml from adblockplus repository it will also add recommendation to the list of suggested subscriptions. I see two possibility: * We can go with your suggestion and be ready that users will get recommendations before they will switch to new options UI (for example in FF). * have subscription.xml in adblockplusui repository and use metadata of builds to switch between subscription.xmls for each build. Separating old and new subscription.xml maybe can be handy ? In case of 1-st I think this can be done in current ticket -> https://issues.adblockplus.org/ticket/1422
http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... File options.js (right): http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.js:27: function Collection(details) On 2015/06/09 15:29:13, saroyanm wrote: > On 2015/06/07 21:09:59, Felix H. Dahlke wrote: > > Does details have to be an array? From what I've seen below, you're only ever > > passing a one element array to it. > > on line 139 I'm passing array from 2 elements. > "Adblocking for websites in.." section and "Added languages" in dialog have have > same data in there and handled same way. True, fair enough. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.js:32: Collection.prototype = Object.create(Array.prototype); On 2015/06/09 15:29:13, saroyanm wrote: > On 2015/06/07 21:09:59, Felix H. Dahlke wrote: > > From what I've seen, the only member functions used on Collection instances > are > > addItems(), removeItem() and clearAll(). In that case I believe we should > prefer > > composition over inheritance here, i.e. just store the items in a member > > variable rather than inheriting from Array. > > I would like to understand why it's more preferable to use composition in this > case instead of inheritance, I personally think it's more readable, for example > in the example of addItems method for me it's more intuitive that I'm sorting > and pushing the items into the object itself, I don't mind to use composition > instead I just would like to understand whether there is a reason I'm missing or > it's personal preference. It's a pretty common principle in OOP (http://en.wikipedia.org/wiki/Composition_over_inheritance). The whole thing about being a "has a" relationship versus being a "is a" relationship. More practically, inheritance gives clients API they are not supposed to use, since they would be violating invariants. In our concrete case here, there's an invariant that the elements should be sorted, which is upheld by addItems and removeItems. If clients were to use inherited API such as push, they could easily violate it. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.js:69: control.checked = item.disabled == false; On 2015/06/09 15:29:13, saroyanm wrote: > On 2015/06/07 21:15:24, Felix H. Dahlke wrote: > > On 2015/06/07 21:09:59, Felix H. Dahlke wrote: > > > Nit: "Do not compare x == true or x == false. Use (x) or (!x)" > > > > > > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style... > > > > > > There are more instances of this below. > > > > Sorry, wrong link, should have been: > > > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style... > > In this case is bit complicated, while the design itself is bit inconsistent > with logic, we now have subscription which are disabled and aren't added which > are treated differently on view, > so please note the case when we need to show the subscription in the case it's > "disabled" property is null or true. for this case we are using > subscription.desabled == false. > Or we should go with the case of !subscription.desabled && subscription.desabled > != null ? It's still bit confusing. Oh I didn't realise that. No then it's fine. http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.js:220: ["disabled"].forEach(function(property) On 2015/06/09 15:29:13, saroyanm wrote: > On 2015/06/07 21:09:59, Felix H. Dahlke wrote: > > This could be simplified :D > > For future we are going to not only listen to "disbled" property of > subscription, but also to other properties like -> title, lastDownload, homepage > and etc. > I've added comment regarding that, please let me know if think it make sense to > modify it now. If we're absolutely sure we'll need it, I'm OK with it.
Felix the new patch uploaded, can you also please comment under current reply please ? http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/subs... http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... File options.js (right): http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/opti... options.js:32: Collection.prototype = Object.create(Array.prototype); On 2015/06/11 14:40:20, Felix H. Dahlke wrote: > On 2015/06/09 15:29:13, saroyanm wrote: > > On 2015/06/07 21:09:59, Felix H. Dahlke wrote: > > > From what I've seen, the only member functions used on Collection instances > > are > > > addItems(), removeItem() and clearAll(). In that case I believe we should > > prefer > > > composition over inheritance here, i.e. just store the items in a member > > > variable rather than inheriting from Array. > > > > I would like to understand why it's more preferable to use composition in this > > case instead of inheritance, I personally think it's more readable, for > example > > in the example of addItems method for me it's more intuitive that I'm sorting > > and pushing the items into the object itself, I don't mind to use composition > > instead I just would like to understand whether there is a reason I'm missing > or > > it's personal preference. > > It's a pretty common principle in OOP > (http://en.wikipedia.org/wiki/Composition_over_inheritance). The whole thing > about being a "has a" relationship versus being a "is a" relationship. > > More practically, inheritance gives clients API they are not supposed to use, > since they would be violating invariants. In our concrete case here, there's an > invariant that the elements should be sorted, which is upheld by addItems and > removeItems. If clients were to use inherited API such as push, they could > easily violate it. Done.
http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/subs... File subscriptions.xml (right): http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/subs... subscriptions.xml:1: <?xml version="1.0"?> On 2015/06/09 15:29:13, saroyanm wrote: > On 2015/06/03 16:20:43, Felix H. Dahlke wrote: > > So the purpose of bundling this file here is so that we temporarily don't have > > to change the format of the canonical one? > > > > Because this file is already bundled by the extension, and used to select the > > default subscriptions, so duplicating it here is not a good idea. > > > > I'm OK with using a bundled file temporarily, but I'd like: > > > > 1. A follow-up issue for getting these changes into the real subscriptions.xml > > file and using that one. > > 2. A comment in this copy of the file here, that explains why it's bundled, > > pointing to that issue. > > But do we want to have the recommendation available in old interface ? Please > note if we will replace the existing subscriptions.xml from adblockplus > repository it will also add recommendation to the list of suggested > subscriptions. I see two possibility: > * We can go with your suggestion and be ready that users will get > recommendations before they will switch to new options UI (for example in FF). > * have subscription.xml in adblockplusui repository and use metadata of builds > to switch between subscription.xmls for each build. > Separating old and new subscription.xml maybe can be handy ? > > In case of 1-st I think this can be done in current ticket -> > https://issues.adblockplus.org/ticket/1422 What do you mean by recommendation? "popular"? I believe I also commented on that: The very fact that the subscription is in subscriptions.xml makes it "popluar", doesn't it? So that attribute is not necessary. If that distinction really is necessary, we want to be able to make the same distinction on other platforms. Note that subscriptions.xml is currently both bundled with the extension and fetched from our servers (as subscriptions2.xml). #1422 appears to be about actually using subscriptions2.xml from the server in ABP for Firefox instead of the bundled subscriptions.xml. ABP for Firefox uses both for different things, for reasons I have not yet been able to get behind. Do we really want to add a third subscriptions.xml now? I rather believe we need to: 1. Get all the subscription meta data and the canonical list of recommended/popular subscriptions into subscriptions2.xml on the server. 2. Bundle the latest version of subscriptions2.xml during the build, and update it regularly from the extension. But for the sake of progress, I'm fine with keeping subscriptions.xml here for the time being, as long as there's a comment and a follow-up issue for using the same subscriptions.xml we use for the language selection. http://codereview.adblockplus.org/6088024630755328/diff/5756596743307264/opti... File options.js (right): http://codereview.adblockplus.org/6088024630755328/diff/5756596743307264/opti... options.js:30: this.items = Object.create(Array.prototype); Wouldn't `this.items = []` do?
Patch updated, And replied to subscription.xml comment http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/subs... File subscriptions.xml (right): http://codereview.adblockplus.org/6088024630755328/diff/5754563613163520/subs... subscriptions.xml:1: <?xml version="1.0"?> On 2015/06/12 12:30:47, Felix H. Dahlke wrote: > On 2015/06/09 15:29:13, saroyanm wrote: > > On 2015/06/03 16:20:43, Felix H. Dahlke wrote: > > > So the purpose of bundling this file here is so that we temporarily don't > have > > > to change the format of the canonical one? > > > > > > Because this file is already bundled by the extension, and used to select > the > > > default subscriptions, so duplicating it here is not a good idea. > > > > > > I'm OK with using a bundled file temporarily, but I'd like: > > > > > > 1. A follow-up issue for getting these changes into the real > subscriptions.xml > > > file and using that one. > > > 2. A comment in this copy of the file here, that explains why it's bundled, > > > pointing to that issue. > > > > But do we want to have the recommendation available in old interface ? Please > > note if we will replace the existing subscriptions.xml from adblockplus > > repository it will also add recommendation to the list of suggested > > subscriptions. I see two possibility: > > * We can go with your suggestion and be ready that users will get > > recommendations before they will switch to new options UI (for example in FF). > > * have subscription.xml in adblockplusui repository and use metadata of builds > > to switch between subscription.xmls for each build. > > Separating old and new subscription.xml maybe can be handy ? > > > > In case of 1-st I think this can be done in current ticket -> > > https://issues.adblockplus.org/ticket/1422 > > What do you mean by recommendation? "popular"? Yes I mean popular. > that: The very fact that the subscription is in subscriptions.xml makes it > "popluar", doesn't it? So that attribute is not necessary. I'm using this attribute to mark them as popular in the interface, please note that when we are loading subscription that are not listed as popular in the XML they doesn't have popular mark next to them. > If that distinction really is necessary, we want to be able to make the same distinction on other > platforms. The thing is that we will have Facebook Annoyance block listed as well, we are not yet sure whether we want it to be listed or not, so I'll suggest to deal with subscription.xml in separate ticket as you suggested. > Note that subscriptions.xml is currently both bundled with the extension and > fetched from our servers (as subscriptions2.xml). > > #1422 appears to be about actually using subscriptions2.xml from the server in > ABP for Firefox instead of the bundled subscriptions.xml. ABP for Firefox uses > both for different things, for reasons I have not yet been able to get behind. Well #1422 marked as blocking for #1526, so that why I thought that they are connected to each other. > Do we really want to add a third subscriptions.xml now? I rather believe we need > to: > > 1. Get all the subscription meta data and the canonical list of > recommended/popular subscriptions into subscriptions2.xml on the server. > 2. Bundle the latest version of subscriptions2.xml during the build, and update > it regularly from the extension. Isn't it same what #1422 suggest ? It's missing the part of updating subscriptions2.xml with recommended/popular, also subscriptions2.xml contain subscriptions like Xfiles which we don't want to list in recommended subscriptions I think. > But for the sake of progress, I'm fine with keeping subscriptions.xml here for > the time being, as long as there's a comment and a follow-up issue for using the > same subscriptions.xml we use for the language selection. Sure I'll update the XML with the comment I just want to understand how we are trying to deal with this file, for proper comment and follow up ticket.
On 2015/06/12 13:11:59, saroyanm wrote: > The thing is that we will have Facebook Annoyance block listed as well, we are > not yet sure whether we want it to be listed or not, so I'll suggest to deal > with subscription.xml in separate ticket as you suggested. I'm fine with leaving popular in for now, discussing this in this issue is too much of a mess. > Isn't it same what #1422 suggest ? It's missing the part of updating > subscriptions2.xml with recommended/popular, also subscriptions2.xml contain > subscriptions like Xfiles which we don't want to list in recommended > subscriptions I think. #1422 is about this behaviour in Firefox - in Chrome things work differently, and here in the new UI it's a different story again. Sure, the root issue is the same: Two different subscription files. > > But for the sake of progress, I'm fine with keeping subscriptions.xml here for > > the time being, as long as there's a comment and a follow-up issue for using > the > > same subscriptions.xml we use for the language selection. > Sure I'll update the XML with the comment I just want to understand how we are > trying to deal with this file, for proper comment and follow up ticket. I had a go at creating that issue :P https://issues.adblockplus.org/ticket/2677
On 2015/06/12 16:48:07, Felix H. Dahlke wrote: > I had a go at creating that issue :P > > https://issues.adblockplus.org/ticket/2677 Oops I was thinking about change ticket creation, well this ticket doesn't provide solution, but describing the problem itself, anyway better than nothing. Thanks for creating the ticket, make more sense to discuss the solution under the ticket, agree.
Patch updated.
LGTM. Disclaimer: I did more of a high level review here, and a couple of things that need to be changed will be addressed in follow-ups, for the sake of progress. On 2015/06/13 13:02:25, saroyanm wrote: > On 2015/06/12 16:48:07, Felix H. Dahlke wrote: > > I had a go at creating that issue :P > > > > https://issues.adblockplus.org/ticket/2677 > Oops I was thinking about change ticket creation, well this ticket doesn't > provide solution, but describing the problem itself, anyway better than nothing. > Thanks for creating the ticket, make more sense to discuss the solution under > the ticket, agree. I didn't have this case before either. Quickly discussed it with Philip and he suggested to use a defect for quality issues we're not sure how to address yet, went with that. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
