Issue 29317001: Relocated icon and redesigned icon popup (Closed)
|
Created:
Nov. 7, 2013, 5:44 p.m. by Thomas Greiner Modified:
Dec. 17, 2013, 2:47 p.m. Visibility:
Public. |
DescriptionThis review includes the following changes:
- relocated icon to toolbar
- added blocked requests number to icon (including opt-out mechanism)
- changed icon image
- added opt-out for number in icon
- applied Sven's design
- removed jQuery dependency
Patch Set 1 #Patch Set 2 : Rebased #
Total comments: 15
Patch Set 3 : Applied Sebastian's suggestions and updated strings #
Total comments: 52
Patch Set 4 : Addressed Felix' comments #
Total comments: 9
Patch Set 5 : Addressed Wladimir's comments #Patch Set 6 : #Patch Set 7 : #Patch Set 8 : Merged setBadgeNumber and setBadgeBackgroundColor #
Total comments: 2
MessagesTotal messages: 38
Please remember to update all chrome.* API calls to ext.* (our new Chrome/Safari abstraction layer). Otherwise the the Safari build will break.
Also note that the bubble has a fixed size in Safari. So please set it to an appropriate size in metadata.safari. This also means that the height of the bubble can not depend on the context and that it must not have expandable areas.
Rebased to revision 1026
LGTM for the strings, with some suggestions for the descriptions below. http://codereview.adblockplus.org/29317001/diff/5668600916475904/_locales/en_... File _locales/en_US/messages.json (right): http://codereview.adblockplus.org/29317001/diff/5668600916475904/_locales/en_... _locales/en_US/messages.json:44: "description": "Popup balloon label to disable blocking", I'm not sure if balloon is the right term, pretty Windows-specific AFAIK. How about "Popup menu"? Likewise below. http://codereview.adblockplus.org/29317001/diff/5668600916475904/_locales/en_... _locales/en_US/messages.json:106: "description": "Link to options page from balloon popup", s/to/to the/ and s/from/from the/ http://codereview.adblockplus.org/29317001/diff/5668600916475904/_locales/en_... _locales/en_US/messages.json:167: "description": "Toggle display of number of blocked ads in icon.", How about "Toggle for displaying the number of blocked ads in the icon"
http://codereview.adblockplus.org/29317001/diff/5668600916475904/popup.js File popup.js (right): http://codereview.adblockplus.org/29317001/diff/5668600916475904/popup.js#new... popup.js:19: var imports = ["require", "isWhitelisted", "extractHostFromURL", "refreshIconAndContextMenu", "openOptions"]; I like that you have added openOptions to the imports, which made the wrapper below obsolete. However you didn't removed it. So this import will be overridden by "function openOptions() { backgroundPage.openOptions(); }".
http://codereview.adblockplus.org/29317001/diff/5668600916475904/safari/backg... File safari/background.js (right): http://codereview.adblockplus.org/29317001/diff/5668600916475904/safari/backg... safari/background.js:94: setBadgeText: function(text) {} It turned out that Safari actually can show a badge on toolbar items. However it can only be a number. So you should implement that function and consider renaming it to setBadgeNumber. setBadgeNumber: function(n) { safari.extension.toolbarItems[0].badge = n; } https://developer.apple.com/library/safari/documentation/UserExperience/Refer...
LGTM on the strings (Felix' nits still apply unless I noted otherwise). I have a concern listed below but it isn't related to strings. http://codereview.adblockplus.org/29317001/diff/5668600916475904/_locales/en_... File _locales/en_US/messages.json (right): http://codereview.adblockplus.org/29317001/diff/5668600916475904/_locales/en_... _locales/en_US/messages.json:44: "description": "Popup balloon label to disable blocking", On 2013/11/19 15:11:43, Felix H. Dahlke wrote: > I'm not sure if balloon is the right term, pretty Windows-specific AFAIK. How > about "Popup menu"? Likewise below. Given that a Google search for "popup balloon" immediately shows the right thing - this seems to be fine. http://codereview.adblockplus.org/29317001/diff/5668600916475904/_locales/en_... _locales/en_US/messages.json:168: "message": "Show number in icon" That number will quickly go into thousands, are you sure that showing it in the icon makes sense at all?
http://codereview.adblockplus.org/29317001/diff/5668600916475904/_locales/en_... File _locales/en_US/messages.json (right): http://codereview.adblockplus.org/29317001/diff/5668600916475904/_locales/en_... _locales/en_US/messages.json:44: "description": "Popup balloon label to disable blocking", On 2013/11/26 15:31:23, Wladimir Palant wrote: > On 2013/11/19 15:11:43, Felix H. Dahlke wrote: > > I'm not sure if balloon is the right term, pretty Windows-specific AFAIK. How > > about "Popup menu"? Likewise below. > > Given that a Google search for "popup balloon" immediately shows the right thing > - this seems to be fine. Fair enough. http://codereview.adblockplus.org/29317001/diff/5668600916475904/_locales/en_... _locales/en_US/messages.json:168: "message": "Show number in icon" On 2013/11/26 15:31:23, Wladimir Palant wrote: > That number will quickly go into thousands, are you sure that showing it in the > icon makes sense at all? It's the number of ads blocked on the current page, I doubt it'll go that high. The idea behind this is to make people notice the ad count, worked pretty well for AdBlock for Chrome.
http://codereview.adblockplus.org/29317001/diff/5668600916475904/_locales/en_... File _locales/en_US/messages.json (right): http://codereview.adblockplus.org/29317001/diff/5668600916475904/_locales/en_... _locales/en_US/messages.json:168: "message": "Show number in icon" On 2013/11/26 15:33:46, Felix H. Dahlke wrote: > It's the number of ads blocked on the current page, I see - than it is fine of course. I was under the impression that this is about the total count.
Applied Sebastian's suggestions and updated strings http://codereview.adblockplus.org/29317001/diff/5668600916475904/_locales/en_... File _locales/en_US/messages.json (right): http://codereview.adblockplus.org/29317001/diff/5668600916475904/_locales/en_... _locales/en_US/messages.json:44: "description": "Popup balloon label to disable blocking", On 2013/11/26 15:33:46, Felix H. Dahlke wrote: > On 2013/11/26 15:31:23, Wladimir Palant wrote: > > On 2013/11/19 15:11:43, Felix H. Dahlke wrote: > > > I'm not sure if balloon is the right term, pretty Windows-specific AFAIK. > How > > > about "Popup menu"? Likewise below. > > > > Given that a Google search for "popup balloon" immediately shows the right > thing > > - this seems to be fine. > > Fair enough. Done. http://codereview.adblockplus.org/29317001/diff/5668600916475904/_locales/en_... _locales/en_US/messages.json:106: "description": "Link to options page from balloon popup", On 2013/11/19 15:11:43, Felix H. Dahlke wrote: > s/to/to the/ and s/from/from the/ Done. http://codereview.adblockplus.org/29317001/diff/5668600916475904/_locales/en_... _locales/en_US/messages.json:167: "description": "Toggle display of number of blocked ads in icon.", On 2013/11/19 15:11:43, Felix H. Dahlke wrote: > How about "Toggle for displaying the number of blocked ads in the icon" Done. http://codereview.adblockplus.org/29317001/diff/5668600916475904/popup.js File popup.js (right): http://codereview.adblockplus.org/29317001/diff/5668600916475904/popup.js#new... popup.js:19: var imports = ["require", "isWhitelisted", "extractHostFromURL", "refreshIconAndContextMenu", "openOptions"]; On 2013/11/19 15:22:08, sebastian wrote: > I like that you have added openOptions to the imports, which made the wrapper > below obsolete. However you didn't removed it. So this import will be overridden > by "function openOptions() { backgroundPage.openOptions(); }". Done. http://codereview.adblockplus.org/29317001/diff/5668600916475904/safari/backg... File safari/background.js (right): http://codereview.adblockplus.org/29317001/diff/5668600916475904/safari/backg... safari/background.js:94: setBadgeText: function(text) {} On 2013/11/19 15:27:54, sebastian wrote: > It turned out that Safari actually can show a badge on toolbar items. However it > can only be a number. So you should implement that function and consider > renaming it to setBadgeNumber. > > setBadgeNumber: function(n) > { > safari.extension.toolbarItems[0].badge = n; > } > > https://developer.apple.com/library/safari/documentation/UserExperience/Refer... Done.
LGTM
Reviewed the rest of the code (although I have to admit I didn't review the CSS in-depth), looks good mostly. http://codereview.adblockplus.org/29317001/diff/5631943370604544/lib/stats.js File lib/stats.js (right): http://codereview.adblockplus.org/29317001/diff/5631943370604544/lib/stats.js... lib/stats.js:104: forEachTab(function(tab) { Opening brace should go on the next line. http://codereview.adblockplus.org/29317001/diff/5631943370604544/lib/stats.js... lib/stats.js:109: tab.browserAction.setBadgeNumber(frameData.blocked); Shouldn't we set the badge number to null explicitly if something's wrong with frameData? I think the whole function could be simplified if we do that. http://codereview.adblockplus.org/29317001/diff/5631943370604544/popup.js File popup.js (right): http://codereview.adblockplus.org/29317001/diff/5631943370604544/popup.js#new... popup.js:45: document.getElementById("options").addEventListener("click", function() Why not just |.addEventListener("click", openOptions, false);|? http://codereview.adblockplus.org/29317001/diff/5631943370604544/popup.js#new... popup.js:54: collapsers[i].addEventListener("click", toggleCollapse.bind(collapsers[i]), true); Nit: Wouldn't hurt to assign collapsers[i] to a temp here, since it's used a lot. http://codereview.adblockplus.org/29317001/diff/5631943370604544/popup.js#new... popup.js:132: function toggleCollapse(ev) The parameter is unused, might as well get rid of it. http://codereview.adblockplus.org/29317001/diff/5631943370604544/skin/popup.css File skin/popup.css (right): http://codereview.adblockplus.org/29317001/diff/5631943370604544/skin/popup.c... skin/popup.css:189: #enabled.off .icon I find it confusing that there's both #enabled-off and #enabled.off - can you find a less ambiguous term for one case? http://codereview.adblockplus.org/29317001/diff/5631943370604544/skin/popup.c... skin/popup.css:199: #enabled.off #enabled-on Can you merge this and the rule set above? http://codereview.adblockplus.org/29317001/diff/5631943370604544/stats.js File stats.js (right): http://codereview.adblockplus.org/29317001/diff/5631943370604544/stats.js#new... stats.js:74: showIconNumber.setAttribute("aria-checked", Prefs.show_iconnumber); What does "aria" mean here?
http://codereview.adblockplus.org/29317001/diff/5631943370604544/lib/stats.js File lib/stats.js (right): http://codereview.adblockplus.org/29317001/diff/5631943370604544/lib/stats.js... lib/stats.js:104: forEachTab(function(tab) { On 2013/12/02 15:45:58, Felix H. Dahlke wrote: > Opening brace should go on the next line. Done. http://codereview.adblockplus.org/29317001/diff/5631943370604544/lib/stats.js... lib/stats.js:109: tab.browserAction.setBadgeNumber(frameData.blocked); The value is already null when we get here (either because it hasn't been set yet or because it was removed previously as seen below) so it seems redundant. I can, however, put the if(Prefs.show_iconnumber) inside the loop if that's what you mean with "simplify". http://codereview.adblockplus.org/29317001/diff/5631943370604544/popup.js File popup.js (right): http://codereview.adblockplus.org/29317001/diff/5631943370604544/popup.js#new... popup.js:45: document.getElementById("options").addEventListener("click", function() I try to avoid passing event parameters on to external functions that are not solely used as event listeners. Might be an unnecessary optimization, though, so I changed it now. http://codereview.adblockplus.org/29317001/diff/5631943370604544/popup.js#new... popup.js:54: collapsers[i].addEventListener("click", toggleCollapse.bind(collapsers[i]), true); On 2013/12/02 15:45:58, Felix H. Dahlke wrote: > Nit: Wouldn't hurt to assign collapsers[i] to a temp here, since it's used a > lot. Done. http://codereview.adblockplus.org/29317001/diff/5631943370604544/popup.js#new... popup.js:132: function toggleCollapse(ev) On 2013/12/02 15:45:58, Felix H. Dahlke wrote: > The parameter is unused, might as well get rid of it. Done. http://codereview.adblockplus.org/29317001/diff/5631943370604544/skin/popup.css File skin/popup.css (right): http://codereview.adblockplus.org/29317001/diff/5631943370604544/skin/popup.c... skin/popup.css:189: #enabled.off .icon On 2013/12/02 15:45:58, Felix H. Dahlke wrote: > I find it confusing that there's both #enabled-off and #enabled.off - can you > find a less ambiguous term for one case? Done. http://codereview.adblockplus.org/29317001/diff/5631943370604544/skin/popup.c... skin/popup.css:199: #enabled.off #enabled-on On 2013/12/02 15:45:58, Felix H. Dahlke wrote: > Can you merge this and the rule set above? Done. http://codereview.adblockplus.org/29317001/diff/5631943370604544/stats.js File stats.js (right): http://codereview.adblockplus.org/29317001/diff/5631943370604544/stats.js#new... stats.js:74: showIconNumber.setAttribute("aria-checked", Prefs.show_iconnumber); It's a standard set of attributes to make custom elements more accessible (e.g. label a div as a button which acts as one). I know that we're currently not using this standard but I thought in this case - since I needed a "checked" attribute anyway - it might be good to use "aria-checked" instead of "data-checked". More information: (1) https://developer.mozilla.org/en-US/docs/Accessibility/ARIA (2) http://www.w3.org/TR/wai-aria/states_and_properties
My comments apply to patch set 3 - patch set 4 was uploaded while I was reviewing. icons/abp-19-whitelisted.png is the wrong icon - it is clearly a resized (and blurred) version of a larger icon. There was already a proper abp-19-whitelisted.png icon a few revision back, now it is being generated automatically at build however as a greyscale version of abp-19.png. I don't see why you decided to add this icon back. Concerning you replacing abp-19.png - please don't. Your version is badly blurred and not optimized for smaller sizes. http://codereview.adblockplus.org/29317001/diff/5631943370604544/chrome/backg... File chrome/background.js (right): http://codereview.adblockplus.org/29317001/diff/5631943370604544/chrome/backg... chrome/background.js:230: } This is a strange API - we require setting each parameter of the badge separately. Also, we assume that the badge will never contain anything other than a number and we pass null instead of a number to remove the badge. IMHO it should be rather something like the following: setBadge: function(badge) { if (!badge) // Remove badge else { if (!badge.text) throw Error("Badge text is required); chrome.browserAction.setBadgeText(... badge.text ...); if ("backgroundColor" in badge) chrome.browserAction.setBadgeBackgroundColor(... badge.backgroundColor ...); } } http://codereview.adblockplus.org/29317001/diff/5631943370604544/lib/prefs.js File lib/prefs.js (right): http://codereview.adblockplus.org/29317001/diff/5631943370604544/lib/prefs.js... lib/prefs.js:39: show_popupstats: true "iconnumber" doesn't really explain what this is about. It should be "show_statsinicon" and "show_statsinpopup" or something like that. http://codereview.adblockplus.org/29317001/diff/5631943370604544/lib/stats.js File lib/stats.js (right): http://codereview.adblockplus.org/29317001/diff/5631943370604544/lib/stats.js... lib/stats.js:26: let badgeColor = [100, 100, 100, 255]; I'd prefer "#606060" here - it's more compact and it is more obvious what it represents. http://codereview.adblockplus.org/29317001/diff/5631943370604544/lib/stats.js... lib/stats.js:72: tab.browserAction.setBadgeNumber(frameData.blocked); Will this count be automatically removed if the tab location changes? http://codereview.adblockplus.org/29317001/diff/5631943370604544/lib/stats.js... lib/stats.js:85: for (let i = 0; i < windows.length; i++) |for each (let window in windows)| should be a better way to write this, similarly for tabs below. That's a Firefox-only construct but so is the let statement. http://codereview.adblockplus.org/29317001/diff/5631943370604544/popup.html File popup.html (right): http://codereview.adblockplus.org/29317001/diff/5631943370604544/popup.html#n... popup.html:41: <div id="close-notification"></div> This is an inline element, better use <span>. http://codereview.adblockplus.org/29317001/diff/5631943370604544/popup.js File popup.js (right): http://codereview.adblockplus.org/29317001/diff/5631943370604544/popup.js#new... popup.js:45: document.getElementById("options").addEventListener("click", function() On 2013/12/03 12:06:05, Thomas Greiner wrote: > I try to avoid passing event parameters on to external functions that are not > solely used as event listeners. Actually correct here - otherwise the event will be passed as callback parameter and cause an exception. http://codereview.adblockplus.org/29317001/diff/5631943370604544/popup.js#new... popup.js:54: collapsers[i].addEventListener("click", toggleCollapse.bind(collapsers[i]), true); On 2013/12/02 15:45:58, Felix H. Dahlke wrote: > Nit: Wouldn't hurt to assign collapsers[i] to a temp here, since it's used a > lot. Better solution - add toggleCollapse as callback without binding it to a particular node, use event.currentTarget to get the node. Also, any particular reason why the last parameter isn't false? http://codereview.adblockplus.org/29317001/diff/5631943370604544/popup.js#new... popup.js:56: document.getElementById(collapsers[i].dataset.collapsable).classList.add("collapsed"); Isn't the logic reversed here? I think it should be collapsed if the pref is false - at least the way that pref is called right now. http://codereview.adblockplus.org/29317001/diff/5631943370604544/popup.js#new... popup.js:82: var checked = enabledButton.classList.contains("off"); classList.toggle() returns the new value - no need to check the value and toggle separately. How about: var disabled = enabledButton.classList.toggle("off"); if (disabled) ... else ... http://codereview.adblockplus.org/29317001/diff/5631943370604544/popup.js#new... popup.js:132: function toggleCollapse(ev) As I said, it's better to use that parameter and replace this by event.currentTarget. Btw, the canonical name for that parameter is event. http://codereview.adblockplus.org/29317001/diff/5631943370604544/safari/backg... File safari/background.js (right): http://codereview.adblockplus.org/29317001/diff/5631943370604544/safari/backg... safari/background.js:94: // Safari does not provide these functionalities Seeing the plural of functionality is weird, even though that word apparently does have a plural. I would rather say: "The following features aren't supported by Safari." http://codereview.adblockplus.org/29317001/diff/5631943370604544/stats.js File stats.js (right): http://codereview.adblockplus.org/29317001/diff/5631943370604544/stats.js#new... stats.js:74: showIconNumber.setAttribute("aria-checked", Prefs.show_iconnumber); On 2013/12/03 12:06:05, Thomas Greiner wrote: > I know that we're currently not using this standard That's mostly because we are usually using standard widget where we don't have to identify the role or state explicitly. http://codereview.adblockplus.org/29317001/diff/5732694713434112/skin/popup.css File skin/popup.css (right): http://codereview.adblockplus.org/29317001/diff/5732694713434112/skin/popup.c... skin/popup.css:119: background-image: url(background-main-hover.png); Are we seriously changing the background on hover? Can we still reconsider?
http://codereview.adblockplus.org/29317001/diff/5631943370604544/chrome/backg... File chrome/background.js (right): http://codereview.adblockplus.org/29317001/diff/5631943370604544/chrome/backg... chrome/background.js:230: } On 2013/12/03 12:55:43, Wladimir Palant wrote: > This is a strange API - we require setting each parameter of the badge > separately. Also, we assume that the badge will never contain anything other > than a number and we pass null instead of a number to remove the badge. IMHO it > should be rather something like the following: > > setBadge: function(badge) > { > if (!badge) > // Remove badge > else > { > if (!badge.text) > throw Error("Badge text is required); > chrome.browserAction.setBadgeText(... badge.text ...); > if ("backgroundColor" in badge) > chrome.browserAction.setBadgeBackgroundColor(... badge.backgroundColor > ...); > } > } Safari only support numbers for the badge. That is why we call it setBadgeNumber() and don't support arbitrary strings here. Also I don't see why it would be better to have a single function, that takes an object with the badge number/text and the background color, instead of having two functions as in Chrome's API.
http://codereview.adblockplus.org/29317001/diff/5631943370604544/chrome/backg... File chrome/background.js (right): http://codereview.adblockplus.org/29317001/diff/5631943370604544/chrome/backg... chrome/background.js:230: } On 2013/12/03 13:12:26, sebastian wrote: > Safari only support numbers for the badge. That is why we call it > setBadgeNumber() and don't support arbitrary strings here. I see. I guess calling it badge.number rather than badge.text makes more sense then. > Also I don't see why it would be better to have a single function, that takes an > object with the badge number/text and the background color, instead of having > two functions as in Chrome's API. Why should have two function calls to configure the badge? I cannot imagine us calling the one function without the other meaning that separating them makes little sense. Chrome API likely separates them for historical reasons, we don't have those. Note that in Firefox we'll be able to control foreground color as well - I doubt that we want to add a third function call if we decide to support Firefox here.
You have removed the padding from icons/abp-19.png, but not from the notification icons. You have added icons/abp-19-whitelisted.png, but it is auto-generated. So please remove it.
http://codereview.adblockplus.org/29317001/diff/5631943370604544/chrome/backg... File chrome/background.js (right): http://codereview.adblockplus.org/29317001/diff/5631943370604544/chrome/backg... chrome/background.js:230: } On 2013/12/03 13:45:03, Wladimir Palant wrote: > > Also I don't see why it would be better to have a single function, that takes > an > > object with the badge number/text and the background color, instead of having > > two functions as in Chrome's API. > > Why should have two function calls to configure the badge? I cannot imagine us > calling the one function without the other meaning that separating them makes > little sense. Chrome API likely separates them for historical reasons, we don't > have those. > > Note that in Firefox we'll be able to control foreground color as well - I doubt > that we want to add a third function call if we decide to support Firefox here. I still see no advantage of a single function with complex calling convention over multiple functions with a simple calling convention. However I know plenty of reasons, why multiple functions would be preferable, here: 1. Without looking at the implementation (or documentation) everybody knows how to call the function, just from its name. 2. It is more flexible, because you can set the label and background color independently. 3. The implementation is simpler (and faster) without all the unneeded conditions required to dispatch, all the mandatory and optional parameters. 4. It is more similar to Chrome's API, and therefore developers/contributors who already know Chrome's API, have to struggle less with our API, if we adopt it.
Ignore the new icon images from now on as we decided to move those changes to a separate review. http://codereview.adblockplus.org/29317001/diff/5631943370604544/chrome/backg... File chrome/background.js (right): http://codereview.adblockplus.org/29317001/diff/5631943370604544/chrome/backg... chrome/background.js:230: } > Why should have two function calls to configure the badge? I cannot imagine us > calling the one function without the other meaning that separating them makes > little sense. Chrome API likely separates them for historical reasons, we don't > have those. The reason is that in Chrome's API you can either make a single call to set the background color globally for each tab or for each tab specifically. However, I agree that it might be sufficient to only implement it tab-dependently given that the calls are currently being made like that anyway. So merging those functions makes sense in our case. @Sebastian 1. Shouldn't be too big of a problem if we add a documentation above it. 2. We could make both properties optional. This would allow us to either set one or both properties which gives us the biggest possible flexibility. 3. See (2) 4. The advantage of adhering to Chrome's API definition is marginal. We already differ quite a bit from it anyway but that's OK. http://codereview.adblockplus.org/29317001/diff/5631943370604544/lib/prefs.js File lib/prefs.js (right): http://codereview.adblockplus.org/29317001/diff/5631943370604544/lib/prefs.js... lib/prefs.js:39: show_popupstats: true On 2013/12/03 12:55:43, Wladimir Palant wrote: > "iconnumber" doesn't really explain what this is about. It should be > "show_statsinicon" and "show_statsinpopup" or something like that. Done. http://codereview.adblockplus.org/29317001/diff/5631943370604544/lib/stats.js File lib/stats.js (right): http://codereview.adblockplus.org/29317001/diff/5631943370604544/lib/stats.js... lib/stats.js:26: let badgeColor = [100, 100, 100, 255]; On 2013/12/03 12:55:43, Wladimir Palant wrote: > I'd prefer "#606060" here - it's more compact and it is more obvious what it > represents. Done. http://codereview.adblockplus.org/29317001/diff/5631943370604544/lib/stats.js... lib/stats.js:72: tab.browserAction.setBadgeNumber(frameData.blocked); On 2013/12/03 12:55:43, Wladimir Palant wrote: > Will this count be automatically removed if the tab location changes? Yes, it gets reset implicitly. http://codereview.adblockplus.org/29317001/diff/5631943370604544/lib/stats.js... lib/stats.js:85: for (let i = 0; i < windows.length; i++) On 2013/12/03 12:55:43, Wladimir Palant wrote: > |for each (let window in windows)| should be a better way to write this, > similarly for tabs below. That's a Firefox-only construct but so is the let > statement. Done. http://codereview.adblockplus.org/29317001/diff/5631943370604544/popup.html File popup.html (right): http://codereview.adblockplus.org/29317001/diff/5631943370604544/popup.html#n... popup.html:41: <div id="close-notification"></div> On 2013/12/03 12:55:43, Wladimir Palant wrote: > This is an inline element, better use <span>. Done. http://codereview.adblockplus.org/29317001/diff/5631943370604544/popup.js File popup.js (right): http://codereview.adblockplus.org/29317001/diff/5631943370604544/popup.js#new... popup.js:45: document.getElementById("options").addEventListener("click", function() On 2013/12/03 12:55:43, Wladimir Palant wrote: > On 2013/12/03 12:06:05, Thomas Greiner wrote: > > I try to avoid passing event parameters on to external functions that are not > > solely used as event listeners. > > Actually correct here - otherwise the event will be passed as callback parameter > and cause an exception. Done. http://codereview.adblockplus.org/29317001/diff/5631943370604544/popup.js#new... popup.js:54: collapsers[i].addEventListener("click", toggleCollapse.bind(collapsers[i]), true); On 2013/12/03 12:55:43, Wladimir Palant wrote: > On 2013/12/02 15:45:58, Felix H. Dahlke wrote: > > Nit: Wouldn't hurt to assign collapsers[i] to a temp here, since it's used a > > lot. > > Better solution - add toggleCollapse as callback without binding it to a > particular node, use event.currentTarget to get the node. Done. > Also, any particular reason why the last parameter isn't false? No, not anymore. http://codereview.adblockplus.org/29317001/diff/5631943370604544/popup.js#new... popup.js:56: document.getElementById(collapsers[i].dataset.collapsable).classList.add("collapsed"); On 2013/12/03 12:55:43, Wladimir Palant wrote: > Isn't the logic reversed here? I think it should be collapsed if the pref is > false - at least the way that pref is called right now. Done. http://codereview.adblockplus.org/29317001/diff/5631943370604544/popup.js#new... popup.js:82: var checked = enabledButton.classList.contains("off"); On 2013/12/03 12:55:43, Wladimir Palant wrote: > classList.toggle() returns the new value - no need to check the value and toggle > separately. How about: > > var disabled = enabledButton.classList.toggle("off"); > if (disabled) > ... > else > ... Done. http://codereview.adblockplus.org/29317001/diff/5631943370604544/popup.js#new... popup.js:132: function toggleCollapse(ev) On 2013/12/03 12:55:43, Wladimir Palant wrote: > As I said, it's better to use that parameter and replace this by > event.currentTarget. Btw, the canonical name for that parameter is event. Done. http://codereview.adblockplus.org/29317001/diff/5631943370604544/safari/backg... File safari/background.js (right): http://codereview.adblockplus.org/29317001/diff/5631943370604544/safari/backg... safari/background.js:94: // Safari does not provide these functionalities On 2013/12/03 12:55:43, Wladimir Palant wrote: > Seeing the plural of functionality is weird, even though that word apparently > does have a plural. I would rather say: "The following features aren't supported > by Safari." Done.
http://codereview.adblockplus.org/29317001/diff/5631943370604544/chrome/backg... File chrome/background.js (right): http://codereview.adblockplus.org/29317001/diff/5631943370604544/chrome/backg... chrome/background.js:230: } On 2013/12/04 10:44:50, Thomas Greiner wrote: > > Why should have two function calls to configure the badge? I cannot imagine us > > calling the one function without the other meaning that separating them makes > > little sense. Chrome API likely separates them for historical reasons, we > don't > > have those. > > The reason is that in Chrome's API you can either make a single call to set the > background color globally for each tab or for each tab specifically. Or to set the background color only once, instead of every time you update the badge number. Are there any reasons why you set the background color every time you update the number? > @Sebastian > 1. Shouldn't be too big of a problem if we add a documentation above it. This just mitigates an issue, that we won't have otherwise. So it certainly isn't a reason to use a single function. > 2. We could make both properties optional. This would allow us to either set one > or both properties which gives us the biggest possible flexibility. > 3. See (2) Yes, in that case we would achieve the same flexibility, as with multiple functions. However it still makes the implementation more complex as it has to be, due to the need of conditions to dispatch the parameters. > 4. The advantage of adhering to Chrome's API definition is marginal. We already > differ quite a bit from it anyway but that's OK. Only in cases where it would be not worth the effort or is impossible to adopt Chrome's API on Safari. This isn't the case here.
http://codereview.adblockplus.org/29317001/diff/5631943370604544/lib/stats.js File lib/stats.js (right): http://codereview.adblockplus.org/29317001/diff/5631943370604544/lib/stats.js... lib/stats.js:109: tab.browserAction.setBadgeNumber(frameData.blocked); On 2013/12/03 12:06:05, Thomas Greiner wrote: > The value is already null when we get here (either because it hasn't been set > yet or because it was removed previously as seen below) so it seems redundant. True, fine like this then. > I can, however, put the if(Prefs.show_iconnumber) inside the loop if that's what > you mean with "simplify". Yup, I had something like this in mind: forEachTab(function(tab) { let badgeNumber = null; if (Prefs.show_iconnumber) { let frameData = getFrameData(tab, 0); if (frameData && "blocked" in frameData) badgeNumber = frameData.blocked; } tab.browserAction.setBadgeNumber(badgeNumber); } http://codereview.adblockplus.org/29317001/diff/5631943370604544/stats.js File stats.js (right): http://codereview.adblockplus.org/29317001/diff/5631943370604544/stats.js#new... stats.js:74: showIconNumber.setAttribute("aria-checked", Prefs.show_iconnumber); On 2013/12/03 12:55:43, Wladimir Palant wrote: > On 2013/12/03 12:06:05, Thomas Greiner wrote: > > I know that we're currently not using this standard > > That's mostly because we are usually using standard widget where we don't have > to identify the role or state explicitly. Ah, I see. Well I would definitely prefer using standard controls, but this aria stuff seems like the next best thing. http://codereview.adblockplus.org/29317001/diff/5732694713434112/skin/popup.css File skin/popup.css (right): http://codereview.adblockplus.org/29317001/diff/5732694713434112/skin/popup.c... skin/popup.css:119: background-image: url(background-main-hover.png); On 2013/12/03 12:55:43, Wladimir Palant wrote: > Are we seriously changing the background on hover? Can we still reconsider? I thought the same to be honest. It's hard to judge without seeing this in action, but I thought just underlining the settings link would suffice affordance-wise.
http://codereview.adblockplus.org/29317001/diff/5631943370604544/chrome/backg... File chrome/background.js (right): http://codereview.adblockplus.org/29317001/diff/5631943370604544/chrome/backg... chrome/background.js:230: } On 2013/12/04 11:48:38, sebastian wrote: > Or to set the background color only once, instead of every time you update the > badge number. Are there any reasons why you set the background color every time > you update the number? The only reason is that it would be more confusing to have |browserAction| in two different places (ext.browserAction.setBadgeBackgroundColor and Tab.browserAction.setBadgeNumber). Performance-wise it shouldn't be a big deal anyway. http://codereview.adblockplus.org/29317001/diff/5631943370604544/lib/stats.js File lib/stats.js (right): http://codereview.adblockplus.org/29317001/diff/5631943370604544/lib/stats.js... lib/stats.js:109: tab.browserAction.setBadgeNumber(frameData.blocked); On 2013/12/04 12:30:06, Felix H. Dahlke wrote: > On 2013/12/03 12:06:05, Thomas Greiner wrote: > > The value is already null when we get here (either because it hasn't been set > > yet or because it was removed previously as seen below) so it seems redundant. > > True, fine like this then. > > > I can, however, put the if(Prefs.show_iconnumber) inside the loop if that's > what > > you mean with "simplify". > > Yup, I had something like this in mind: > > forEachTab(function(tab) > { > let badgeNumber = null; > if (Prefs.show_iconnumber) > { > let frameData = getFrameData(tab, 0); > if (frameData && "blocked" in frameData) > badgeNumber = frameData.blocked; > } > tab.browserAction.setBadgeNumber(badgeNumber); > } Done. http://codereview.adblockplus.org/29317001/diff/5732694713434112/skin/popup.css File skin/popup.css (right): http://codereview.adblockplus.org/29317001/diff/5732694713434112/skin/popup.c... skin/popup.css:119: background-image: url(background-main-hover.png); Are we talking about performance, extension size increase or design here? Regarding the last one it's more consistent with the menu items' hover effect. Otherwise the need for the second background image could be eliminated by overlaying a semi-transparent layer over it to achieve the same effect.
It's just about the hover effect. http://codereview.adblockplus.org/29317001/diff/5732694713434112/skin/popup.css File skin/popup.css (right): http://codereview.adblockplus.org/29317001/diff/5732694713434112/skin/popup.c... skin/popup.css:119: background-image: url(background-main-hover.png); IMHO it's very unusual and insufficient only underlining the label of a button if the hover effect is activated after hovering a way bigger element. If you right or left click to any extension in a browser you will see that it's common practice to change the background color after hovering a menu item. The other argument for a background change is like thomas mentioned the consistency. So which objective arguments are there to change it and did you mean only the footer or all menu items Wladimir? On 2013/12/04 12:30:07, Felix H. Dahlke wrote: > On 2013/12/03 12:55:43, Wladimir Palant wrote: > > Are we seriously changing the background on hover? Can we still reconsider? > > I thought the same to be honest. It's hard to judge without seeing this in > action, but I thought just underlining the settings link would suffice > affordance-wise.
It's just about the hover effect.
http://codereview.adblockplus.org/29317001/diff/5732694713434112/skin/popup.css File skin/popup.css (right): http://codereview.adblockplus.org/29317001/diff/5732694713434112/skin/popup.c... skin/popup.css:119: background-image: url(background-main-hover.png); On 2013/12/10 10:09:29, Thomas Greiner wrote: > Are we talking about performance, extension size increase or design here? Design, isn't that obvious? On 2013/12/11 11:00:27, sven wrote: > IMHO it's very unusual and insufficient only underlining the label of a button > if the hover effect is activated after hovering a way bigger element. Yes, I meant just the link should react to hover/click events of course. > If you > right or left click to any extension in a browser you will see that it's common > practice to change the background color after hovering a menu item. The difference here is that this looks like a part of the border last time I checked, and hence weird. But I've never seen it in action, I can only imagine. > So > which objective arguments are there to change it and did you mean only the > footer or all menu items Wladimir? I suggested an alternative here, not Wladimir. But we can't have that discussion here, I need to see it in action. Can either of you send some current screenshots over?
http://codereview.adblockplus.org/29317001/diff/5732694713434112/skin/popup.css File skin/popup.css (right): http://codereview.adblockplus.org/29317001/diff/5732694713434112/skin/popup.c... skin/popup.css:119: background-image: url(background-main-hover.png); Apparently, the idea is simply to make the background slightly darker on hover. The current approach has issues: it adds one more background image that has to be kept in sync with the regular background image. Please consider the following solution: footer:hover { background: -moz-linear-gradient(top, rgba(0, 0, 0, 0.2), rgba(0, 0, 0, 0.2)), url(background-main.png); } Note that, unlike with a plain background color, a linear gradient can be displayed on top of the background image. Of course we would also need an unprefixed and a -webkit- prefixed version as well but this should work without an additional image.
http://codereview.adblockplus.org/29317001/diff/5732694713434112/skin/popup.css File skin/popup.css (right): http://codereview.adblockplus.org/29317001/diff/5732694713434112/skin/popup.c... skin/popup.css:119: background-image: url(background-main-hover.png); Just saw the screen cast, looks fine design-wise IMO. Regarding Wladimir's suggestion - why is this an image to begin with? Looks like a slightly transparent solid colour, rgba should do, wouldn't it?
http://codereview.adblockplus.org/29317001/diff/5732694713434112/skin/popup.css File skin/popup.css (right): http://codereview.adblockplus.org/29317001/diff/5732694713434112/skin/popup.c... skin/popup.css:119: background-image: url(background-main-hover.png); It isn't quite solid, it rather has a bit of structure in it (same as our homepage background). We used to implement that structure as a linear gradient but the performance of that solution wasn't great.
http://codereview.adblockplus.org/29317001/diff/5631943370604544/chrome/backg... File chrome/background.js (right): http://codereview.adblockplus.org/29317001/diff/5631943370604544/chrome/backg... chrome/background.js:230: } Can we agree on using Wladimir's suggestion? @Felix What's your opinion on this? http://codereview.adblockplus.org/29317001/diff/5732694713434112/skin/popup.css File skin/popup.css (right): http://codereview.adblockplus.org/29317001/diff/5732694713434112/skin/popup.c... skin/popup.css:119: background-image: url(background-main-hover.png); On 2013/12/11 11:20:26, Wladimir Palant wrote: > Apparently, the idea is simply to make the background slightly darker on hover. > The current approach has issues: it adds one more background image that has to > be kept in sync with the regular background image. Please consider the following > solution: > > footer:hover > { > background: -moz-linear-gradient(top, rgba(0, 0, 0, 0.2), rgba(0, 0, 0, 0.2)), > url(background-main.png); > } > > Note that, unlike with a plain background color, a linear gradient can be > displayed on top of the background image. Of course we would also need an > unprefixed and a -webkit- prefixed version as well but this should work without > an additional image. Done.
http://codereview.adblockplus.org/29317001/diff/5631943370604544/chrome/backg... File chrome/background.js (right): http://codereview.adblockplus.org/29317001/diff/5631943370604544/chrome/backg... chrome/background.js:230: } I think it's nicer API-wise to have a single function that can set various properties of the badge than to have one function per property. As long as it doesn't touch unspecified properties, but Wladimir's implementation doesn't, so I'm good.
Merged setBadgeNumber and setBadgeBackgroundColor http://codereview.adblockplus.org/29317001/diff/5631943370604544/chrome/backg... File chrome/background.js (right): http://codereview.adblockplus.org/29317001/diff/5631943370604544/chrome/backg... chrome/background.js:230: } On 2013/12/11 13:23:02, Felix H. Dahlke wrote: > I think it's nicer API-wise to have a single function that can set various > properties of the badge than to have one function per property. As long as it > doesn't touch unspecified properties, but Wladimir's implementation doesn't, so > I'm good. Done.
LGTM from my side, with the final nit below addressed. http://codereview.adblockplus.org/29317001/diff/5742506566221824/lib/stats.js File lib/stats.js (right): http://codereview.adblockplus.org/29317001/diff/5742506566221824/lib/stats.js... lib/stats.js:81: * @param {Function} func function to be executed s/ / / s'il vous plaît.
On 2013/12/03 14:48:08, sebastian wrote: > You have added icons/abp-19-whitelisted.png, but it is auto-generated. So please > remove it. This wasn't addressed yet.
On 2013/12/16 13:38:53, sebastian wrote: > On 2013/12/03 14:48:08, sebastian wrote: > > You have added icons/abp-19-whitelisted.png, but it is auto-generated. So > please > > remove it. > > This wasn't addressed yet. See http://codereview.adblockplus.org/29317001/#msg20 "Ignore the new icon images from now on as we decided to move those changes to a separate review."
On 2013/12/16 14:33:34, Thomas Greiner wrote: > On 2013/12/16 13:38:53, sebastian wrote: > > On 2013/12/03 14:48:08, sebastian wrote: > > > You have added icons/abp-19-whitelisted.png, but it is auto-generated. So > > please > > > remove it. > > > > This wasn't addressed yet. > > See http://codereview.adblockplus.org/29317001/#msg20 > "Ignore the new icon images from now on as we decided to move those changes to a > separate review." OK, but note that this will break the icon animation some kind of, in the meantime. And the whitelisted icon is overridden anyway. Beside that, LGTM.
LGTM with the issue below addressed. http://codereview.adblockplus.org/29317001/diff/5742506566221824/skin/popup.css File skin/popup.css (right): http://codereview.adblockplus.org/29317001/diff/5742506566221824/skin/popup.c... skin/popup.css:123: background: linear-gradient(top, rgba(70, 50, 0, 0.1), rgba(70, 50, 0, 0.1)), The standardized linear gradient syntax is actually different, see https://developer.mozilla.org/en-US/docs/Web/CSS/linear-gradient#Cross-browse.... Should be: background: linear-gradient(to bottom, ...) 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
