Issue 5294633391226880: issue 1435 - Port popup.html from Chrome/Safari/Opera to Firefox (Closed)
|
Created:
Sept. 26, 2014, 12:48 p.m. by saroyanm Modified:
Jan. 12, 2017, 3:23 p.m. Visibility:
Public. |
DescriptionThis review is related to current ticket:
https://issues.adblockplus.org/ticket/1435
Patch Set 1 #
Total comments: 47
Patch Set 2 : #
Total comments: 20
Patch Set 3 : #
Total comments: 17
Patch Set 4 : #
Total comments: 7
Patch Set 5 : #
Total comments: 10
Patch Set 6 : #
Total comments: 33
Patch Set 7 : #
Total comments: 1
MessagesTotal messages: 17
Sorry guys, for the delay, took little while splitting the patch. Please have a look when you have time.
http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... File chrome/content/ui/ext/common.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/content/ui/ext/common.js:20: var UI = require("ui").UI; We can make use of some ES6 features in this and other files (e.g.`let {UI} = require("ui");`) http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/content/ui/ext/common.js:24: function Tab(tab) // {id, url} This is not a proper documentation format (for examples see lib/filterClasses.js). http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/content/ui/ext/common.js:39: responseCallback(); // TODO Remove the TODOs here if nothing's missing. http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/content/ui/ext/common.js:45: // TODO Remove the TODOs here if nothing's missing. http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... File chrome/content/ui/ext/popup.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/content/ui/ext/popup.js:31: var mayResize = true; Is this variable ever set to false? http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/content/ui/ext/popup.js:40: iframe.parentNode.sizeTo(document.body.scrollWidth, document.body.offsetHeight + 11); Why `+ 11`? http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/content/ui/ext/popup.js:69: } What about the "else" case? Please look at the current implementation which toggles the bottom bar and changes the button text. http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/content/ui/ext/popup.js:73: return !UI.isBottombarOpen(topWindow); This function should no longer be necessary if you implement the above mentioned behavior. http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... File chrome/content/ui/popup.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/content/ui/popup.js:32: if (!/^https?:\/\//.exec(tab.url)) You can replace both the check and the class assignment with the following: `document.body.classList.toggle("local", !/^https?:\/\//.test(tab.url);` http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/content/ui/popup.js:54: var menuItem1 = document.getElementById("clickhide"); A bit more descriptive variable names would be nice (e.g. menuItem1 -> menuItemActivate, menuItem2 -> menuItemCancel). http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/content/ui/popup.js:64: // Otherwise, we are in default state. The description doesn't need to mention what's going on in the background page so reducing it to the first two sentences should be sufficient. http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/content/ui/popup.js:93: if (!ext.showBlockable()) In regard to my suggestion to get rid of showBlockable, change this to `if (!("openBlockable" in ext))` http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... File chrome/locale/en-US/popup.properties (right): http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/locale/en-US/popup.properties:1: blockable_items=Open blockable items Some of these strings already exist in overlay.dtd. Make sure that they are no longer needed and then remove them from there. http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... File chrome/skin/popup.css (right): http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/skin/popup.css:81: background: -moz-linear-gradient(top, rgba(70, 50, 0, 0.1), rgba(70, 50, 0, 0.1)), Seems like we don't need to support this syntax anymore since it has been dropped in Firefox 10 and Chrome 26 (see https://developer.mozilla.org/en-US/docs/Web/CSS/linear-gradient#Browser_comp...) http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/lib/... File lib/ui.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/lib/... lib/ui.js:1295: { What about adding the following line to avoid accessing match[1] and match[2]: `let [, prefix, name] = match;`
Thanks Thomas for your comments, I'm going to investigate scrollHeight (+11) further and hope to come with more details on it, also please have a look on updates and comment concerning openBlockable part when you find some time. http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... File chrome/content/ui/ext/common.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/content/ui/ext/common.js:20: var UI = require("ui").UI; On 2014/09/29 16:19:49, Thomas Greiner wrote: > We can make use of some ES6 features in this and other files (e.g.`let {UI} = > require("ui");`) updated scripts imported to popup.html except the one which also should be used in Chrome/Safari/Opera: chrome/content/ui/popup.js Should we think about converting variables during build ? In that case we can also use let in backport files. http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/content/ui/ext/common.js:24: function Tab(tab) // {id, url} On 2014/09/29 16:19:49, Thomas Greiner wrote: > This is not a proper documentation format (for examples see > lib/filterClasses.js). Done. http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/content/ui/ext/common.js:39: responseCallback(); // TODO On 2014/09/29 16:19:49, Thomas Greiner wrote: > Remove the TODOs here if nothing's missing. Done. http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/content/ui/ext/common.js:45: // TODO On 2014/09/29 16:19:49, Thomas Greiner wrote: > Remove the TODOs here if nothing's missing. Done. http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... File chrome/content/ui/ext/popup.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/content/ui/ext/popup.js:31: var mayResize = true; On 2014/09/29 16:19:49, Thomas Greiner wrote: > Is this variable ever set to false? Good point, removed. http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/content/ui/ext/popup.js:40: iframe.parentNode.sizeTo(document.body.scrollWidth, document.body.offsetHeight + 11); On 2014/09/29 16:19:49, Thomas Greiner wrote: > Why `+ 11`? I guess here I should use scrollHeight instead of offsetHeight, but in both cases the height is cutting down on arround 11 pixel, It fills like a size of scroller, so in this case it's looks to be a bug while the height of scrollbar also should be included in offsetHeight, I guess I need more investigations here. http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/content/ui/ext/popup.js:69: } On 2014/09/29 16:19:49, Thomas Greiner wrote: > What about the "else" case? Please look at the current implementation which > toggles the bottom bar and changes the button text. I guess if you can clarify on the comment in content/ui/popup.js I will understand the idea. http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/content/ui/ext/popup.js:73: return !UI.isBottombarOpen(topWindow); On 2014/09/29 16:19:49, Thomas Greiner wrote: > This function should no longer be necessary if you implement the above mentioned > behavior. Same as comment above. http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... File chrome/content/ui/popup.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/content/ui/popup.js:32: if (!/^https?:\/\//.exec(tab.url)) On 2014/09/29 16:19:49, Thomas Greiner wrote: > You can replace both the check and the class assignment with the following: > > `document.body.classList.toggle("local", !/^https?:\/\//.test(tab.url);` Awesome! Didn't know about second attribute. http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/content/ui/popup.js:54: var menuItem1 = document.getElementById("clickhide"); On 2014/09/29 16:19:49, Thomas Greiner wrote: > A bit more descriptive variable names would be nice (e.g. menuItem1 -> > menuItemActivate, menuItem2 -> menuItemCancel). Done. http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/content/ui/popup.js:64: // Otherwise, we are in default state. On 2014/09/29 16:19:49, Thomas Greiner wrote: > The description doesn't need to mention what's going on in the background page > so reducing it to the first two sentences should be sufficient. Done. http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/content/ui/popup.js:93: if (!ext.showBlockable()) On 2014/09/29 16:19:49, Thomas Greiner wrote: > In regard to my suggestion to get rid of showBlockable, change this to > > `if (!("openBlockable" in ext))` I guess I'm not catching on this part. Can you please clarify why we should look for "openBlockable" parameter in ext ? In case we would like to have similar implementation as in ui.js, then maybe we should have something like: var bottombarOpen = UI.isBottombarOpen(topWindow); if (bottombarOpen) return; var menuItem = document.getElementById("blockable"); menuItem.addEventListener("click", function() { UI.toggleBottombar(topWindow); ext.closePopup(); }, false); menuItem.removeAttribute("hidden"); But I think current implementation is more readable while we are importing UI module in ext/popup.js. http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... File chrome/locale/en-US/popup.properties (right): http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/locale/en-US/popup.properties:1: blockable_items=Open blockable items On 2014/09/29 16:19:49, Thomas Greiner wrote: > Some of these strings already exist in overlay.dtd. Make sure that they are no > longer needed and then remove them from there. That strings are used to create ABP menu item in Tools menu. Maybe we can think about using property file and stringbundles for XUL localization, but I think that should be separate issue. http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... File chrome/skin/popup.css (right): http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/skin/popup.css:81: background: -moz-linear-gradient(top, rgba(70, 50, 0, 0.1), rgba(70, 50, 0, 0.1)), On 2014/09/29 16:19:49, Thomas Greiner wrote: > Seems like we don't need to support this syntax anymore since it has been > dropped in Firefox 10 and Chrome 26 (see > https://developer.mozilla.org/en-US/docs/Web/CSS/linear-gradient#Browser_comp...) Done. http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/lib/... File lib/ui.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/lib/... lib/ui.js:1295: { On 2014/09/29 16:19:49, Thomas Greiner wrote: > What about adding the following line to avoid accessing match[1] and match[2]: > > `let [, prefix, name] = match;` I like it. Done.
http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... File chrome/content/ui/ext/popup.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/content/ui/ext/popup.js:40: iframe.parentNode.sizeTo(document.body.scrollWidth, document.body.offsetHeight + 11); On 2014/10/02 07:53:56, saroyanm wrote: > On 2014/09/29 16:19:49, Thomas Greiner wrote: > > Why `+ 11`? > > I guess here I should use scrollHeight instead of offsetHeight, but in both > cases the height is cutting down on arround 11 pixel, It fills like a size of > scroller, so in this case it's looks to be a bug while the height of scrollbar > also should be included in offsetHeight, I guess I need more investigations > here. Thomas, so here are the results of investigation: panel.sizeTo method (https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/panel#m-sizeTo) change the size of panel, which include also the arrow and panel container, so in case we are changing the size of panel of type="arrow" using above method, then we also should calculate current sizes: 1. arrow icon size -> http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/pop... in linux it's 9px 2. panel content style size -> http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/pop... for #2 we are overwriting the padding rule setting to 0 in overlay.css but we still have border of 1px. So 11px is "arrow icon" size + "panel content" border = 11px, in Linux. I'm not sure if it's a bug in Firefox, or this is how sizeTo method should work.
http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... File chrome/content/ui/ext/common.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/content/ui/ext/common.js:20: var UI = require("ui").UI; On 2014/10/02 07:53:56, saroyanm wrote: > On 2014/09/29 16:19:49, Thomas Greiner wrote: > > We can make use of some ES6 features in this and other files (e.g.`let {UI} = > > require("ui");`) > > updated scripts imported to popup.html except the one which also should be used > in Chrome/Safari/Opera: chrome/content/ui/popup.js > > Should we think about converting variables during build ? > In that case we can also use let in backport files. We haven't done this in other files which are used in Platform (e.g. firstRun.js). Actually, I think the reason was due to the type="application/javascript;version=1.7" attribute that is required for that. So using ES6 features in ext-files was a bad advice from me then. Sorry about that. http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/content/ui/ext/common.js:146: return "Missing translation: " + key; We should at least return null here. http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... File chrome/content/ui/ext/popup.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/content/ui/ext/popup.js:30: // event listeners to do so. It's true that we're not using event listeners for that but we are using MutationObserver. http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/content/ui/ext/popup.js:40: iframe.parentNode.sizeTo(document.body.scrollWidth, document.body.offsetHeight + 11); On 2014/10/07 13:02:05, saroyanm wrote: > On 2014/10/02 07:53:56, saroyanm wrote: > > On 2014/09/29 16:19:49, Thomas Greiner wrote: > > > Why `+ 11`? > > > > I guess here I should use scrollHeight instead of offsetHeight, but in both > > cases the height is cutting down on arround 11 pixel, It fills like a size of > > scroller, so in this case it's looks to be a bug while the height of scrollbar > > also should be included in offsetHeight, I guess I need more investigations > > here. > > Thomas, so here are the results of investigation: > panel.sizeTo method > (https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/panel#m-sizeTo) > change the size of panel, which include also the arrow and panel container, so > in case we are changing the size of panel of type="arrow" using above method, > then we also should calculate current sizes: > 1. arrow icon size -> > http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/pop... > in linux it's 9px > 2. panel content style size -> > http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/pop... > > for #2 we are overwriting the padding rule setting to 0 in overlay.css but we > still have border of 1px. So 11px is "arrow icon" size + "panel content" border > = 11px, in Linux. Couldn't we check for those values on runtime? Those values might differ depending on the platform Firefox is running on. > I'm not sure if it's a bug in Firefox, or this is how sizeTo method should work. It's not specified so it might be. Better check for existing issue reports on that or further documentation. http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... File chrome/content/ui/popup.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/content/ui/popup.js:93: if (!ext.showBlockable()) On 2014/10/02 07:53:56, saroyanm wrote: > On 2014/09/29 16:19:49, Thomas Greiner wrote: > > In regard to my suggestion to get rid of showBlockable, change this to > > > > `if (!("openBlockable" in ext))` > > I guess I'm not catching on this part. > Can you please clarify why we should look for "openBlockable" parameter in ext ? This menu item should always be shown on platforms that provide ext.openBlockable. Therefore we only need to check for the existence of that function. In its current state this code hides that menu item when the bottom panel is shown. However, this is inconsistent with the implementation in current Adblock Plus versions which always show that menu items. http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... File chrome/locale/en-US/popup.properties (right): http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/locale/en-US/popup.properties:1: blockable_items=Open blockable items On 2014/10/02 07:53:56, saroyanm wrote: > On 2014/09/29 16:19:49, Thomas Greiner wrote: > > Some of these strings already exist in overlay.dtd. Make sure that they are no > > longer needed and then remove them from there. > > That strings are used to create ABP menu item in Tools menu. > Maybe we can think about using property file and stringbundles for XUL > localization, but I think that should be separate issue. If they are unique to the popup you can still leave them in here. Apart from that please check whether we can even use texts from overlay.dtd in the popup. http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... File chrome/skin/popup.css (right): http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/skin/popup.css:81: background: -moz-linear-gradient(top, rgba(70, 50, 0, 0.1), rgba(70, 50, 0, 0.1)), On 2014/10/02 07:53:56, saroyanm wrote: > On 2014/09/29 16:19:49, Thomas Greiner wrote: > > Seems like we don't need to support this syntax anymore since it has been > > dropped in Firefox 10 and Chrome 26 (see > > > https://developer.mozilla.org/en-US/docs/Web/CSS/linear-gradient#Browser_comp...) > > Done. "and Chrome 26" http://codereview.adblockplus.org/5294633391226880/diff/5130467284090880/chro... File chrome/content/ui/ext/common.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5130467284090880/chro... chrome/content/ui/ext/common.js:1: /* I assume you implemented this before we changed the ext.* API to use pages instead of tabs. To ensure that interfaces match up when backporting those changes, adapt the code accordingly. http://codereview.adblockplus.org/5294633391226880/diff/5130467284090880/chro... chrome/content/ui/ext/common.js:19: * @fileOverview implementation of platform specific general classes and API Nit: Start with a capital letter to make it more consistent. http://codereview.adblockplus.org/5294633391226880/diff/5130467284090880/chro... chrome/content/ui/ext/common.js:19: * @fileOverview implementation of platform specific general classes and API What about "Implemenation of Firefox-specific general classes"? http://codereview.adblockplus.org/5294633391226880/diff/5130467284090880/chro... chrome/content/ui/ext/common.js:28: * @param {Object} tab an object with url parameter Nit: It's a property and not a parameter. http://codereview.adblockplus.org/5294633391226880/diff/5130467284090880/chro... chrome/content/ui/ext/common.js:60: * Pass Tab object of current active Window to callback function. The text should describe the purpose of the function and not the exact inner workings so my suggestion would be "Get currently active tab". You can mention that it's passed to the callback function in the parameter description below. http://codereview.adblockplus.org/5294633391226880/diff/5130467284090880/chro... chrome/content/ui/ext/common.js:61: * @param {function} callback Nit: Function should start with a capital letter. http://codereview.adblockplus.org/5294633391226880/diff/5130467284090880/chro... chrome/content/ui/ext/common.js:113: * get locale string Nit: Start with a capital letter. http://codereview.adblockplus.org/5294633391226880/diff/5130467284090880/chro... chrome/content/ui/ext/common.js:116: */ Please document only where it's appropriate. Apart from that it's missing the return value. http://codereview.adblockplus.org/5294633391226880/diff/5130467284090880/chro... chrome/content/ui/ext/common.js:133: * @param {function} callback Nit: Function and description should start with a capital letter. http://codereview.adblockplus.org/5294633391226880/diff/5130467284090880/chro... File chrome/content/ui/popup.html (right): http://codereview.adblockplus.org/5294633391226880/diff/5130467284090880/chro... chrome/content/ui/popup.html:23: <script type="application/javascript;version=1.7" src="utils.js"></script> ???
@Thomas thanks for comments, looks more is comming :) Please have a look when you find time. http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... File chrome/content/ui/ext/common.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/content/ui/ext/common.js:20: var UI = require("ui").UI; On 2014/10/08 10:40:43, Thomas Greiner wrote: > On 2014/10/02 07:53:56, saroyanm wrote: > > On 2014/09/29 16:19:49, Thomas Greiner wrote: > > > We can make use of some ES6 features in this and other files (e.g.`let {UI} > = > > > require("ui");`) > > > > updated scripts imported to popup.html except the one which also should be > used > > in Chrome/Safari/Opera: chrome/content/ui/popup.js > > > > Should we think about converting variables during build ? > > In that case we can also use let in backport files. > > We haven't done this in other files which are used in Platform (e.g. > firstRun.js). Actually, I think the reason was due to the > type="application/javascript;version=1.7" attribute that is required for that. > So using ES6 features in ext-files was a bad advice from me then. Sorry about > that. np, I should also wrote that down, reverted. http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/content/ui/ext/common.js:146: return "Missing translation: " + key; On 2014/10/08 10:40:43, Thomas Greiner wrote: > We should at least return null here. Done. http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... File chrome/content/ui/ext/popup.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/content/ui/ext/popup.js:30: // event listeners to do so. On 2014/10/08 10:40:43, Thomas Greiner wrote: > It's true that we're not using event listeners for that but we are using > MutationObserver. Done. http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/content/ui/ext/popup.js:40: iframe.parentNode.sizeTo(document.body.scrollWidth, document.body.offsetHeight + 11); > Couldn't we check for those values on runtime? Those values might differ > depending on the platform Firefox is running on. Yes, looks like we can, while iframe is stretching to the content of panel, so we can calculate difference of iframe height and panel's height. > It's not specified so it might be. Better check for existing issue reports on > that or further documentation. There is no existing issue, but also in documentation it's not mentioned that the method should resize the content part of panel, but the panel itself. http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... File chrome/content/ui/popup.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/content/ui/popup.js:93: if (!ext.showBlockable()) On 2014/10/08 10:40:43, Thomas Greiner wrote: > On 2014/10/02 07:53:56, saroyanm wrote: > > On 2014/09/29 16:19:49, Thomas Greiner wrote: > > > In regard to my suggestion to get rid of showBlockable, change this to > > > > > > `if (!("openBlockable" in ext))` > > > > I guess I'm not catching on this part. > > Can you please clarify why we should look for "openBlockable" parameter in ext > ? > > This menu item should always be shown on platforms that provide > ext.openBlockable. Therefore we only need to check for the existence of that > function. > In its current state this code hides that menu item when the bottom panel is > shown. However, this is inconsistent with the implementation in current Adblock > Plus versions which always show that menu items. > Got it, please let me know if you think we can give a go with new implementation. http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... File chrome/locale/en-US/popup.properties (right): http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/locale/en-US/popup.properties:1: blockable_items=Open blockable items > If they are unique to the popup you can still leave them in here. Apart from > that please check whether we can even use texts from overlay.dtd in the popup. popup only uses stringbundles from "popup.property" file, it has no access to overlay.dtd. The entities cannot be used in the script and seems like we can't use text from overlay.dtd in the popup, what I think we can do here is to use stringbundles for xul localization, but it's not what suggested in mozilla documentation, so the entities purpose is for localization of xul elements, but for localization of HTML pages we should use stringbundles from property files. Maybe @Wladimir can give good suggestion here. http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... File chrome/skin/popup.css (right): http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/skin/popup.css:81: background: -moz-linear-gradient(top, rgba(70, 50, 0, 0.1), rgba(70, 50, 0, 0.1)), On 2014/10/08 10:40:43, Thomas Greiner wrote: > On 2014/10/02 07:53:56, saroyanm wrote: > > On 2014/09/29 16:19:49, Thomas Greiner wrote: > > > Seems like we don't need to support this syntax anymore since it has been > > > dropped in Firefox 10 and Chrome 26 (see > > > > > > https://developer.mozilla.org/en-US/docs/Web/CSS/linear-gradient#Browser_comp...) > > > > Done. > > "and Chrome 26" Done. http://codereview.adblockplus.org/5294633391226880/diff/5130467284090880/chro... File chrome/content/ui/ext/common.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5130467284090880/chro... chrome/content/ui/ext/common.js:1: /* On 2014/10/08 10:40:43, Thomas Greiner wrote: > I assume you implemented this before we changed the ext.* API to use pages > instead of tabs. To ensure that interfaces match up when backporting those > changes, adapt the code accordingly. Adapted, but have a little doubt. http://codereview.adblockplus.org/5294633391226880/diff/5130467284090880/chro... chrome/content/ui/ext/common.js:19: * @fileOverview implementation of platform specific general classes and API On 2014/10/08 10:40:43, Thomas Greiner wrote: > Nit: Start with a capital letter to make it more consistent. Done. http://codereview.adblockplus.org/5294633391226880/diff/5130467284090880/chro... chrome/content/ui/ext/common.js:19: * @fileOverview implementation of platform specific general classes and API On 2014/10/08 10:40:43, Thomas Greiner wrote: > What about "Implemenation of Firefox-specific general classes"? Sounds better. http://codereview.adblockplus.org/5294633391226880/diff/5130467284090880/chro... chrome/content/ui/ext/common.js:28: * @param {Object} tab an object with url parameter On 2014/10/08 10:40:43, Thomas Greiner wrote: > Nit: It's a property and not a parameter. oops. http://codereview.adblockplus.org/5294633391226880/diff/5130467284090880/chro... chrome/content/ui/ext/common.js:113: * get locale string On 2014/10/08 10:40:43, Thomas Greiner wrote: > Nit: Start with a capital letter. Done. http://codereview.adblockplus.org/5294633391226880/diff/5130467284090880/chro... chrome/content/ui/ext/common.js:116: */ On 2014/10/08 10:40:43, Thomas Greiner wrote: > Please document only where it's appropriate. Apart from that it's missing the > return value. Done. http://codereview.adblockplus.org/5294633391226880/diff/5130467284090880/chro... File chrome/content/ui/popup.html (right): http://codereview.adblockplus.org/5294633391226880/diff/5130467284090880/chro... chrome/content/ui/popup.html:23: <script type="application/javascript;version=1.7" src="utils.js"></script> On 2014/10/08 10:40:43, Thomas Greiner wrote: > ??? utils.js are used to provide access to the ABP modules, using require() function. I understand the doubt about the script import in popup.html, so I see the only possibility to implement the require function in ext/common.js, should We go for that ? http://codereview.adblockplus.org/5294633391226880/diff/5764640680181760/chro... File chrome/content/ui/ext/common.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5764640680181760/chro... chrome/content/ui/ext/common.js:111: if (info.active && info.lastFocusedWindow) Not sure about array that should be returned with callback in else case, what you think can we leave as it is now ?
http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... File chrome/content/ui/popup.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/content/ui/popup.js:93: if (!ext.showBlockable()) On 2014/10/10 12:05:58, saroyanm wrote: > On 2014/10/08 10:40:43, Thomas Greiner wrote: > > On 2014/10/02 07:53:56, saroyanm wrote: > > > On 2014/09/29 16:19:49, Thomas Greiner wrote: > > > > In regard to my suggestion to get rid of showBlockable, change this to > > > > > > > > `if (!("openBlockable" in ext))` > > > > > > I guess I'm not catching on this part. > > > Can you please clarify why we should look for "openBlockable" parameter in > ext > > ? > > > > This menu item should always be shown on platforms that provide > > ext.openBlockable. Therefore we only need to check for the existence of that > > function. > > In its current state this code hides that menu item when the bottom panel is > > shown. However, this is inconsistent with the implementation in current > Adblock > > Plus versions which always show that menu items. > > > > Got it, please let me know if you think we can give a go with new > implementation. The new implementation looks good. http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... File chrome/locale/en-US/popup.properties (right): http://codereview.adblockplus.org/5294633391226880/diff/5629499534213120/chro... chrome/locale/en-US/popup.properties:1: blockable_items=Open blockable items On 2014/10/10 12:05:58, saroyanm wrote: > > If they are unique to the popup you can still leave them in here. Apart from > > that please check whether we can even use texts from overlay.dtd in the popup. > > popup only uses stringbundles from "popup.property" file, it has no access to > overlay.dtd. > The entities cannot be used in the script and seems like we can't use text from > overlay.dtd in the popup, what I think we can do here is to use stringbundles > for xul localization, but it's not what suggested in mozilla documentation, so > the entities purpose is for localization of xul elements, but for localization > of HTML pages we should use stringbundles from property files. Maybe @Wladimir > can give good suggestion here. Ok, then let's keep them separated. http://codereview.adblockplus.org/5294633391226880/diff/5130467284090880/chro... File chrome/content/ui/ext/common.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5130467284090880/chro... chrome/content/ui/ext/common.js:1: /* On 2014/10/10 12:05:58, saroyanm wrote: > On 2014/10/08 10:40:43, Thomas Greiner wrote: > > I assume you implemented this before we changed the ext.* API to use pages > > instead of tabs. To ensure that interfaces match up when backporting those > > changes, adapt the code accordingly. > > Adapted, but have a little doubt. Why is that? http://codereview.adblockplus.org/5294633391226880/diff/5130467284090880/chro... File chrome/content/ui/ext/popup.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5130467284090880/chro... chrome/content/ui/ext/popup.js:61: openBlockable: function() This function is no longer used to only open the dialog but to toggle it which should be reflected in the name (e.g. "toggleBlockable"). http://codereview.adblockplus.org/5294633391226880/diff/5130467284090880/chro... File chrome/content/ui/popup.html (right): http://codereview.adblockplus.org/5294633391226880/diff/5130467284090880/chro... chrome/content/ui/popup.html:23: <script type="application/javascript;version=1.7" src="utils.js"></script> On 2014/10/10 12:05:58, saroyanm wrote: > On 2014/10/08 10:40:43, Thomas Greiner wrote: > > ??? > > utils.js are used to provide access to the ABP modules, using require() > function. > I understand the doubt about the script import in popup.html, so I see the only > possibility to implement the require function in ext/common.js, should We go for > that ? You can ignore this comment, it got resolved in the discussion about using ES6 features. http://codereview.adblockplus.org/5294633391226880/diff/5764640680181760/chro... File chrome/content/ui/ext/common.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5764640680181760/chro... chrome/content/ui/ext/common.js:111: if (info.active && info.lastFocusedWindow) On 2014/10/10 12:05:58, saroyanm wrote: > Not sure about array that should be returned with callback in else case, what > you think can we leave as it is now ? We do need to call the callback function even if there are no pages matching the query. Therefore the else case should be `callback([])` http://codereview.adblockplus.org/5294633391226880/diff/5764640680181760/chro... chrome/content/ui/ext/common.js:115: url: location.spec This will throw an error if location is `null`. http://codereview.adblockplus.org/5294633391226880/diff/5764640680181760/chro... File chrome/content/ui/ext/popup.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5764640680181760/chro... chrome/content/ui/ext/popup.js:39: // We need to calculate height difference while panel include What do you mean with "while panel include"? Please also provide a short explanation why we need to have this. http://codereview.adblockplus.org/5294633391226880/diff/5764640680181760/chro... File chrome/content/ui/popup.html (right): http://codereview.adblockplus.org/5294633391226880/diff/5764640680181760/chro... chrome/content/ui/popup.html:55: <li id="blockable-close" class="menu-item" role="button" hidden> No need to duplicate that menu item. The appropriate text can be shown using CSS. We just need to add the text to the existing item above: <li id="blockable" class="menu-item" role="button" hidden> <div class="icon"></div> <span class="i18n_open_blockable_items"></span> <span class="i18n_close_blockable_items"></span> </li> http://codereview.adblockplus.org/5294633391226880/diff/5764640680181760/chro... File chrome/content/ui/popup.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5764640680181760/chro... chrome/content/ui/popup.js:1: /* Keep in mind that at some point we need to merge this version with the one in adblockpluschrome. I'll leave it up to you whether you prefer rebasing the changes now or as part of the backport. http://codereview.adblockplus.org/5294633391226880/diff/5764640680181760/chro... chrome/content/ui/popup.js:20: var page = null; I assume that in one of the later reviews this variable needs to be global but in this one it doesn't. http://codereview.adblockplus.org/5294633391226880/diff/5764640680181760/chro... chrome/content/ui/popup.js:28: document.body.classList.toggle("local", !/^https?:\/\//.test(page.url)); This will throw an error if the pages array is empty.
I guess we are near. http://codereview.adblockplus.org/5294633391226880/diff/5764640680181760/chro... File chrome/content/ui/ext/common.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5764640680181760/chro... chrome/content/ui/ext/common.js:111: if (info.active && info.lastFocusedWindow) On 2014/10/13 13:18:24, Thomas Greiner wrote: > On 2014/10/10 12:05:58, saroyanm wrote: > > Not sure about array that should be returned with callback in else case, what > > you think can we leave as it is now ? > > We do need to call the callback function even if there are no pages matching the > query. Therefore the else case should be `callback([])` Done. http://codereview.adblockplus.org/5294633391226880/diff/5764640680181760/chro... chrome/content/ui/ext/common.js:115: url: location.spec On 2014/10/13 13:18:24, Thomas Greiner wrote: > This will throw an error if location is `null`. Done. http://codereview.adblockplus.org/5294633391226880/diff/5764640680181760/chro... File chrome/content/ui/ext/popup.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5764640680181760/chro... chrome/content/ui/ext/popup.js:39: // We need to calculate height difference while panel include On 2014/10/13 13:18:24, Thomas Greiner wrote: > What do you mean with "while panel include"? Please also provide a short > explanation why we need to have this. Done. http://codereview.adblockplus.org/5294633391226880/diff/5764640680181760/chro... File chrome/content/ui/popup.html (right): http://codereview.adblockplus.org/5294633391226880/diff/5764640680181760/chro... chrome/content/ui/popup.html:55: <li id="blockable-close" class="menu-item" role="button" hidden> On 2014/10/13 13:18:24, Thomas Greiner wrote: > No need to duplicate that menu item. The appropriate text can be shown using > CSS. We just need to add the text to the existing item above: > > <li id="blockable" class="menu-item" role="button" hidden> > <div class="icon"></div> > <span class="i18n_open_blockable_items"></span> > <span class="i18n_close_blockable_items"></span> > </li> Done. http://codereview.adblockplus.org/5294633391226880/diff/5764640680181760/chro... File chrome/content/ui/popup.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5764640680181760/chro... chrome/content/ui/popup.js:1: /* On 2014/10/13 13:18:24, Thomas Greiner wrote: > Keep in mind that at some point we need to merge this version with the one in > adblockpluschrome. I'll leave it up to you whether you prefer rebasing the > changes now or as part of the backport. I guess would be nice to do it during backport, or after all #181 related tickets are covered, because popup.js and popup.html will again be changed by #181 related subtickets. http://codereview.adblockplus.org/5294633391226880/diff/5764640680181760/chro... chrome/content/ui/popup.js:20: var page = null; On 2014/10/13 13:18:24, Thomas Greiner wrote: > I assume that in one of the later reviews this variable needs to be global but > in this one it doesn't. I tried to keep it like in Chrome/safari/opera version, to make less confusion by keeping in mind backport and yes that variable should be global after #1436 ticket. Let me know if it's preferable to change the implementation for current review and make it global in #1436 ticket review. http://codereview.adblockplus.org/5294633391226880/diff/5764640680181760/chro... chrome/content/ui/popup.js:28: document.body.classList.toggle("local", !/^https?:\/\//.test(page.url)); On 2014/10/13 13:18:24, Thomas Greiner wrote: > This will throw an error if the pages array is empty. Done. http://codereview.adblockplus.org/5294633391226880/diff/5150194068881408/chro... File chrome/skin/popup.css (right): http://codereview.adblockplus.org/5294633391226880/diff/5150194068881408/chro... chrome/skin/popup.css:167: #blockable .i18n_close_blockable_items, What you think, can we use i18n related class names here in css file, or better assign IDs to <span> tags ?
Only some minor changes remaining. http://codereview.adblockplus.org/5294633391226880/diff/5764640680181760/chro... File chrome/content/ui/popup.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5764640680181760/chro... chrome/content/ui/popup.js:1: /* On 2014/10/16 11:26:15, saroyanm wrote: > On 2014/10/13 13:18:24, Thomas Greiner wrote: > > Keep in mind that at some point we need to merge this version with the one in > > adblockpluschrome. I'll leave it up to you whether you prefer rebasing the > > changes now or as part of the backport. > > I guess would be nice to do it during backport, or after all #181 related > tickets are covered, because popup.js and popup.html will again be changed by > #181 related subtickets. That's fine with me. http://codereview.adblockplus.org/5294633391226880/diff/5764640680181760/chro... chrome/content/ui/popup.js:20: var page = null; On 2014/10/16 11:26:15, saroyanm wrote: > On 2014/10/13 13:18:24, Thomas Greiner wrote: > > I assume that in one of the later reviews this variable needs to be global but > > in this one it doesn't. > > I tried to keep it like in Chrome/safari/opera version, to make less confusion > by keeping in mind backport and yes that variable should be global after #1436 > ticket. > Let me know if it's preferable to change the implementation for current review > and make it global in #1436 ticket review. No, that's not necessary then. http://codereview.adblockplus.org/5294633391226880/diff/5150194068881408/chro... File chrome/content/ui/popup.html (right): http://codereview.adblockplus.org/5294633391226880/diff/5150194068881408/chro... chrome/content/ui/popup.html:52: <span class="i18n_blockable_items"></span> Nit: As included in my previous comment, please use "i18n_open_blockable_items". http://codereview.adblockplus.org/5294633391226880/diff/5150194068881408/chro... File chrome/skin/popup.css (right): http://codereview.adblockplus.org/5294633391226880/diff/5150194068881408/chro... chrome/skin/popup.css:167: #blockable .i18n_close_blockable_items, On 2014/10/16 11:26:15, saroyanm wrote: > What you think, can we use i18n related class names here in css file, or better > assign IDs to <span> tags ? Good point, let's better keep them separated. One other option besides IDs is to use data-* attributes (e.g. <span data-action="open">, <span data-action="close">). http://codereview.adblockplus.org/5294633391226880/diff/5150194068881408/chro... chrome/skin/popup.css:168: #blockable.open .i18n_blockable_items By using the :not() pseudo-class you could merge those two rules into a single one like this: #blockable:not(.open) .i18n_blockable_items, #blockable.open .i18n_close_blockable_items { display: none; }
Sorry for delay Thomas, Updated. http://codereview.adblockplus.org/5294633391226880/diff/5150194068881408/chro... File chrome/content/ui/popup.html (right): http://codereview.adblockplus.org/5294633391226880/diff/5150194068881408/chro... chrome/content/ui/popup.html:52: <span class="i18n_blockable_items"></span> On 2014/10/16 13:39:44, Thomas Greiner wrote: > Nit: As included in my previous comment, please use "i18n_open_blockable_items". Done. http://codereview.adblockplus.org/5294633391226880/diff/5150194068881408/chro... File chrome/skin/popup.css (right): http://codereview.adblockplus.org/5294633391226880/diff/5150194068881408/chro... chrome/skin/popup.css:167: #blockable .i18n_close_blockable_items, On 2014/10/16 13:39:44, Thomas Greiner wrote: > On 2014/10/16 11:26:15, saroyanm wrote: > > What you think, can we use i18n related class names here in css file, or > better > > assign IDs to <span> tags ? > > Good point, let's better keep them separated. One other option besides IDs is to > use data-* attributes (e.g. <span data-action="open">, <span > data-action="close">). good point. http://codereview.adblockplus.org/5294633391226880/diff/5150194068881408/chro... chrome/skin/popup.css:168: #blockable.open .i18n_blockable_items On 2014/10/16 13:39:44, Thomas Greiner wrote: > By using the :not() pseudo-class you could merge those two rules into a single > one like this: > > #blockable:not(.open) .i18n_blockable_items, > #blockable.open .i18n_close_blockable_items > { > display: none; > } Should have implemented, great!
Went through the whole code again and found one oddity in lib/ui.js http://codereview.adblockplus.org/5294633391226880/diff/5647308616105984/chro... File chrome/content/ui/ext/popup.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5647308616105984/chro... chrome/content/ui/ext/popup.js:20: var {UI} = require("ui"); This must've been overlooked when reverting back from ES6 syntax. http://codereview.adblockplus.org/5294633391226880/diff/5647308616105984/chro... File chrome/content/ui/overlay.xul (right): http://codereview.adblockplus.org/5294633391226880/diff/5647308616105984/chro... chrome/content/ui/overlay.xul:53: <menu id="abp-menuitem" label="&toolbarbutton.label;"> You removed the comment "Tools menu". Please add it back above here. http://codereview.adblockplus.org/5294633391226880/diff/5647308616105984/chro... File chrome/content/ui/popup.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5647308616105984/chro... chrome/content/ui/popup.js:19: window["openOptions"] = backgroundPage["openOptions"]; I guess this is a leftover from the loop that was here before. Wouldn't you rather write this? var openOptions = backgroundPage.openOptions; http://codereview.adblockplus.org/5294633391226880/diff/5647308616105984/lib/... File lib/ui.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5647308616105984/lib/... lib/ui.js:381: if ("abp-status-popup" in this.overlay) This whole block appears to be have been overlooked. Keep in mind that the menu items are no longer found in "abp-status-popup". http://codereview.adblockplus.org/5294633391226880/diff/5647308616105984/lib/... lib/ui.js:1304: this.fillIconMenu(event, popup.ownerDocument.defaultView, match[1]); You're still using `match[1]` here instead of `prefix`.
On 2014/10/17 15:02:20, Thomas Greiner wrote: > Went through the whole code again and found one oddity in lib/ui.js Wish to had a chance finish review this week, but I'll be out of country on weekend, will have a closer look on Monday. Sorry for that Thomas.
@Thomas please have a look when you have time. http://codereview.adblockplus.org/5294633391226880/diff/5647308616105984/chro... File chrome/content/ui/overlay.xul (right): http://codereview.adblockplus.org/5294633391226880/diff/5647308616105984/chro... chrome/content/ui/overlay.xul:53: <menu id="abp-menuitem" label="&toolbarbutton.label;"> On 2014/10/17 15:02:21, Thomas Greiner wrote: > You removed the comment "Tools menu". Please add it back above here. Done. http://codereview.adblockplus.org/5294633391226880/diff/5647308616105984/chro... File chrome/content/ui/popup.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5647308616105984/chro... chrome/content/ui/popup.js:19: window["openOptions"] = backgroundPage["openOptions"]; On 2014/10/17 15:02:21, Thomas Greiner wrote: > I guess this is a leftover from the loop that was here before. Wouldn't you > rather write this? > > var openOptions = backgroundPage.openOptions; Done. http://codereview.adblockplus.org/5294633391226880/diff/5647308616105984/lib/... File lib/ui.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5647308616105984/lib/... lib/ui.js:381: if ("abp-status-popup" in this.overlay) On 2014/10/17 15:02:21, Thomas Greiner wrote: > This whole block appears to be have been overlooked. Keep in mind that the menu > items are no longer found in "abp-status-popup". Thomas I didn't left comment in this block as we discussed via IRC because I've double checked and this block will be changed with next tasks when we will implement badge functionality and change the way how toolbarbutton works now. So we will only add bubble UI panel to toolbarbutton in future. http://codereview.adblockplus.org/5294633391226880/diff/5647308616105984/lib/... lib/ui.js:1304: this.fillIconMenu(event, popup.ownerDocument.defaultView, match[1]); On 2014/10/17 15:02:21, Thomas Greiner wrote: > You're still using `match[1]` here instead of `prefix`. Oops.
LGTM I added some information to my destructuring assignment comment for others to understand why it hasn't been addressed. http://codereview.adblockplus.org/5294633391226880/diff/5647308616105984/chro... File chrome/content/ui/ext/popup.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5647308616105984/chro... chrome/content/ui/ext/popup.js:20: var {UI} = require("ui"); On 2014/10/17 15:02:21, Thomas Greiner wrote: > This must've been overlooked when reverting back from ES6 syntax. We agreed on IRC to ignore this comment because it turns out that destructuring assignments do not require `type="application/javascript;version=1.7"` to be specified on the script object (see https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/1.7)
On 2014/10/21 16:28:05, Thomas Greiner wrote: > I added some information to my destructuring assignment comment for others to > understand why it hasn't been addressed. Good point, thanks, @Wladimir while in this review we have major Bubble UI port implementation, would you also like to review the patch, before moving forward ?
http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... File chrome/content/ui/ext/common.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... chrome/content/ui/ext/common.js:70: } Why is this a closure returning the require() function? Shouldn't it be: require: require ? http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... chrome/content/ui/ext/common.js:79: ext = { This is an undeclared variable. Please do it like this: var ext = function() { ... return { ... }; }; http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... chrome/content/ui/ext/common.js:98: return (args ? stringBundle.formatStringFromName(key, args, args.length) : stringBundle.GetStringFromName(key)); Please don't use stringBundle.formatStringFromName(), we don't use that placeholder format - it doesn't allow changing order of placeholders in translated strings. If anything, we should use ?1? and ?2? for placeholders (that's what we normally use in Firefox). We will need Chrome/Opera/Safari changes however if any shared strings use these placeholders. http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... chrome/content/ui/ext/common.js:117: callback([new Page(tab)]); Is the Page class used anywhere else? If not, then the following should be sufficient: callback([{ url: location.spec, sendMessage: (message, responseCallback) => responseCallback() }]); No need for a tab object and a Page class. http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... File chrome/content/ui/ext/popup.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... chrome/content/ui/ext/popup.js:25: .chromeEventHandler; Nice trick, wasn't aware of that. http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... chrome/content/ui/ext/popup.js:26: var topWindow = iframe.ownerDocument.defaultView; Please add a variable |panel = iframe.parentNode| - currently it's unclear what this refers to. http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... chrome/content/ui/ext/popup.js:43: resizingScheduled = false; Please move that line to the top of the function - if any other code in this function throws an exception we shouldn't end up in an invalid state. http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... chrome/content/ui/ext/popup.js:59: ext = { Again, please don't assign to undeclared variables. http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... File chrome/content/ui/popup.html (right): http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... chrome/content/ui/popup.html:59: </li> The idea is that this file will be shared between Firefox and Chrome. So, where did (currently) Chrome-specific functionality go, like the hit counter? http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... File chrome/content/ui/popup.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... chrome/content/ui/popup.js:18: var backgroundPage = ext.backgroundPage.getWindow(); Can I just assume that this file has been moved from adblockpluschrome without any changes? http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... File chrome/locale/en-US/popup.properties (right): http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... chrome/locale/en-US/popup.properties:3: clickhide_instructions=After closing this popup, click (or right-click) an element on the page. Please add a comment above explaining that this is Chrome-specific functionality - otherwise the translators will be confused. http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... chrome/locale/en-US/popup.properties:5: easy_create_filter=Block element Same here, this needs a comment. http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... File chrome/skin/popup.css (right): http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... chrome/skin/popup.css:18: body Can I assume that this file has been copied from Chrome without any changes? http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/lib/... File lib/ui.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/lib/... lib/ui.js:389: this.overlay["abp-toolbarbutton"].appendChild(fixId(menuSource.cloneNode(true), "abp-toolbar")); Shouldn't this go away as well, along with the fixId() function? http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/lib/... lib/ui.js:1299: let window = popup.ownerDocument.defaultView; How about defining the window variable before the |if| block and using it for the |else| case as well? http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/lib/... lib/ui.js:1318: if (popup.id == "abp-toolbar-popup") What about abp-status-popup?
@Wladimir thanks for finding time and reviewing changes, will really appreciate if you can also comment under my last comments. After this review things will go more smoothly. http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... File chrome/content/ui/ext/common.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... chrome/content/ui/ext/common.js:70: } On 2014/10/23 21:57:40, Wladimir Palant wrote: > Why is this a closure returning the require() function? Shouldn't it be: > > require: require > > ? Done. http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... chrome/content/ui/ext/common.js:79: ext = { On 2014/10/23 21:57:40, Wladimir Palant wrote: > This is an undeclared variable. Please do it like this: > > var ext = function() > { > ... > return { > ... > }; > }; I've declared ext in Utils and left a comment there. http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... chrome/content/ui/ext/common.js:98: return (args ? stringBundle.formatStringFromName(key, args, args.length) : stringBundle.GetStringFromName(key)); On 2014/10/23 21:57:40, Wladimir Palant wrote: > Please don't use stringBundle.formatStringFromName(), we don't use that > placeholder format - it doesn't allow changing order of placeholders in > translated strings. If anything, we should use ?1? and ?2? for placeholders > (that's what we normally use in Firefox). We will need Chrome/Opera/Safari > changes however if any shared strings use these placeholders. Done, please let me know if I'm missing something. http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... chrome/content/ui/ext/common.js:117: callback([new Page(tab)]); On 2014/10/23 21:57:40, Wladimir Palant wrote: > Is the Page class used anywhere else? If not, then the following should be > sufficient: > > callback([{ > url: location.spec, > sendMessage: (message, responseCallback) => responseCallback() > }]); > > No need for a tab object and a Page class. Done. http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... File chrome/content/ui/ext/popup.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... chrome/content/ui/ext/popup.js:26: var topWindow = iframe.ownerDocument.defaultView; On 2014/10/23 21:57:40, Wladimir Palant wrote: > Please add a variable |panel = iframe.parentNode| - currently it's unclear what > this refers to. Done. http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... chrome/content/ui/ext/popup.js:43: resizingScheduled = false; On 2014/10/23 21:57:40, Wladimir Palant wrote: > Please move that line to the top of the function - if any other code in this > function throws an exception we shouldn't end up in an invalid state. Done. http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... chrome/content/ui/ext/popup.js:59: ext = { On 2014/10/23 21:57:40, Wladimir Palant wrote: > Again, please don't assign to undeclared variables. the ext from common.js should be accessible here I guess, I've declared ext in utils.js and left a comment there. http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... File chrome/content/ui/i18n.js (left): http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... chrome/content/ui/i18n.js:48: var placeholders = message.placeholders; @Wladimir Why did we used this placeholder ? Seems like it always was null ? http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... chrome/content/ui/i18n.js:67: text = text.split("$" + key + "$").join(replacement); @Wladimir why we used this approach ? http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... File chrome/content/ui/popup.html (right): http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... chrome/content/ui/popup.html:59: </li> On 2014/10/23 21:57:40, Wladimir Palant wrote: > The idea is that this file will be shared between Firefox and Chrome. So, where > did (currently) Chrome-specific functionality go, like the hit counter? Mentioned functionality will be added with relevant ticket, I was thinking not to overload this review. Finally this file will have only small changes that we will need to backport to Chrome. http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... File chrome/content/ui/popup.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... chrome/content/ui/popup.js:18: var backgroundPage = ext.backgroundPage.getWindow(); On 2014/10/23 21:57:40, Wladimir Palant wrote: > Can I just assume that this file has been moved from adblockpluschrome without > any changes? No, it's missing functionality that will be added by relevant tickets and also this contains functionality, like open blockable items which is not implemented in Chrome/Safari/Opera so the open blockable item block will be hidden there. http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... File chrome/locale/en-US/popup.properties (right): http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... chrome/locale/en-US/popup.properties:3: clickhide_instructions=After closing this popup, click (or right-click) an element on the page. On 2014/10/23 21:57:40, Wladimir Palant wrote: > Please add a comment above explaining that this is Chrome-specific functionality > - otherwise the translators will be confused. Done. http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... chrome/locale/en-US/popup.properties:5: easy_create_filter=Block element On 2014/10/23 21:57:40, Wladimir Palant wrote: > Same here, this needs a comment. Done. http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... File chrome/skin/popup.css (right): http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/chro... chrome/skin/popup.css:18: body On 2014/10/23 21:57:40, Wladimir Palant wrote: > Can I assume that this file has been copied from Chrome without any changes? Some rules are yet again missing which will be added during relevant ticket reviews and also added rule for open blockable item. http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/lib/... File lib/ui.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/lib/... lib/ui.js:389: this.overlay["abp-toolbarbutton"].appendChild(fixId(menuSource.cloneNode(true), "abp-toolbar")); On 2014/10/23 21:57:40, Wladimir Palant wrote: > Shouldn't this go away as well, along with the fixId() function? In current review we are using abp-status-popup for showing panel during right click when the default left click action is changed using customization addon and abp-toolbar-popup, but in future we will change that behavior and use openPopup method to show panel, both for left and right click, so with that review this part shouldn't make that much confusion. http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/lib/... lib/ui.js:1299: let window = popup.ownerDocument.defaultView; On 2014/10/23 21:57:40, Wladimir Palant wrote: > How about defining the window variable before the |if| block and using it for > the |else| case as well? Oops. http://codereview.adblockplus.org/5294633391226880/diff/5635415851663360/lib/... lib/ui.js:1318: if (popup.id == "abp-toolbar-popup") On 2014/10/23 21:57:40, Wladimir Palant wrote: > What about abp-status-popup? Yes this is wrong and also wrong while now we are using same id (abp-toolbar-popup-browser) in two places (in popupset and toolbarbutton), but this again will be changed with the review mentioned above. http://codereview.adblockplus.org/5294633391226880/diff/5769928858664960/chro... File chrome/content/ui/utils.js (right): http://codereview.adblockplus.org/5294633391226880/diff/5769928858664960/chro... chrome/content/ui/utils.js:50: var ext; @Wladimir I'm not sure if we should use require function to return ext as a module ? if (module == "ext") return ext; 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
