Issue 29536764: Issue 5587, 5748 - Use mobile options page on Firefox for Android (Closed)
|
Created:
Sept. 6, 2017, 1:05 a.m. by Manish Jethani Modified:
Oct. 9, 2017, 3:09 p.m. CC:
kzar Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
Patch Set 1 #
Total comments: 30
Patch Set 2 : Move browserAction handler to lib/browserAction.js #
Total comments: 6
Patch Set 3 : Add options page loader #
Total comments: 5
Patch Set 4 : Use iframe instead of redirection #Patch Set 5 : Move file names to HTML #Patch Set 6 : Pass boolean instead of object #
Total comments: 2
Patch Set 7 : Use runtime.openOptionsPage on Firefox 57+ #Patch Set 8 : Forward query string and fragment identifier to iframe #
Total comments: 9
Patch Set 9 : Query tabs on Firefox as well #
Total comments: 2
Patch Set 10 : Queue up messages from background page and forward them later #
Total comments: 19
Patch Set 11 : Update title, add defer attribute, coding style #
Total comments: 3
Patch Set 12 : Use ping messages to sync up #
Total comments: 13
Patch Set 13 : Rebase on next #
Total comments: 10
Patch Set 14 : Wait for app.listen #
Total comments: 3
Patch Set 15 : Update comment about relative URL #
Total comments: 9
Patch Set 16 : Merge lib/browserAction.js into lib/options.js #
Total comments: 11
Patch Set 17 : Remove ping and handle error #
Total comments: 7
Patch Set 18 : Do not callback if tab not found #Patch Set 19 : Set content size #
Total comments: 3
Patch Set 20 : Remove redundant top and left properties #Patch Set 21 : Rebase #
Total comments: 3
Patch Set 22 : Hide content frame until loaded #
Total comments: 8
Patch Set 23 : Remove workaround for FOUC issue and update adblockplusui dependency #
Total comments: 3
MessagesTotal messages: 79
Patch Set 1 See inline comments. https://codereview.adblockplus.org/29536764/diff/29536765/dependencies File dependencies (right): https://codereview.adblockplus.org/29536764/diff/29536765/dependencies#newcode6 dependencies:6: adblockplusui = adblockplusui hg:8ad6a38e6870 git:9460adb I've updated this to the latest as it'll include another change that we need for Firefox for Android. https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js File ext/background.js (left): https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js#o... ext/background.js:705: // Some versions of Firefox for Android before version 57 do have a There's no longer any need to mention this stuff about Firefox for Android's poor support for runtime.openOptionsPage, since that's no longer the reason why we're not using it. https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js#o... ext/background.js:735: else if ("windows" in chrome) Now this code is used for Firefox for Android as well. We need to do this because runtime.openOptionsPage, even if supported well, will only open options.html. We want to open mobile-options.html. https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js#o... ext/background.js:738: // and so this workaround needs to stay for now. Removed this part of the comment since this is no longer a "workaround", at least for Firefox for Android. Also, we're already using extension.getURL here, part of the comment is outdated. https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js#n... ext/background.js:333: currentPage.url.toString() == optionsPage.url.toString()) Do nothing if the options page is opened from the options page itself. https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js#n... ext/background.js:344: whitelisted: require("whitelisting") require is not available at the time of initialization of this file so we have to put it here.
Added Ollie. Ollie, does this look OK: https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js#o...
LGTM https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js#n... ext/background.js:790: ext.pages.open(optionsUrl, callback); Maybe add a comment above this line why we are not using fullOptionsUrl here? Something like: We are not using fullOptionsUrl here because of Edge issue: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/10276332
https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js File ext/background.js (left): https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js#o... ext/background.js:322: if (!("getPopup" in chrome.browserAction)) Is this check still correct? We don't want the popup but, go right to the options page, on mobile, even in (future) Firefox versions that support browser action popups. https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js#o... ext/background.js:705: // Some versions of Firefox for Android before version 57 do have a On 2017/09/06 01:12:12, Manish Jethani wrote: > There's no longer any need to mention this stuff about Firefox for Android's > poor support for runtime.openOptionsPage, since that's no longer the reason why > we're not using it. Hmm, the "options_ui.page" given in manifest.json, which is opened by runtime.openOptionsPage(), is also opened directly by the browser, e.g. through about:addons. So it seems we'd have to make sure anyway, one way or another, that when this page is opened, the user ends up with mobile UI. https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js#n... ext/background.js:333: currentPage.url.toString() == optionsPage.url.toString()) On 2017/09/06 01:12:12, Manish Jethani wrote: > Do nothing if the options page is opened from the options page itself. I wonder whether this should rather be handled by the options page itself, than by the code here? Also there are other scenarios like pages with protocols adblocking isn't supported anyway (e.g. ftp://, moz-extension://, about:), where therefore the option to whitelist this page should not show up either. https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js#n... ext/background.js:344: whitelisted: require("whitelisting") On 2017/09/06 01:12:12, Manish Jethani wrote: > require is not available at the time of initialization of this file so we have > to put it here. Which raises the question, why is this code inside ext, and not somewhere in lib/*, in the first place. Latest after these changes, this doesn't seem to be abstraction code anymore, but application logic.
https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js File ext/background.js (left): https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js#o... ext/background.js:322: if (!("getPopup" in chrome.browserAction)) On 2017/09/06 03:17:12, Sebastian Noack wrote: > Is this check still correct? We don't want the popup but, go right to the > options page, on mobile, even in (future) Firefox versions that support browser > action popups. You're right, this check is not correct. We don't have the info module available at this point either. I think we'll have to add the listener unconditionally and do the check in the handler. https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js#o... ext/background.js:705: // Some versions of Firefox for Android before version 57 do have a On 2017/09/06 03:17:12, Sebastian Noack wrote: > On 2017/09/06 01:12:12, Manish Jethani wrote: > > There's no longer any need to mention this stuff about Firefox for Android's > > poor support for runtime.openOptionsPage, since that's no longer the reason > why > > we're not using it. > > Hmm, the "options_ui.page" given in manifest.json, which is opened by > runtime.openOptionsPage(), is also opened directly by the browser, e.g. through > about:addons. So it seems we'd have to make sure anyway, one way or another, > that when this page is opened, the user ends up with mobile UI. This is a difficult one. Our options are to have a separate build target for Android, which I'm not sure is going to help if we can't release a separate mobile version of ABP for Firefox, or to have a single options.html and have it decide dynamically what UI to show to the user. Thomas, any thoughts on this? https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js#n... ext/background.js:333: currentPage.url.toString() == optionsPage.url.toString()) On 2017/09/06 03:17:12, Sebastian Noack wrote: > On 2017/09/06 01:12:12, Manish Jethani wrote: > > Do nothing if the options page is opened from the options page itself. > > I wonder whether this should rather be handled by the options page itself, than Perhaps that's a better idea. > by the code here? Also there are other scenarios like pages with protocols > adblocking isn't supported anyway (e.g. ftp://, moz-extension://, about:), where > therefore the option to whitelist this page should not show up either. Right, I'll add the check for http://*/* and https://*/* https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js#n... ext/background.js:344: whitelisted: require("whitelisting") On 2017/09/06 03:17:12, Sebastian Noack wrote: > On 2017/09/06 01:12:12, Manish Jethani wrote: > > require is not available at the time of initialization of this file so we have > > to put it here. > > Which raises the question, why is this code inside ext, and not somewhere in > lib/*, in the first place. Latest after these changes, this doesn't seem to be > abstraction code anymore, but application logic. Makes sense. Do you think this should be a new module like lib/mobileOptions.js? https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js#n... ext/background.js:790: ext.pages.open(optionsUrl, callback); On 2017/09/06 02:51:58, Oleksandr wrote: > Maybe add a comment above this line why we are not using fullOptionsUrl here? > Something like: > > We are not using fullOptionsUrl here because of Edge issue: > https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/10276332 Acknowledged.
https://codereview.adblockplus.org/29536764/diff/29536765/dependencies File dependencies (right): https://codereview.adblockplus.org/29536764/diff/29536765/dependencies#newcode6 dependencies:6: adblockplusui = adblockplusui hg:8ad6a38e6870 git:9460adb On 2017/09/06 01:12:12, Manish Jethani wrote: > I've updated this to the latest as it'll include another change that we need for > Firefox for Android. Thanks. I'd expect at least one more dependency update to be necessary before the release so keeping it up-to-date is definitely beneficial. https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js File ext/background.js (left): https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js#o... ext/background.js:705: // Some versions of Firefox for Android before version 57 do have a On 2017/09/06 10:32:22, Manish Jethani wrote: > This is a difficult one. Our options are to have a separate build target for > Android, which I'm not sure is going to help if we can't release a separate > mobile version of ABP for Firefox, or to have a single options.html and have it > decide dynamically what UI to show to the user. > > Thomas, any thoughts on this? I've been wondering about that too but couldn't come up with anything reasonable so I hoped that you guys had some input on that. Distributing separate builds is likely not an option so one idea would be to set up an automatic redirect in options.html but that's a very ugly workaround, obviously. Fortunately, any workaround we come up with will no longer be necessary as soon as the new options page is made responsive at which point we'll no longer need to bundle mobile-options.html. https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js#n... ext/background.js:345: .checkWhitelisted(currentPage) Looking at the desktop equivalent of this feature at https://hg.adblockplus.org/adblockpluschrome/file/a401d9555223/popup.js#l96 I noticed that the UI's behavior needs to change because it differs from the existing implementation in that it only cares about whether the page's domain is whitelisted. So to make this compatible with the way it is currently implemented, we'd instead need to check whether the filter "@@||example.com^$document" is active. So either we make it compatible now and change it to use `checkWhitelisted()` in a future dependency update or we accept this inconsistency until the UI's behavior is corrected. I'm fine with either of them so feel free to choose whichever you prefer. Sorry about the trouble. https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js#n... ext/background.js:772: // Firefox won't let us query for moz-extension:// pages either: Detail: It's worth mentioning that this has been fixed in Firefox 56 (see https://bugzilla.mozilla.org/show_bug.cgi?id=1271354)
https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js#n... ext/background.js:345: .checkWhitelisted(currentPage) FYI: I've created a spec ticket to get the Product team's take on this as well (see https://bitbucket.org/adblockplus/spec/issues/72/enable-toggle-behavior-is-in...).
https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js File ext/background.js (left): https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js#o... ext/background.js:705: // Some versions of Firefox for Android before version 57 do have a On 2017/09/06 12:40:02, Thomas Greiner wrote: > On 2017/09/06 10:32:22, Manish Jethani wrote: > > This is a difficult one. Our options are to have a separate build target for > > Android, which I'm not sure is going to help if we can't release a separate > > mobile version of ABP for Firefox, or to have a single options.html and have > it > > decide dynamically what UI to show to the user. > > > > Thomas, any thoughts on this? > > I've been wondering about that too but couldn't come up with anything reasonable > so I hoped that you guys had some input on that. > > Distributing separate builds is likely not an option so one idea would be to set > up an automatic redirect in options.html but that's a very ugly workaround, > obviously. > > Fortunately, any workaround we come up with will no longer be necessary as soon > as the new options page is made responsive at which point we'll no longer need > to bundle mobile-options.html. How about having the default options page embed the mobile options page in a frame, and hiding all other content, if running on Android? https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js#n... ext/background.js:344: whitelisted: require("whitelisting") On 2017/09/06 10:32:22, Manish Jethani wrote: > On 2017/09/06 03:17:12, Sebastian Noack wrote: > > On 2017/09/06 01:12:12, Manish Jethani wrote: > > > require is not available at the time of initialization of this file so we > have > > > to put it here. > > > > Which raises the question, why is this code inside ext, and not somewhere in > > lib/*, in the first place. Latest after these changes, this doesn't seem to be > > abstraction code anymore, but application logic. > > Makes sense. > > Do you think this should be a new module like lib/mobileOptions.js? No strong opinion. Though, another option would be lib/browserAction.js.
Patch Set 2: Move browserAction handler to lib/browserAction.js I haven't addressed the issue of options.html vs. mobile-options.html in this one. https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js#n... ext/background.js:333: currentPage.url.toString() == optionsPage.url.toString()) On 2017/09/06 10:32:22, Manish Jethani wrote: > On 2017/09/06 03:17:12, Sebastian Noack wrote: > > On 2017/09/06 01:12:12, Manish Jethani wrote: > > > Do nothing if the options page is opened from the options page itself. > > > > I wonder whether this should rather be handled by the options page itself, > than > > Perhaps that's a better idea. > > > by the code here? Also there are other scenarios like pages with protocols > > adblocking isn't supported anyway (e.g. ftp://, moz-extension://, about:), > where > > therefore the option to whitelist this page should not show up either. > > Right, I'll add the check for http://*/* and https://*/* Done. https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js#n... ext/background.js:344: whitelisted: require("whitelisting") On 2017/09/07 02:53:38, Sebastian Noack wrote: > On 2017/09/06 10:32:22, Manish Jethani wrote: > > On 2017/09/06 03:17:12, Sebastian Noack wrote: > > > On 2017/09/06 01:12:12, Manish Jethani wrote: > > > > require is not available at the time of initialization of this file so we > > have > > > > to put it here. > > > > > > Which raises the question, why is this code inside ext, and not somewhere in > > > lib/*, in the first place. Latest after these changes, this doesn't seem to > be > > > abstraction code anymore, but application logic. > > > > Makes sense. > > > > Do you think this should be a new module like lib/mobileOptions.js? > > No strong opinion. Though, another option would be lib/browserAction.js. Done. I've moved it to lib/browserAction.js now. https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js#n... ext/background.js:772: // Firefox won't let us query for moz-extension:// pages either: On 2017/09/06 12:40:02, Thomas Greiner wrote: > Detail: It's worth mentioning that this has been fixed in Firefox 56 (see > https://bugzilla.mozilla.org/show_bug.cgi?id=1271354) Done. https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js#n... ext/background.js:790: ext.pages.open(optionsUrl, callback); On 2017/09/06 10:32:22, Manish Jethani wrote: > On 2017/09/06 02:51:58, Oleksandr wrote: > > Maybe add a comment above this line why we are not using fullOptionsUrl here? > > Something like: > > > > We are not using fullOptionsUrl here because of Edge issue: > > https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/10276332 > > Acknowledged. Done. https://codereview.adblockplus.org/29536764/diff/29538606/lib/browserAction.js File lib/browserAction.js (right): https://codereview.adblockplus.org/29536764/diff/29538606/lib/browserAction.j... lib/browserAction.js:27: chrome.browserAction.onClicked.addListener(() => Note: We don't have to check for "fennec" here, because the handler simply won't be called on desktop since there already is a popup associated with the browser action. https://codereview.adblockplus.org/29536764/diff/29538606/lib/browserAction.j... lib/browserAction.js:35: if (!["http:", "https:"].includes(currentPage.url.protocol)) Now checking specifically for HTTP and HTTPS. https://codereview.adblockplus.org/29536764/diff/29538606/lib/browserAction.j... lib/browserAction.js:43: host: getDecodedHostname(currentPage.url).replace(/^www\./, ""), Using getDecodedHostname here so we can handle Unicode hostnames, similar to how popup.js does it.
https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js#n... ext/background.js:345: .checkWhitelisted(currentPage) On 2017/09/06 13:06:34, Thomas Greiner wrote: > FYI: I've created a spec ticket to get the Product team's take on this as well > (see > https://bitbucket.org/adblockplus/spec/issues/72/enable-toggle-behavior-is-in...). I'm not sure I understand. The difference between spec and popup UI behavior is only in the third point, i.e. switching it to "on"? Shouldn't checkWhitelisted("example.com") effectively check for "@@||example.com^$document", or does it do something different?
https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js File ext/background.js (left): https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js#o... ext/background.js:705: // Some versions of Firefox for Android before version 57 do have a On 2017/09/07 02:53:38, Sebastian Noack wrote: > On 2017/09/06 12:40:02, Thomas Greiner wrote: > > On 2017/09/06 10:32:22, Manish Jethani wrote: > > > This is a difficult one. Our options are to have a separate build target for > > > Android, which I'm not sure is going to help if we can't release a separate > > > mobile version of ABP for Firefox, or to have a single options.html and have > > it > > > decide dynamically what UI to show to the user. > > > > > > Thomas, any thoughts on this? > > > > I've been wondering about that too but couldn't come up with anything > reasonable > > so I hoped that you guys had some input on that. > > > > Distributing separate builds is likely not an option so one idea would be to > set > > up an automatic redirect in options.html but that's a very ugly workaround, > > obviously. > > > > Fortunately, any workaround we come up with will no longer be necessary as > soon > > as the new options page is made responsive at which point we'll no longer need > > to bundle mobile-options.html. > > How about having the default options page embed the mobile options page in a > frame, and hiding all other content, if running on Android? What about this? I would be open for other suggestions, but one way or another we should address this. https://codereview.adblockplus.org/29536764/diff/29538606/lib/browserAction.js File lib/browserAction.js (right): https://codereview.adblockplus.org/29536764/diff/29538606/lib/browserAction.j... lib/browserAction.js:35: if (!["http:", "https:"].includes(currentPage.url.protocol)) On 2017/09/07 11:04:16, Manish Jethani wrote: > Now checking specifically for HTTP and HTTPS. It would be great if we could reuse the same code here as we do in the popup to determine whether the current page can be whitelisted and whether it already is.
Patch Set 3: Add options page loader https://codereview.adblockplus.org/29536764/diff/29538606/lib/browserAction.js File lib/browserAction.js (right): https://codereview.adblockplus.org/29536764/diff/29538606/lib/browserAction.j... lib/browserAction.js:35: if (!["http:", "https:"].includes(currentPage.url.protocol)) On 2017/09/07 16:36:45, Sebastian Noack wrote: > On 2017/09/07 11:04:16, Manish Jethani wrote: > > Now checking specifically for HTTP and HTTPS. > > It would be great if we could reuse the same code here as we do in the popup to > determine whether the current page can be whitelisted and whether it already is. From what I can tell the code here is nearly identical to that in popup.js https://hg.adblockplus.org/adblockpluschrome/file/18983acc26fe/popup.js#l36 https://hg.adblockplus.org/adblockpluschrome/file/18983acc26fe/popup.js#l58 https://codereview.adblockplus.org/29536764/diff/29539648/metadata.gecko-webext File metadata.gecko-webext (right): https://codereview.adblockplus.org/29536764/diff/29539648/metadata.gecko-webe... metadata.gecko-webext:8: # Firefox has a different options page depending on whether it's desktop or Now Firefox has a different file for the options page, which then loads the actual options page. https://codereview.adblockplus.org/29536764/diff/29539648/options-loader.js File options-loader.js (right): https://codereview.adblockplus.org/29536764/diff/29539648/options-loader.js#n... options-loader.js:22: let url = "options.html"; The path here is relative to options-loader.html https://codereview.adblockplus.org/29536764/diff/29539648/options-loader.js#n... options-loader.js:28: document.location.replace(url); We use location.replace instead of location.assign so we don't end up with an extra entry in the browsing history (i.e. no back button).
Patch Set 4: Use iframe instead of redirection I like this approach because there's no redirection, which was looking bad in terms of UX, and all browsers have the same options page. Rietveld has messed this up, but essentially I've renamed options.* to desktop-options.* and now the new options.* is simply the wrapper around the actual options.* There's some CSS in options.html to make sure the iframe takes up the entire viewport. This needs to be tested on Edge. https://codereview.adblockplus.org/29536764/diff/29539648/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29536764/diff/29539648/ext/background.js#n... ext/background.js:728: // Firefox for Android has its own options page. There's no need for this now. https://codereview.adblockplus.org/29536764/diff/29539648/metadata.gecko-webext File metadata.gecko-webext (right): https://codereview.adblockplus.org/29536764/diff/29539648/metadata.gecko-webe... metadata.gecko-webext:8: # Firefox has a different options page depending on whether it's desktop or On 2017/09/08 10:02:25, Manish Jethani wrote: > Now Firefox has a different file for the options page, which then loads the > actual options page. And there's no need for this now. All browsers will have the same options page, the one that embeds either desktop-options.html or mobile-options.html.
Patch Set 5: Move file names to HTML Slightly better as the file names are now in the declarative part of the code, the JS simply applies values from there.
Patch Set 6: Pass boolean instead of object Fixed a bug here, it as passing the filter object to the options page, now it's just true or false.
https://codereview.adblockplus.org/29536764/diff/29538606/lib/browserAction.js File lib/browserAction.js (right): https://codereview.adblockplus.org/29536764/diff/29538606/lib/browserAction.j... lib/browserAction.js:35: if (!["http:", "https:"].includes(currentPage.url.protocol)) On 2017/09/08 10:02:24, Manish Jethani wrote: > On 2017/09/07 16:36:45, Sebastian Noack wrote: > > On 2017/09/07 11:04:16, Manish Jethani wrote: > > > Now checking specifically for HTTP and HTTPS. > > > > It would be great if we could reuse the same code here as we do in the popup > to > > determine whether the current page can be whitelisted and whether it already > is. > > From what I can tell the code here is nearly identical to that in popup.js > > https://hg.adblockplus.org/adblockpluschrome/file/18983acc26fe/popup.js#l36 > https://hg.adblockplus.org/adblockpluschrome/file/18983acc26fe/popup.js#l58 What I meant was to avoid duplicating the logic here and in popup.js. But it's probably not worth to be wrapped into a module or something. So never mind. https://codereview.adblockplus.org/29536764/diff/29539735/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29536764/diff/29539735/ext/background.js#n... ext/background.js:697: info.application != "fennec") Does this change still make sense? We open options.html in any scenario, again, then options.html will load either the mobile or dektop UI in a frame. So we can still use runtime.openOptionsPage(), if supported, right?
Patch Set 7: Use runtime.openOptionsPage on Firefox 57+ https://codereview.adblockplus.org/29536764/diff/29539735/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29536764/diff/29539735/ext/background.js#n... ext/background.js:697: info.application != "fennec") On 2017/09/11 22:40:02, Sebastian Noack wrote: > Does this change still make sense? We open options.html in any scenario, again, > then options.html will load either the mobile or dektop UI in a frame. So we can > still use runtime.openOptionsPage(), if supported, right? You're right, now we can just use runtime.openOptionsPage if it works. Done.
LGTM (for "next" bookmark)
Patch Set 8: Forward query string and fragment identifier to iframe According to adblockplusui/README.md the mobile options page relies on some query string parameters. Even though I couldn't verify this, it's a good idea to forward the query string and the fragment identifier anyway just in case either desktop-options.html or mobile-options.html starts relying on these in the future.
On 2017/09/13 00:28:30, Manish Jethani wrote: > Patch Set 8: Forward query string and fragment identifier to iframe > > According to adblockplusui/README.md the mobile options page relies on some > query string parameters. Even though I couldn't verify this, it's a good idea to > forward the query string and the fragment identifier anyway just in case either > desktop-options.html or mobile-options.html starts relying on these in the > future. These query string parameters are only supported (for testing purposes) when running the options page standalone from within the adblockplusui repository, not when it is integrated in the extension. So we can ignore them here. As for the hash, keep in mind that if the hash is updated in the top level document this change wouldn't be reflected in the frame.
On 2017/09/13 00:45:51, Sebastian Noack wrote: > On 2017/09/13 00:28:30, Manish Jethani wrote: > > Patch Set 8: Forward query string and fragment identifier to iframe > > > > According to adblockplusui/README.md the mobile options page relies on some > > query string parameters. Even though I couldn't verify this, it's a good idea > to > > forward the query string and the fragment identifier anyway just in case > either > > desktop-options.html or mobile-options.html starts relying on these in the > > future. > > These query string parameters are only supported (for testing purposes) when > running the options page standalone from within the adblockplusui repository, > not when it is integrated in the extension. So we can ignore them here. As for > the hash, keep in mind that if the hash is updated in the top level document > this change wouldn't be reflected in the frame. OK, I see. Thomas, would you like to take a look at Patch Set 7 (the previous one)?
On 2017/09/13 00:57:00, Manish Jethani wrote: > Thomas, would you like to take a look at Patch Set 7 (the previous one)? Yes, I still need to catch up with all the changes that happened since my last comment but I'm on it now so you can expect my reply sometime today.
I like the approach you use of having interstitial page for doing the redirect. On 2017/09/13 00:28:30, Manish Jethani wrote: > Even though I couldn't verify this, it's a good idea to > forward the query string and the fragment identifier anyway just in case either > desktop-options.html or mobile-options.html starts relying on these in the > future. I agree that it's probably a good idea to forward the URL's query string parameters and hash value for forwards-compatibility. https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29536764/diff/29536765/ext/background.js#n... ext/background.js:345: .checkWhitelisted(currentPage) On 2017/09/07 11:12:55, Manish Jethani wrote: > On 2017/09/06 13:06:34, Thomas Greiner wrote: > > FYI: I've created a spec ticket to get the Product team's take on this as well > > (see > > > https://bitbucket.org/adblockplus/spec/issues/72/enable-toggle-behavior-is-in...). > > I'm not sure I understand. > > The difference between spec and popup UI behavior is only in the third point, > i.e. switching it to "on"? > > Shouldn't checkWhitelisted("example.com") effectively check for > mailto:"@@||example.com^$document", or does it do something different? It checks whether the page is whitelisted, not the domain. The page might be whitelisted even if the domain isn't. We've discussed it in today's UI meeting as well and agreed on being consistent with the way it's implemented in the popup for now. That means that we can go with `checkWhitelisted()` in this review. Later on (meaning: sometime after the webext launch) we'll have to revisit this topic though to resolve this inconsistency. https://codereview.adblockplus.org/29536764/diff/29542849/options.js File options.js (right): https://codereview.adblockplus.org/29536764/diff/29542849/options.js#newcode29 options.js:29: iframe.src = src; What difference does it make whether we do the navigation in the top-level frame or a sub frame? Wouldn't it actually be simpler to just get rid of the iFrame and simply do `location.href = src` here without having to first wait for "DOMContentLoaded"? I'd say that we should use a redirect so that pages that we link to in the options page, by default, will open in the top-level frame rather than in the iFrame. Some problems with loading an external page within a sub frame: - The navigation is not reflected in the browser's address bar - Some pages prevent that they're being shown in an iFrame (e.g. by using the "X-Frame-Options" HTTP header)
Patch Set 9: Query tabs on Firefox as well Comments inline. https://codereview.adblockplus.org/29536764/diff/29542849/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29536764/diff/29542849/ext/background.js#n... ext/background.js:725: else if ("windows" in chrome) We need to query open tabs on Firefox for Android as well, in order to avoid opening the mobile options page more than once. https://codereview.adblockplus.org/29536764/diff/29542849/ext/background.js#n... ext/background.js:757: // Firefox for Android before version 57 does not support Moved this comment up. https://codereview.adblockplus.org/29536764/diff/29542849/options.js File options.js (right): https://codereview.adblockplus.org/29536764/diff/29542849/options.js#newcode22 options.js:22: document.addEventListener("DOMContentLoaded", () => No need for DOMContentLoaded now. https://codereview.adblockplus.org/29536764/diff/29542849/options.js#newcode28 options.js:28: src += location.search + location.hash; I find myself agreeing with Sebastian now that this is not required. If it's required in the future, it should be done properly to address the actual use case whenever it comes up. https://codereview.adblockplus.org/29536764/diff/29542849/options.js#newcode29 options.js:29: iframe.src = src; On 2017/09/13 14:56:29, Thomas Greiner wrote: > What difference does it make whether we do the navigation in the top-level frame > or a sub frame? Wouldn't it actually be simpler to just get rid of the iFrame > and simply do `location.href = src` here without having to first wait for > "DOMContentLoaded"? There are two issues with redirection at the top level: 1. The redirection is quite visible, which is slightly annoying in terms of user experience (but not a very big deal otherwise) 2. If the address bar shows anything other than "options.html", runtime.openOptionsPage will open yet another options page when it is called, which partly defeats the purpose of making these changes With the iframe embedding approach, the options page is always "options.html" regardless of what platform it is on. As for DOMContentLoaded, we can simply move the script tag to the bottom of the HTML instead. > I'd say that we should use a redirect so that pages that we link to in the > options page, by default, will open in the top-level frame rather than in the > iFrame. Some problems with loading an external page within a sub frame: > - The navigation is not reflected in the browser's address bar > - Some pages prevent that they're being shown in an iFrame (e.g. by using the > "X-Frame-Options" HTTP header) If these aren't actual problems we face with this approach right now, maybe we should keep these in mind without abandoning this approach, especially since top level redirection has its own problems as I mentioned above. https://codereview.adblockplus.org/29536764/diff/29543746/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29536764/diff/29543746/ext/background.js#n... ext/background.js:737: let optionsUrl = "options.html"; Just moved optionsUrl inside the hander. https://codereview.adblockplus.org/29536764/diff/29543746/options.html File options.html (right): https://codereview.adblockplus.org/29536764/diff/29543746/options.html#newcode42 options.html:42: <script src="options.js" charset="utf-8"></script> Moved the script tag down here so we don't have to listen for DOMContentLoaded
On 2017/09/13 14:56:32, Thomas Greiner wrote: > I agree that it's probably a good idea to forward the URL's query string > parameters and hash value for forwards-compatibility. It is not that trivial. If we suddenly have a query string our logic that detects the open options page would no longer work. And as for the hash, as I said before, it can change without the document being reloaded. It would be possible to address both scenarios, but that would require more changes than just passing through the query string + hash, which don't seem to be justified at the current point. https://codereview.adblockplus.org/29536764/diff/29542849/options.js File options.js (right): https://codereview.adblockplus.org/29536764/diff/29542849/options.js#newcode29 options.js:29: iframe.src = src; On 2017/09/13 16:07:43, Manish Jethani wrote: > On 2017/09/13 14:56:29, Thomas Greiner wrote: > > I'd say that we should use a redirect so that pages that we link to in the > > options page, by default, will open in the top-level frame rather than in the > > iFrame. Some problems with loading an external page within a sub frame: > > - The navigation is not reflected in the browser's address bar > > - Some pages prevent that they're being shown in an iFrame (e.g. by using the > > "X-Frame-Options" HTTP header) > > If these aren't actual problems we face with this approach right now, maybe we > should keep these in mind without abandoning this approach, especially since top > level redirection has its own problems as I mentioned above. It seems that all (external) links in the options page are opened in a new tab anyway. If we should ever have links that are supposed to be opened in the same tab we can just add the "target=_top" attribute to the link (or add a <base target="_top"> element to the document). I generally agree with Manish, and think using an iframe is causing less problems than redirecting the top level frame, here.
https://codereview.adblockplus.org/29536764/diff/29542849/options.js File options.js (right): https://codereview.adblockplus.org/29536764/diff/29542849/options.js#newcode29 options.js:29: iframe.src = src; On 2017/09/13 16:07:43, Manish Jethani wrote: > There are two issues with redirection at the top level: > > 1. The redirection is quite visible, which is slightly annoying in terms of > user experience (but not a very big deal otherwise) Do you mean on Android or in general? > 2. If the address bar shows anything other than "options.html", > runtime.openOptionsPage will open yet another options page when it is called, > which partly defeats the purpose of making these changes > > With the iframe embedding approach, the options page is always "options.html" > regardless of what platform it is on. I see what you mean. Can't we just use `window.history.replaceState(null, null, "/options.html")` in the two options pages to swap out the URL? Because that way they could pretend to be "options.html" without causing a redirect (at least as long as the origin of the new URL is the same).
@Sebastian Sorry, I didn't see your recent comment when I replied but yeah, while it'd be nice to pass parameters for the sake of forwards compatibility I'd consider it to be only a nice-to-have so no big deal. Regarding the second part of your comment, please let me know what you think about the suggestion I made in my previous comment. Otherwise we seem to be out of options anyway meaning that we need to go with the iFrame solution.
On 2017/09/13 16:57:11, Thomas Greiner wrote:
> Regarding the second part of your comment, please let me know what you think
> about the suggestion I made in my previous comment. Otherwise we seem to be
out
> of options anyway meaning that we need to go with the iFrame solution.
It seems history.replaceState() would do the trick, so that we can find the tab
later with chrome.tabs.query({url: chrome.extension.getURL("options.html")}).
However, the URL will still change, from "options.html" to
"desktop-options.html" or "mobile-options.html" back to "options.html" which
might look distracting in the address bar (and potentially causes other side
effects). We could, however, limit that effect to mobile (and possibly simplify
the implementation) by removing the intermediate page, moving this logic to the
original (desktop) options page, so that no redirect would have to happen on
desktop. I don't have a strong opinion whether this is better than the approach
with the iframe though.
On 2017/09/13 17:57:52, Sebastian Noack wrote: > We could, however, limit that effect to mobile (and possibly simplify > the implementation) by removing the intermediate page, moving this logic to the > original (desktop) options page, so that no redirect would have to happen on > desktop. I don't have a strong opinion whether this is better than the approach > with the iframe though. My original plan was to do the redirection in the original options.html and limit the effect of this change to Firefox for Android. We would use location.replace [1] here. Unfortunately this would mean that the code after the location.replace call would still get executed, causing errors and possible unintended effects on mobile. We could still work around it by inserting all the script tags into the DOM dynamically after confirming that it's not Firefox for Android, but I did not try this approach and I'm not sure that it's worth it just to avoid a redirect only on desktop. [1]: https://developer.mozilla.org/en-US/docs/Web/API/Location/replace
https://codereview.adblockplus.org/29536764/diff/29542849/options.js File options.js (right): https://codereview.adblockplus.org/29536764/diff/29542849/options.js#newcode29 options.js:29: iframe.src = src; On 2017/09/13 16:46:44, Thomas Greiner wrote: > On 2017/09/13 16:07:43, Manish Jethani wrote: > > There are two issues with redirection at the top level: > > > > 1. The redirection is quite visible, which is slightly annoying in terms of > > user experience (but not a very big deal otherwise) > > Do you mean on Android or in general? The redirection is visible on both desktop and mobile, it looks especially bad on mobile though. Basically there's a progress bar below the address bar and it gets interrupted and reset. > > 2. If the address bar shows anything other than "options.html", > > runtime.openOptionsPage will open yet another options page when it is called, > > which partly defeats the purpose of making these changes > > > > With the iframe embedding approach, the options page is always "options.html" > > regardless of what platform it is on. > > I see what you mean. Can't we just use `window.history.replaceState(null, null, > "/options.html")` in the two options pages to swap out the URL? Because that way > they could pretend to be "options.html" without causing a redirect (at least as > long as the origin of the new URL is the same). I just tried this approach, and here's what I found: 1. Works OK on desktop (Chrome and Firefox), except for the updating of the address bar and an extra entry in the user's browsing history 2. On Firefox for Android, the "app.respond" message sent to the options page doesn't make it because of the redirection being too slow
On 2017/09/13 16:57:11, Thomas Greiner wrote: > Regarding the second part of your comment, please let me know what you think > about the suggestion I made in my previous comment. Otherwise we seem to be out > of options anyway meaning that we need to go with the iFrame solution. Another option is to treat desktop-options.html and mobile-options.html as "templates" and load the correct template using XMLHttpRequest and inject the content into the page.
On 2017/09/14 03:30:36, Manish Jethani wrote: > We would use > location.replace [1] here. Unfortunately this would mean that the code after the > location.replace call would still get executed, causing errors and possible > unintended effects on mobile. We could prevent any follow-up script to be executed, for instance, by throwing an exception. > We could still work around it by inserting all the > script tags into the DOM dynamically after confirming that it's not Firefox for > Android, but I did not try this approach and I'm not sure that it's worth it > just to avoid a redirect only on desktop. I agree that this likely wouldn't be worth the effort. > The redirection is visible on both desktop and mobile, it looks especially bad > on mobile though. Basically there's a progress bar below the address bar and it > gets interrupted and reset. Sounds like expected behavior for a redirect so that might not be too bad. > 2. On Firefox for Android, the "app.respond" message sent to the options page > doesn't make it because of the redirection being too slow I wonder why this issue doesn't arise with the iFrame solution. Because, in theory, the race condition should have the same impact on either solution. Presumably, we could use the callbacks for `chrome.tabs.update()` as well as for `chrome.windows.update()` to get notified of when the tab got focused and created. Because at the moment we call `callback(new Page(tab))` immediately which may be the reason why we encounter this race condition. > Another option is to treat desktop-options.html and mobile-options.html as > "templates" and load the correct template using XMLHttpRequest and inject the > content into the page. It may be worth a try because I can imagine that it could help us avoid the race condition.
On 2017/09/14 10:13:41, Thomas Greiner wrote: > On 2017/09/14 03:30:36, Manish Jethani wrote: > > 2. On Firefox for Android, the "app.respond" message sent to the options > page > > doesn't make it because of the redirection being too slow > > I wonder why this issue doesn't arise with the iFrame solution. Because, in > theory, the race condition should have the same impact on either solution. > > Presumably, we could use the callbacks for `chrome.tabs.update()` as well as for > `chrome.windows.update()` to get notified of when the tab got focused and > created. Because at the moment we call `callback(new Page(tab))` immediately > which may be the reason why we encounter this race condition. I'll try to look into it, but I haven't run into the race condition and we could also deal with it separately if it comes up. > > Another option is to treat desktop-options.html and mobile-options.html as > > "templates" and load the correct template using XMLHttpRequest and inject the > > content into the page. > > It may be worth a try because I can imagine that it could help us avoid the race > condition. OK, so I've tried this approach. Let me give you the gist of how it works. First, the HTML contains the names of the template files: <html data-template-src="desktop-options.html" data-template-src-fennec="mobile-options.html"> ... </html> On DOMContentLoaded, we load the correct template file using XMLHttpRequest with the responseType set to "document" so it parses as HTML. Then we have a few options: 1. Use document.write to overwrite all the contents of the document 2. Use document.documentElement.innerHTML to populate the content 3. Add data-attribute-src="..." and data-attribute-href="..." attributes to the scripts and styles in the head element and set those values dynamically, then only populate the body of the document using one of the previous two methods The first one bombs on Android, as in the tab crashes, but it works on desktop beautifully. The second one populates the document content but doesn't execute the scripts, this is by design in HTML5, and we can than manually execute the scripts using a hack (create a copy of the script element and insert it, remove the original) but it's not reliable and we still need to fake a DOMContentLoaded event because the scripts rely on it. The third one is even more complicated, it works on desktop after we populate the body, then set the src and href attributes on the scripts and styles (link elements) respectively, then dispatch a fake DOMContentLoaded event. It does not work on mobile and I haven't yet looked into why that is. Anyway, this is getting way too complicated now and we already have a simple solution that works, modulo the potential race condition. I'm going to stash my changes now and work on polishing up the iframe approach. Let me know if you have any thoughts on the above.
On 2017/09/15 09:49:24, Manish Jethani wrote: > The third one is even more complicated, it works on desktop after we populate > the body, then set the src and href attributes on the scripts and styles (link > elements) respectively, then dispatch a fake DOMContentLoaded event. It does not > work on mobile and I haven't yet looked into why that is. This by the way involved moving the inline style sheet in desktop-options.html into its own skin/desktop-options.css.
On 2017/09/15 09:49:24, Manish Jethani wrote: > Anyway, this is getting way too complicated now and we already have a simple > solution that works, modulo the potential race condition. I'm going to stash my > changes now and work on polishing up the iframe approach. > > Let me know if you have any thoughts on the above. Ok, unless using the callbacks doesn't resolve the race condition on mobile I agree that we should go for the workaround with the iFrame.
Patch Set 10: Queue up messages from background page and forward them later This addresses the race condition. It works well on both desktop and mobile. https://codereview.adblockplus.org/29536764/diff/29545684/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29536764/diff/29545684/ext/background.js#n... ext/background.js:752: { This should be done regardless of the other changes. https://codereview.adblockplus.org/29536764/diff/29545684/options.js File options.js (right): https://codereview.adblockplus.org/29536764/diff/29545684/options.js#newcode22 options.js:22: let messageQueue = []; Let me explain this. Basically the top frame's load event is dispatched by the browser before all the frames have finished loading, which causes the background page to believe that the options page has finished loading, when in fact it has not. Now until the iframe has finished loading, we need to queue up any messages we receive. In the iframe's load handler, we forward the messages to the iframe's document. More comments below. https://codereview.adblockplus.org/29536764/diff/29545684/options.js#newcode28 options.js:28: // Ignore messages from anywhere but the background page. We probably only need to ignore messages from the options page itself, and I'm not even sure about that. But just to be safe, I'm only considering messages that are definitely from the background page. sender.url is undefined for the background page on Chrome and it is the actual URL of the background page on Firefox. https://codereview.adblockplus.org/29536764/diff/29545684/options.js#newcode49 options.js:49: let messageQueueLocalReference = messageQueue; This is just a safety measure just in case any errors are thrown in the following lines of code.
Looks good overall. Just a few small remarks since we've now concluded that the iFrame option is the only viable solution. https://codereview.adblockplus.org/29536764/diff/29545684/options.html File options.html (left): https://codereview.adblockplus.org/29536764/diff/29545684/options.html#oldcode30 options.html:30: <title>Adblock Plus Options</title> Don't get rid of the page title. It's a bit weird that the title text will be disconnected from the actual page content but at least we should keep it around and translate it. Alternatively, we could extract the title from the frame content because we're not updating the text anymore after the page has been loaded. https://codereview.adblockplus.org/29536764/diff/29545684/options.html File options.html (right): https://codereview.adblockplus.org/29536764/diff/29545684/options.html#newcode20 options.html:20: <meta charset="utf-8"> Coding style: "Don't omit optional HTML tags." https://codereview.adblockplus.org/29536764/diff/29545684/options.html#newcode27 options.html:27: } Detail: Since you're using fixed positioning to position the iFrame, what do you need these rules for? https://codereview.adblockplus.org/29536764/diff/29545684/options.html#newcode42 options.html:42: <script src="options.js" charset="utf-8"></script> Suggestion: You could use the "defer" or "async" attribute if you want to put the script tag inside the document head. https://codereview.adblockplus.org/29536764/diff/29545684/options.html#newcode42 options.html:42: <script src="options.js" charset="utf-8"></script> Detail: The "charset" attribute here is redundant because it has already been specified for the entire document. https://codereview.adblockplus.org/29536764/diff/29545684/options.js File options.js (right): https://codereview.adblockplus.org/29536764/diff/29545684/options.js#newcode37 options.js:37: // Return true to make the background page wait for the response. The UI may or may not call `sendResponse()` so we should only wait until we know whether any of the callbacks return `true`. i.e. `iframe.contentWindow.ext.onMessage._dispatch(...).includes(true)` However, currently, the background page doesn't expect any response from the options page for any messages it sends to it so it should be sufficient to return `false` if you want to keep things simple.
Patch Set 11: Update title, add defer attribute, coding style https://codereview.adblockplus.org/29536764/diff/29545684/options.html File options.html (left): https://codereview.adblockplus.org/29536764/diff/29545684/options.html#oldcode30 options.html:30: <title>Adblock Plus Options</title> On 2017/09/19 17:51:20, Thomas Greiner wrote: > Don't get rid of the page title. It's a bit weird that the title text will be > disconnected from the actual page content but at least we should keep it around > and translate it. > > Alternatively, we could extract the title from the frame content because we're > not updating the text anymore after the page has been loaded. I'm using "document.title = iframe.contentDocument.title" now. It should be OK even on mobile since the title is populated on DOMContentLoaded while we're waiting for the iframe's load event, which is later. Tested on Chrome, Firefox, and Firefox for Android. https://codereview.adblockplus.org/29536764/diff/29545684/options.html File options.html (right): https://codereview.adblockplus.org/29536764/diff/29545684/options.html#newcode20 options.html:20: <meta charset="utf-8"> On 2017/09/19 17:51:20, Thomas Greiner wrote: > Coding style: "Don't omit optional HTML tags." Done. https://codereview.adblockplus.org/29536764/diff/29545684/options.html#newcode27 options.html:27: } On 2017/09/19 17:51:20, Thomas Greiner wrote: > Detail: Since you're using fixed positioning to position the iFrame, what do you > need these rules for? This is a "just in case", for example if the user agent style sheet or a user style sheet (inserted by some other extension) sets any of these properties. If a border is set, for example, then it is clearly visible through the iframe since the iframe's background is transparent. While the padding does not affect the position and size of the iframe, it can affect the size of the page, causing a scrollbar to be displayed on the top frame (e.g. "padding: 1000px" to use an extreme example). Maybe we should remove all these and just set "overflow: hidden" since that would be necessary in case there are any UA/user styles that mess with the size of the page. These are not strictly necessary though, I can remove them if you think they clutter the code needlessly. https://codereview.adblockplus.org/29536764/diff/29545684/options.html#newcode42 options.html:42: <script src="options.js" charset="utf-8"></script> On 2017/09/19 17:51:20, Thomas Greiner wrote: > Suggestion: You could use the "defer" or "async" attribute if you want to put > the script tag inside the document head. Done. https://codereview.adblockplus.org/29536764/diff/29545684/options.html#newcode42 options.html:42: <script src="options.js" charset="utf-8"></script> On 2017/09/19 17:51:20, Thomas Greiner wrote: > Detail: The "charset" attribute here is redundant because it has already been > specified for the entire document. Done. https://codereview.adblockplus.org/29536764/diff/29545684/options.js File options.js (right): https://codereview.adblockplus.org/29536764/diff/29545684/options.js#newcode37 options.js:37: // Return true to make the background page wait for the response. On 2017/09/19 17:51:21, Thomas Greiner wrote: > The UI may or may not call `sendResponse()` so we should only wait until we know > whether any of the callbacks return `true`. > > i.e. `iframe.contentWindow.ext.onMessage._dispatch(...).includes(true)` > > However, currently, the background page doesn't expect any response from the > options page for any messages it sends to it so it should be sufficient to > return `false` if you want to keep things simple. OK, in that case queueMessage now doesn't return anything. Done. https://codereview.adblockplus.org/29536764/diff/29549910/options.js File options.js (right): https://codereview.adblockplus.org/29536764/diff/29549910/options.js#newcode44 options.js:44: document.title = iframe.contentDocument.title; Update title.
LGTM https://codereview.adblockplus.org/29536764/diff/29545684/options.html File options.html (left): https://codereview.adblockplus.org/29536764/diff/29545684/options.html#oldcode30 options.html:30: <title>Adblock Plus Options</title> On 2017/09/19 19:42:04, Manish Jethani wrote: > I'm using "document.title = iframe.contentDocument.title" now. It should be OK > even on mobile since the title is populated on DOMContentLoaded while we're > waiting for the iframe's load event, which is later. > > Tested on Chrome, Firefox, and Firefox for Android. The title in the options page is not hardcoded like here but generated in a listener for the "DOMContentLoaded" event. But since you were able to verify that it works on all platforms, it appears that the "DOMContentLoaded" event within the iFrame is dispatched before the one in the top-level frame. So in that case this seems to be working as expected. https://codereview.adblockplus.org/29536764/diff/29545684/options.html File options.html (right): https://codereview.adblockplus.org/29536764/diff/29545684/options.html#newcode27 options.html:27: } On 2017/09/19 19:42:04, Manish Jethani wrote: > This is a "just in case", for example if the user agent style sheet or a user > style sheet (inserted by some other extension) sets any of these properties. > > If a border is set, for example, then it is clearly visible through the iframe > since the iframe's background is transparent. > > While the padding does not affect the position and size of the iframe, it can > affect the size of the page, causing a scrollbar to be displayed on the top > frame (e.g. "padding: 1000px" to use an extreme example). > > Maybe we should remove all these and just set "overflow: hidden" since that > would be necessary in case there are any UA/user styles that mess with the size > of the page. > > These are not strictly necessary though, I can remove them if you think they > clutter the code needlessly. Optimizing for user styles has never even occurred to me and it hasn't been discussed yet whether this is a use-case we can/want to optimize for. I don't have a preference towards either though so feel free to leave it like this.
On 2017/09/20 12:30:00, Thomas Greiner wrote: > LGTM Thanks. Sebastian brought up on IRC that maybe we could use two-way messaging to synchronize between the background page and the options page, as an alternative to queuing up messages like this. I'll give it a try, but if that doesn't look good we can go back to this.
https://codereview.adblockplus.org/29536764/diff/29545684/options.html File options.html (left): https://codereview.adblockplus.org/29536764/diff/29545684/options.html#oldcode30 options.html:30: <title>Adblock Plus Options</title> On 2017/09/20 12:29:59, Thomas Greiner wrote: > On 2017/09/19 19:42:04, Manish Jethani wrote: > > I'm using "document.title = iframe.contentDocument.title" now. It should be OK > > even on mobile since the title is populated on DOMContentLoaded while we're > > waiting for the iframe's load event, which is later. > > > > Tested on Chrome, Firefox, and Firefox for Android. > > The title in the options page is not hardcoded like here but generated in a > listener for the "DOMContentLoaded" event. But since you were able to verify > that it works on all platforms, it appears that the "DOMContentLoaded" event > within the iFrame is dispatched before the one in the top-level frame. So in > that case this seems to be working as expected. We update the title in iframe.onload, which is triggered when the "load" event of the iframe occurs. It's definitely after the iframe's "DOMContentLoaded" event. The iframe's "DOMContentLoaded" cannot occur before the top frame's because we wait for the top frame's "DOMContentLoaded" to start loading the iframe's content at all, but anyway this is not a problem since we're waiting until the iframe's "load" event in fact.
Patch Set 12: Use ping messages to sync up https://codereview.adblockplus.org/29536764/diff/29549910/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29536764/diff/29549910/ext/background.js#n... ext/background.js:690: ext.showOptions = callback => All of this code has been moved into lib/options.js https://codereview.adblockplus.org/29536764/diff/29549910/options.js File options.js (right): https://codereview.adblockplus.org/29536764/diff/29549910/options.js#newcode28 options.js:28: // Ignore messages from anywhere but the background page. Now there's no need to queue messages, all of this code is gone. https://codereview.adblockplus.org/29536764/diff/29556686/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29536764/diff/29556686/desktop-options.js#... desktop-options.js:749: $(() => As the last thing in this script, we add a listener for jQuery's ready event and dispatch a ping from there. We could wait until window.load but it is not necessary. Also note that we could do this in the top frame, but then the top frame would have to know when the best time to dispatch the ping would be, for both desktop-options and mobile-options. This knowledge is internal to the two different pages, hence it's best they do it themselves. https://codereview.adblockplus.org/29536764/diff/29556686/desktop-options.js#... desktop-options.js:754: sendResponse(true); We also have to respond to any ping we receive. https://codereview.adblockplus.org/29536764/diff/29556686/lib/options.js File lib/options.js (right): https://codereview.adblockplus.org/29536764/diff/29556686/lib/options.js#newc... lib/options.js:18: /** @module options */ lib/options.js is now a new module, with just the previously ext.showOptions for now. https://codereview.adblockplus.org/29536764/diff/29556686/lib/options.js#newc... lib/options.js:57: exports.showOptions = callback => This is just ext.showOptions with a tiny bit of refactoring. https://codereview.adblockplus.org/29536764/diff/29556686/popup.js File popup.js (right): https://codereview.adblockplus.org/29536764/diff/29556686/popup.js#newcode81 popup.js:81: showOptions(); I realize I need to rebase this one on next too.
Some more comments ... https://codereview.adblockplus.org/29536764/diff/29556686/lib/options.js File lib/options.js (right): https://codereview.adblockplus.org/29536764/diff/29556686/lib/options.js#newc... lib/options.js:25: function whenOnline(page) There should probably be a module lib/page.js and it should contain ext.Page as well as utility functions like these, which can then be used from anywhere. https://codereview.adblockplus.org/29536764/diff/29556686/lib/options.js#newc... lib/options.js:38: port.on("ping", onPing); I was going to call this app.ping or options.ping, but then settled for ping since it's so basic. Let me know if you have suggestions. My idea is that anyone can send a ping to anyone. A listener may respond to a ping with a true value to indicate that it's listening (otherwise the default is undefined if there's no listener available).
Patch Set 13: Rebase on next
https://codereview.adblockplus.org/29536764/diff/29556686/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29536764/diff/29556686/desktop-options.js#... desktop-options.js:749: $(() => On 2017/09/26 20:37:16, Manish Jethani wrote: > As the last thing in this script, we add a listener for jQuery's ready event and > dispatch a ping from there. > > We could wait until window.load but it is not necessary. > > Also note that we could do this in the top frame, but then the top frame would > have to know when the best time to dispatch the ping would be, for both > desktop-options and mobile-options. This knowledge is internal to the two > different pages, hence it's best they do it themselves. Perhaps we don't even need this. All, the old options page, the new desktop options, and the new mobile options page send an "app.listen" message as soon as they are ready. The problem, however, is that we still need a way to detect the options page in case it has already been open. But perhaps, we don't have to bother about timing that much in this situation. It seems extremely unlikely that the user could trigger the options page to be opened and then clicks an abp:subscribe (for example) in less than it takes to load the options page. So we could just check whether the options page is already open, and if so assume that it is ready, and if not open it and then wait for the "app.listen" event. What do you think? https://codereview.adblockplus.org/29536764/diff/29556686/options.js File options.js (right): https://codereview.adblockplus.org/29536764/diff/29556686/options.js#newcode20 options.js:20: const {require} = chrome.extension.getBackgroundPage(); Note that when we are using webpack soon, we can only use require() in scripts that are part of the bundle. However, you can request the info that you need here asynchronously with the following message: {type: "app.get", what: "application"} https://codereview.adblockplus.org/29536764/diff/29556757/lib/browserAction.js File lib/browserAction.js (right): https://codereview.adblockplus.org/29536764/diff/29556757/lib/browserAction.j... lib/browserAction.js:18: /** @module browserAction */ Perhaps, this code can go into lib/options.js as well now? https://codereview.adblockplus.org/29536764/diff/29556757/lib/notificationHel... File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29536764/diff/29556757/lib/notificationHel... lib/notificationHelper.js:127: showOptions(page => I didn't go through the code, but assuming all instanced of showOptions() with callback are in the same form: showOptions(page => { page.sendMessage({ type: "app.respond", action: <some_action> args: <some_args> }); }); It seems to make more sense to make showOptions() take "action" and "args" instead of a callback: showOptions([some_action[, some_args]]);
https://codereview.adblockplus.org/29536764/diff/29556686/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29536764/diff/29556686/desktop-options.js#... desktop-options.js:749: $(() => On 2017/09/27 01:38:34, Sebastian Noack wrote: > On 2017/09/26 20:37:16, Manish Jethani wrote: > > As the last thing in this script, we add a listener for jQuery's ready event > and > > dispatch a ping from there. > > > > We could wait until window.load but it is not necessary. > > > > Also note that we could do this in the top frame, but then the top frame would > > have to know when the best time to dispatch the ping would be, for both > > desktop-options and mobile-options. This knowledge is internal to the two > > different pages, hence it's best they do it themselves. > > Perhaps we don't even need this. > > All, the old options page, the new desktop options, and the new mobile options > page send an "app.listen" message as soon as they are ready. We could use "app.listen" in lieu of "ping" here. It seems to be dispatched before the frame's load event. Perhaps then it would make sense to move the code that waits for the "app.listen" message before sending its own message, "app.respond" or "app.addSubscription" or whatever, out of lib/options.js and wherever it is specifically needed. Either that, or showOptions takes a "waitForMessage" parameter and waits until that message before invoking the callback; then the caller can pass in the appropriate message type. > The problem, however, is that we still need a way to detect the options page in > case it has already been open. But perhaps, we don't have to bother about timing > that much in this situation. It seems extremely unlikely that the user could > trigger the options page to be opened and then clicks an abp:subscribe (for > example) in less than it takes to load the options page. > > So we could just check whether the options page is already open, and if so > assume that it is ready, and if not open it and then wait for the "app.listen" > event. What do you think? runtime.openOptionsPage doesn't tell you if the page was already open, so then we'd have to resort to using tabs.query the way we're doing for Edge and Firefox for Android, but yeah it would work. https://codereview.adblockplus.org/29536764/diff/29556686/options.js File options.js (right): https://codereview.adblockplus.org/29536764/diff/29556686/options.js#newcode20 options.js:20: const {require} = chrome.extension.getBackgroundPage(); On 2017/09/27 01:38:34, Sebastian Noack wrote: > Note that when we are using webpack soon, we can only use require() in scripts > that are part of the bundle. I see. > However, you can request the info that you need here asynchronously with the > following message: > > {type: "app.get", what: "application"} OK. Perhaps there should be a stateless version lib/adblockplus.js that includes things like the info module and some utility classes/functions, then we can include it in the popup and the options page and not have to use messaging for every little thing. https://codereview.adblockplus.org/29536764/diff/29556757/lib/browserAction.js File lib/browserAction.js (right): https://codereview.adblockplus.org/29536764/diff/29556757/lib/browserAction.j... lib/browserAction.js:18: /** @module browserAction */ On 2017/09/27 01:38:35, Sebastian Noack wrote: > Perhaps, this code can go into lib/options.js as well now? For now it could, but technically "browser action" is different than "options", it's actually more related to the popup than the options page (only on mobile we open the options page directly), therefore maybe it's good to just leave it here. If we want to get rid of ext/background.js eventually then the BrowserAction class would end up here, I think. https://codereview.adblockplus.org/29536764/diff/29556757/lib/notificationHel... File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29536764/diff/29556757/lib/notificationHel... lib/notificationHelper.js:127: showOptions(page => On 2017/09/27 01:38:35, Sebastian Noack wrote: > I didn't go through the code, but assuming all instanced of showOptions() with > callback are in the same form: > > showOptions(page => > { > page.sendMessage({ > type: "app.respond", > action: <some_action> > args: <some_args> > }); > }); > > It seems to make more sense to make showOptions() take "action" and "args" > instead of a callback: > > showOptions([some_action[, some_args]]); In messageResponder.js we have this: ext.showOptions(() => { if (!message.action) return; sendMessage("app", message.action, ...message.args); }); That sendMessage function sends the message to all the listeners after running some conversion on the arguments. We could inline some of that code here, just for the options page, but I think still means that the options page would receive the message twice.
Patch Set 14: Wait for app.listen https://codereview.adblockplus.org/29536764/diff/29557653/lib/options.js File lib/options.js (right): https://codereview.adblockplus.org/29536764/diff/29557653/lib/options.js#newc... lib/options.js:69: let page = new ext.Page(tab); Create a Page object and release the tab reference here while we wait for a response. https://codereview.adblockplus.org/29536764/diff/29557653/lib/options.js#newc... lib/options.js:83: findOptionsTab(optionsTab => Now we first try to find the options tab, since that is common to all platforms. The platform-specific branches are then inside the callback. https://codereview.adblockplus.org/29536764/diff/29557653/lib/options.js#newc... lib/options.js:118: chrome.tabs.create({url: optionsUrl}, () => I've changed this to chrome.tabs.create, from ext.pages.open, because we specifically want to start listening for the message as early as possible. I realized this when it did not work on Firefox for Android with ext.pages.open.
Patch Set 15: Update comment about relative URL
https://codereview.adblockplus.org/29536764/diff/29556686/options.js File options.js (right): https://codereview.adblockplus.org/29536764/diff/29556686/options.js#newcode20 options.js:20: const {require} = chrome.extension.getBackgroundPage(); On 2017/09/27 11:20:54, Manish Jethani wrote: > On 2017/09/27 01:38:34, Sebastian Noack wrote: > > Note that when we are using webpack soon, we can only use require() in scripts > > that are part of the bundle. > > I see. > > > However, you can request the info that you need here asynchronously with the > > following message: > > > > {type: "app.get", what: "application"} > > OK. > > Perhaps there should be a stateless version lib/adblockplus.js that includes > things like the info module and some utility classes/functions, then we can > include it in the popup and the options page and not have to use messaging for > every little thing. If it is just for the info module, I don't think it is too bad having to use asynchronous messaging the couple few times we need to query these information from outside the background page. And I wouldn't know for which other logic we could potentially avoid messaging, by redundantly including the code. Also note that on Firefox, application and applicationVersion is retrieved from an async APIs. By the time the options page is opened it is safe to assume, these information are available in the background page, but if we would just include the same code here, we would have to wait, which isn't any less complicated than just asking the background page. Anyway, the latest patch set still uses chrome.extension.getBackgroundPage().require(). We should change this. https://codereview.adblockplus.org/29536764/diff/29556757/lib/browserAction.js File lib/browserAction.js (right): https://codereview.adblockplus.org/29536764/diff/29556757/lib/browserAction.j... lib/browserAction.js:18: /** @module browserAction */ On 2017/09/27 11:20:55, Manish Jethani wrote: > On 2017/09/27 01:38:35, Sebastian Noack wrote: > > Perhaps, this code can go into lib/options.js as well now? > > For now it could, but technically "browser action" is different than "options", > it's actually more related to the popup than the options page (only on mobile we > open the options page directly), therefore maybe it's good to just leave it > here. If we want to get rid of ext/background.js eventually then the > BrowserAction class would end up here, I think. We would not preserve the BrowserAction class as it is. It only exists as a class so that it can be a property on the Page object, which should go away as well. However, we might need to keep a wrapper around the chrome.tabs.browserAction.set* APIs, to keep working around the issue when the tab is pre-rendered, unless this isn't necessary anymore, or we find a better way to deal with it. Anyway, for the time being, I prefer to not put every single tiny piece of code into their separate module, and the code here is arguably related to the options. That we use the browserAction API here is rather an implementation detail, but the code isn't any more related to any other code we might move in here in the future, than the code now added with the "options" module. https://codereview.adblockplus.org/29536764/diff/29556757/lib/notificationHel... File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29536764/diff/29556757/lib/notificationHel... lib/notificationHelper.js:127: showOptions(page => On 2017/09/27 11:20:55, Manish Jethani wrote: > On 2017/09/27 01:38:35, Sebastian Noack wrote: > > I didn't go through the code, but assuming all instanced of showOptions() with > > callback are in the same form: > > > > showOptions(page => > > { > > page.sendMessage({ > > type: "app.respond", > > action: <some_action> > > args: <some_args> > > }); > > }); > > > > It seems to make more sense to make showOptions() take "action" and "args" > > instead of a callback: > > > > showOptions([some_action[, some_args]]); > > In messageResponder.js we have this: > > ext.showOptions(() => > { > if (!message.action) > return; > > sendMessage("app", message.action, ...message.args); > }); > > That sendMessage function sends the message to all the listeners after running > some conversion on the arguments. We could inline some of that code here, just > for the options page, but I think still means that the options page would > receive the message twice. The callback there seems to be dead code to me. I can only find one instance of app.open in the whole code base, and message.action isn't set there. Or do I miss something? If not, I suggest to just remove the callback from that call. We have to adapt adblockplusui, anyway, since it is no longer ext.showOptions() but require("options").showOptions(). https://codereview.adblockplus.org/29536764/diff/29557665/lib/options.js File lib/options.js (right): https://codereview.adblockplus.org/29536764/diff/29557665/lib/options.js#newc... lib/options.js:64: port.off("app.listen"); It seems you forgot to specify the listener function as second argument here. https://codereview.adblockplus.org/29536764/diff/29557665/lib/options.js#newc... lib/options.js:69: let page = new ext.Page(tab); You could get rid of this temporary variable (without adding any duplication if you just inline it above: callback(new ext.Path(tab)) And then just use tab.id instead of page.id in the check above that. https://codereview.adblockplus.org/29536764/diff/29557665/lib/options.js#newc... lib/options.js:79: * @static Is marking this function as @static necessary? It seems we don't for any other exported function in any other module. Except there is a reason we should remain consistent, I guess. https://codereview.adblockplus.org/29536764/diff/29557665/lib/options.js#newc... lib/options.js:101: else if (optionsTab) Shouldn't this branch go first?
Patch Set 16: Merge lib/browserAction.js into lib/options.js https://codereview.adblockplus.org/29536764/diff/29556686/options.js File options.js (right): https://codereview.adblockplus.org/29536764/diff/29556686/options.js#newcode20 options.js:20: const {require} = chrome.extension.getBackgroundPage(); On 2017/09/30 02:38:43, Sebastian Noack wrote: > On 2017/09/27 11:20:54, Manish Jethani wrote: > > On 2017/09/27 01:38:34, Sebastian Noack wrote: > > > Note that when we are using webpack soon, we can only use require() in > scripts > > > that are part of the bundle. > > > > I see. > > > > > However, you can request the info that you need here asynchronously with the > > > following message: > > > > > > {type: "app.get", what: "application"} > > > > OK. > > > > Perhaps there should be a stateless version lib/adblockplus.js that includes > > things like the info module and some utility classes/functions, then we can > > include it in the popup and the options page and not have to use messaging for > > every little thing. > > If it is just for the info module, I don't think it is too bad having to use > asynchronous messaging the couple few times we need to query these information > from outside the background page. And I wouldn't know for which other logic we > could potentially avoid messaging, by redundantly including the code. > > Also note that on Firefox, application and applicationVersion is retrieved from > an async APIs. By the time the options page is opened it is safe to assume, > these information are available in the background page, but if we would just > include the same code here, we would have to wait, which isn't any less > complicated than just asking the background page. > > Anyway, the latest patch set still uses > chrome.extension.getBackgroundPage().require(). We should change this. Done. https://codereview.adblockplus.org/29536764/diff/29556757/lib/browserAction.js File lib/browserAction.js (right): https://codereview.adblockplus.org/29536764/diff/29556757/lib/browserAction.j... lib/browserAction.js:18: /** @module browserAction */ On 2017/09/30 02:38:43, Sebastian Noack wrote: > On 2017/09/27 11:20:55, Manish Jethani wrote: > > On 2017/09/27 01:38:35, Sebastian Noack wrote: > > > Perhaps, this code can go into lib/options.js as well now? > > > > For now it could, but technically "browser action" is different than > "options", > > it's actually more related to the popup than the options page (only on mobile > we > > open the options page directly), therefore maybe it's good to just leave it > > here. If we want to get rid of ext/background.js eventually then the > > BrowserAction class would end up here, I think. > > We would not preserve the BrowserAction class as it is. It only exists as a > class so that it can be a property on the Page object, which should go away as > well. However, we might need to keep a wrapper around the > chrome.tabs.browserAction.set* APIs, to keep working around the issue when the > tab is pre-rendered, unless this isn't necessary anymore, or we find a better > way to deal with it. > > Anyway, for the time being, I prefer to not put every single tiny piece of code > into their separate module, and the code here is arguably related to the > options. That we use the browserAction API here is rather an implementation > detail, but the code isn't any more related to any other code we might move in > here in the future, than the code now added with the "options" module. Done. https://codereview.adblockplus.org/29536764/diff/29556757/lib/notificationHel... File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29536764/diff/29556757/lib/notificationHel... lib/notificationHelper.js:127: showOptions(page => On 2017/09/30 02:38:43, Sebastian Noack wrote: > On 2017/09/27 11:20:55, Manish Jethani wrote: > > On 2017/09/27 01:38:35, Sebastian Noack wrote: > > > I didn't go through the code, but assuming all instanced of showOptions() > with > > > callback are in the same form: > > > > > > showOptions(page => > > > { > > > page.sendMessage({ > > > type: "app.respond", > > > action: <some_action> > > > args: <some_args> > > > }); > > > }); > > > > > > It seems to make more sense to make showOptions() take "action" and "args" > > > instead of a callback: > > > > > > showOptions([some_action[, some_args]]); > > > > In messageResponder.js we have this: > > > > ext.showOptions(() => > > { > > if (!message.action) > > return; > > > > sendMessage("app", message.action, ...message.args); > > }); > > > > That sendMessage function sends the message to all the listeners after running > > some conversion on the arguments. We could inline some of that code here, just > > for the options page, but I think still means that the options page would > > receive the message twice. > > The callback there seems to be dead code to me. I can only find one instance of > app.open in the whole code base, and message.action isn't set there. Or do I > miss something? If not, I suggest to just remove the callback from that call. This callback is required for the abp:subscribe links. In fact, the issue #5748 occurs because this callback is getting called too soon. > [...] https://codereview.adblockplus.org/29536764/diff/29557665/lib/options.js File lib/options.js (right): https://codereview.adblockplus.org/29536764/diff/29557665/lib/options.js#newc... lib/options.js:64: port.off("app.listen"); On 2017/09/30 02:38:44, Sebastian Noack wrote: > It seems you forgot to specify the listener function as second argument here. Done. https://codereview.adblockplus.org/29536764/diff/29557665/lib/options.js#newc... lib/options.js:69: let page = new ext.Page(tab); On 2017/09/30 02:38:45, Sebastian Noack wrote: > You could get rid of this temporary variable (without adding any duplication if > you just inline it above: > > callback(new ext.Path(tab)) > > And then just use tab.id instead of page.id in the check above that. Done. https://codereview.adblockplus.org/29536764/diff/29557665/lib/options.js#newc... lib/options.js:79: * @static On 2017/09/30 02:38:45, Sebastian Noack wrote: > Is marking this function as @static necessary? It seems we don't for any other > exported function in any other module. Except there is a reason we should remain > consistent, I guess. Done. https://codereview.adblockplus.org/29536764/diff/29557665/lib/options.js#newc... lib/options.js:101: else if (optionsTab) On 2017/09/30 02:38:44, Sebastian Noack wrote: > Shouldn't this branch go first? I guess that depends. Do we want to skip calling runtime.openOptionsPage if we've already found an options page open in a tab? Maybe runtime.openOptionsPage does other things (like animating the tab or flashing it to draw the user's attention), either now or in the future, so I'd say if the API is available then maybe we should just use it instead of trying to do it ourselves, this way we'd get the best user experience. Right now though, on the platforms I've tested this on, it seems to make no difference either way.
https://codereview.adblockplus.org/29536764/diff/29560558/options.js File options.js (right): https://codereview.adblockplus.org/29536764/diff/29560558/options.js#newcode27 options.js:27: chrome.runtime.sendMessage({ I should note that there's a layout issue with the mobile options page that has come to surface with this change (though it was always there). If the page is loaded in an iframe too late, its scaling is all messed up. I can also see it in my Android emulator here when I refresh the options page. I suspect this has something to do with <meta name="viewport" ...>, I think we'll file a separate issue for this. Some CSS or meta tags should fix this, as I'm pretty sure it's fairly normal to have iframes in mobile web pages.
https://codereview.adblockplus.org/29536764/diff/29556757/lib/notificationHel... File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29536764/diff/29556757/lib/notificationHel... lib/notificationHelper.js:127: showOptions(page => On 2017/09/30 13:49:52, Manish Jethani wrote: > On 2017/09/30 02:38:43, Sebastian Noack wrote: > > On 2017/09/27 11:20:55, Manish Jethani wrote: > > > On 2017/09/27 01:38:35, Sebastian Noack wrote: > > > > I didn't go through the code, but assuming all instanced of showOptions() > > with > > > > callback are in the same form: > > > > > > > > showOptions(page => > > > > { > > > > page.sendMessage({ > > > > type: "app.respond", > > > > action: <some_action> > > > > args: <some_args> > > > > }); > > > > }); > > > > > > > > It seems to make more sense to make showOptions() take "action" and "args" > > > > instead of a callback: > > > > > > > > showOptions([some_action[, some_args]]); > > > > > > In messageResponder.js we have this: > > > > > > ext.showOptions(() => > > > { > > > if (!message.action) > > > return; > > > > > > sendMessage("app", message.action, ...message.args); > > > }); > > > > > > That sendMessage function sends the message to all the listeners after > running > > > some conversion on the arguments. We could inline some of that code here, > just > > > for the options page, but I think still means that the options page would > > > receive the message twice. > > > > The callback there seems to be dead code to me. I can only find one instance > of > > app.open in the whole code base, and message.action isn't set there. Or do I > > miss something? If not, I suggest to just remove the callback from that call. > > This callback is required for the abp:subscribe links. In fact, the issue #5748 > occurs because this callback is getting called too soon. > > > [...] abp:subscribe links are not handled through the app.open message, and my suggestion was not to unsupport the scenario where there are additional information to be send to the options page, but rather streamline it by passing the data to be send to the options page rather than a callback with duplicated boilerplate, see the first comment in this thread. https://codereview.adblockplus.org/29536764/diff/29557665/lib/options.js File lib/options.js (right): https://codereview.adblockplus.org/29536764/diff/29557665/lib/options.js#newc... lib/options.js:101: else if (optionsTab) On 2017/09/30 13:49:53, Manish Jethani wrote: > On 2017/09/30 02:38:44, Sebastian Noack wrote: > > Shouldn't this branch go first? > > I guess that depends. Do we want to skip calling runtime.openOptionsPage if > we've already found an options page open in a tab? Maybe runtime.openOptionsPage > does other things (like animating the tab or flashing it to draw the user's > attention), either now or in the future, so I'd say if the API is available then > maybe we should just use it instead of trying to do it ourselves, this way we'd > get the best user experience. > > Right now though, on the platforms I've tested this on, it seems to make no > difference either way. I think you are right, it is better to rely on runitm.openOptionPage () when available, in order to bring up the existing options tab if any. https://codereview.adblockplus.org/29536764/diff/29560558/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29536764/diff/29560558/desktop-options.js#... desktop-options.js:749: $(() => So I guess this code isn't necessary anymore? https://codereview.adblockplus.org/29536764/diff/29560558/lib/options.js File lib/options.js (right): https://codereview.adblockplus.org/29536764/diff/29560558/lib/options.js#newc... lib/options.js:96: if (chrome.runtime.lastError) Nit: This logic would be slightly simpler when inverting the logic: if (!chrome.runtime.lastError) returnShowOptionsCall(optionsTab, callback); I wonder though, if this check is even necessary. https://codereview.adblockplus.org/29536764/diff/29560558/options.html File options.html (right): https://codereview.adblockplus.org/29536764/diff/29560558/options.html#newcode39 options.html:39: width: 100%; Isn't width+height redundant with top+bottom+left+right?
Patch Set 17: Remove ping and handle error https://codereview.adblockplus.org/29536764/diff/29556757/lib/notificationHel... File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29536764/diff/29556757/lib/notificationHel... lib/notificationHelper.js:127: showOptions(page => On 2017/10/02 01:47:04, Sebastian Noack wrote: > On 2017/09/30 13:49:52, Manish Jethani wrote: > > On 2017/09/30 02:38:43, Sebastian Noack wrote: > > > On 2017/09/27 11:20:55, Manish Jethani wrote: > > > > On 2017/09/27 01:38:35, Sebastian Noack wrote: > > > > > I didn't go through the code, but assuming all instanced of > showOptions() > > > with > > > > > callback are in the same form: > > > > > > > > > > showOptions(page => > > > > > { > > > > > page.sendMessage({ > > > > > type: "app.respond", > > > > > action: <some_action> > > > > > args: <some_args> > > > > > }); > > > > > }); > > > > > > > > > > It seems to make more sense to make showOptions() take "action" and > "args" > > > > > instead of a callback: > > > > > > > > > > showOptions([some_action[, some_args]]); > > > > > > > > In messageResponder.js we have this: > > > > > > > > ext.showOptions(() => > > > > { > > > > if (!message.action) > > > > return; > > > > > > > > sendMessage("app", message.action, ...message.args); > > > > }); > > > > > > > > That sendMessage function sends the message to all the listeners after > > running > > > > some conversion on the arguments. We could inline some of that code here, > > just > > > > for the options page, but I think still means that the options page would > > > > receive the message twice. > > > > > > The callback there seems to be dead code to me. I can only find one instance > > of > > > app.open in the whole code base, and message.action isn't set there. Or do I > > > miss something? If not, I suggest to just remove the callback from that > call. > > > > This callback is required for the abp:subscribe links. In fact, the issue > #5748 > > occurs because this callback is getting called too soon. > > > > > [...] > > abp:subscribe links are not handled through the app.open message, and my > suggestion was not to unsupport the scenario where there are additional > information to be send to the options page, but rather streamline it by passing > the data to be send to the options page rather than a callback with duplicated > boilerplate, see the first comment in this thread. Sorry, I referred to the wrong code. Here's the part from the "subscriptions.add" handler in messageResponder.js: ext.showOptions(() => { sendMessage("app", "addSubscription", subscription); }); So messageResponder.js would have to handle this itself, it cannot let showOptions send the message, for the reason I mentioned above. There's only one place in the entire code where the callback simply sends a message to the page, we would not be avoiding any duplication then by adding extra parameters to showOptions. https://codereview.adblockplus.org/29536764/diff/29560558/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29536764/diff/29560558/desktop-options.js#... desktop-options.js:749: $(() => On 2017/10/02 01:47:07, Sebastian Noack wrote: > So I guess this code isn't necessary anymore? Sorry about that, done. https://codereview.adblockplus.org/29536764/diff/29560558/lib/options.js File lib/options.js (right): https://codereview.adblockplus.org/29536764/diff/29560558/lib/options.js#newc... lib/options.js:96: if (chrome.runtime.lastError) On 2017/10/02 01:47:07, Sebastian Noack wrote: > Nit: This logic would be slightly simpler when inverting the logic: > > if (!chrome.runtime.lastError) > returnShowOptionsCall(optionsTab, callback); > > I wonder though, if this check is even necessary. Yeah, I don't think we should touch runtime.lastError unless we have to. In this case, we're trying to find the new tab anyway, if there's an error we won't be able to find it, and that should be the error condition. I've moved this to returnShowOptionsCall. https://codereview.adblockplus.org/29536764/diff/29560558/options.html File options.html (right): https://codereview.adblockplus.org/29536764/diff/29560558/options.html#newcode39 options.html:39: width: 100%; On 2017/10/02 01:47:07, Sebastian Noack wrote: > Isn't width+height redundant with top+bottom+left+right? We discussed this earlier above. Strictly speaking it is redundant, but it doesn't hurt to leave it there. https://codereview.adblockplus.org/29536764/diff/29562555/lib/options.js File lib/options.js (right): https://codereview.adblockplus.org/29536764/diff/29562555/lib/options.js#newc... lib/options.js:65: callback(); I think it's better to call the callback anyway with no page object, then let it deal with it (either explicitly ignore the error or let it crash), but let me know if you disagree.
https://codereview.adblockplus.org/29536764/diff/29560558/options.html File options.html (right): https://codereview.adblockplus.org/29536764/diff/29560558/options.html#newcode39 options.html:39: width: 100%; On 2017/10/02 08:03:14, Manish Jethani wrote: > On 2017/10/02 01:47:07, Sebastian Noack wrote: > > Isn't width+height redundant with top+bottom+left+right? > > We discussed this earlier above. Strictly speaking it is redundant, but it > doesn't hurt to leave it there. Well, the prior discussion seem to have been about the redundant styles on the body element, not about the width/height for #content. Anyway, the use case you are optimizing for there seems to be rather theoretical. Extensions cannot inject styles or code on pages in other extensions, and UA/user stylesheets that would break fixed positioning would break websites all over the place. Anyway, what would setting width and height to 100% here do any good, if fixed positioning isn't in effect and this and all surrounding element's accumulated margin/padding/border isn't 0? Note that the 100% applies to the parent's content, but width/height apply to the area enclosed by this element's border. https://codereview.adblockplus.org/29536764/diff/29562555/lib/options.js File lib/options.js (right): https://codereview.adblockplus.org/29536764/diff/29562555/lib/options.js#newc... lib/options.js:65: callback(); On 2017/10/02 08:03:14, Manish Jethani wrote: > I think it's better to call the callback anyway with no page object, then let it > deal with it (either explicitly ignore the error or let it crash), but let me > know if you disagree. I can think of two scenarios here: 1. chrome.runtime.openOptionsPage() or chrome.tabs.create() failed. This is a rather theoretical scenario, which I cannot see how it could happen other than a browser bug (or bug in our code here). Moreover, there is nothing the calling code could do about it. Also note that chrome.runtime.lastError wouldn't be set accordingly in this case, since the last API called is chrome.tabs.query(). 2. The user (or another extension) closed the options page again before chrome.tabs.query() completed. In this case doing nothing is the right thing to do, and either causing an error to be logged or requiring the calling code to check for the argument seems unnecessary. https://codereview.adblockplus.org/29536764/diff/29562555/lib/options.js#newc... lib/options.js:142: if (!["http:", "https:"].includes(currentPage.url.protocol)) You could just use a regular expression here (if you don't want to duplicate the right-hand side, which would be fine too). If you are concerned, about performance this shouldn't be any worse than creating an array on demand.
Patch Set 18: Do not callback if tab not found https://codereview.adblockplus.org/29536764/diff/29560558/options.html File options.html (right): https://codereview.adblockplus.org/29536764/diff/29560558/options.html#newcode39 options.html:39: width: 100%; On 2017/10/04 01:22:32, Sebastian Noack wrote: > On 2017/10/02 08:03:14, Manish Jethani wrote: > > On 2017/10/02 01:47:07, Sebastian Noack wrote: > > > Isn't width+height redundant with top+bottom+left+right? > > > > We discussed this earlier above. Strictly speaking it is redundant, but it > > doesn't hurt to leave it there. > > Well, the prior discussion seem to have been about the redundant styles on the > body element, not about the width/height for #content. Anyway, the use case you > are optimizing for there seems to be rather theoretical. Extensions cannot > inject styles or code on pages in other extensions, and UA/user stylesheets that > would break fixed positioning would break websites all over the place. > [...] I see, that discussion was about margin/padding. OK, I've removed these. https://codereview.adblockplus.org/29536764/diff/29562555/lib/options.js File lib/options.js (right): https://codereview.adblockplus.org/29536764/diff/29562555/lib/options.js#newc... lib/options.js:65: callback(); On 2017/10/04 01:22:32, Sebastian Noack wrote: > On 2017/10/02 08:03:14, Manish Jethani wrote: > > I think it's better to call the callback anyway with no page object, then let > it > > deal with it (either explicitly ignore the error or let it crash), but let me > > know if you disagree. > > I can think of two scenarios here: > > 1. chrome.runtime.openOptionsPage() or chrome.tabs.create() failed. This is a > rather theoretical scenario, which I cannot see how it could happen other than a > browser bug (or bug in our code here). Moreover, there is nothing the calling > code could do about it. Also note that chrome.runtime.lastError wouldn't be set > accordingly in this case, since the last API called is chrome.tabs.query(). > 2. The user (or another extension) closed the options page again before > chrome.tabs.query() completed. In this case doing nothing is the right thing to > do, and either causing an error to be logged or requiring the calling code to > check for the argument seems unnecessary. In general I think that a callback should be called even if there's an error, because there's often some cleanup code in callbacks. If this were a promise, for instance, we would reject it here, but with no "reason" argument. There is one instance where we'd want the callback to be called even if everything else failed, i.e. in popup.js when the Options menu item is clicked, otherwise the UI would seem unresponsive. Anyway, like you said, this is theoretical. I've removed the call to the callback now. https://codereview.adblockplus.org/29536764/diff/29562555/lib/options.js#newc... lib/options.js:142: if (!["http:", "https:"].includes(currentPage.url.protocol)) On 2017/10/04 01:22:33, Sebastian Noack wrote: > You could just use a regular expression here (if you don't want to duplicate the > right-hand side, which would be fine too). If you are concerned, about > performance this shouldn't be any worse than creating an array on demand. Done.
Patch Set 19: Set content size https://codereview.adblockplus.org/29536764/diff/29560558/options.html File options.html (right): https://codereview.adblockplus.org/29536764/diff/29560558/options.html#newcode39 options.html:39: width: 100%; On 2017/10/04 03:46:48, Manish Jethani wrote: > On 2017/10/04 01:22:32, Sebastian Noack wrote: > > On 2017/10/02 08:03:14, Manish Jethani wrote: > > > On 2017/10/02 01:47:07, Sebastian Noack wrote: > > > > Isn't width+height redundant with top+bottom+left+right? > > > > > > We discussed this earlier above. Strictly speaking it is redundant, but it > > > doesn't hurt to leave it there. > > > > Well, the prior discussion seem to have been about the redundant styles on the > > body element, not about the width/height for #content. Anyway, the use case > you > > are optimizing for there seems to be rather theoretical. Extensions cannot > > inject styles or code on pages in other extensions, and UA/user stylesheets > that > > would break fixed positioning would break websites all over the place. > > [...] > > I see, that discussion was about margin/padding. > > OK, I've removed these. Sorry, I was wrong. We do need width and height. It is right and bottom that are redundant here. When both left and right are specified, that does not influence the width of the element, only one of them is used (depending on whether it's left-to-right or right-to-left). Likewise for top and bottom.
Looks good to me except for one small nit. Thanks! https://codereview.adblockplus.org/29536764/diff/29560558/options.html File options.html (right): https://codereview.adblockplus.org/29536764/diff/29560558/options.html#newcode39 options.html:39: width: 100%; On 2017/10/04 03:57:15, Manish Jethani wrote: > On 2017/10/04 03:46:48, Manish Jethani wrote: > > On 2017/10/04 01:22:32, Sebastian Noack wrote: > > > On 2017/10/02 08:03:14, Manish Jethani wrote: > > > > On 2017/10/02 01:47:07, Sebastian Noack wrote: > > > > > Isn't width+height redundant with top+bottom+left+right? > > > > > > > > We discussed this earlier above. Strictly speaking it is redundant, but it > > > > doesn't hurt to leave it there. > > > > > > Well, the prior discussion seem to have been about the redundant styles on > the > > > body element, not about the width/height for #content. Anyway, the use case > > you > > > are optimizing for there seems to be rather theoretical. Extensions cannot > > > inject styles or code on pages in other extensions, and UA/user stylesheets > > that > > > would break fixed positioning would break websites all over the place. > > > [...] > > > > I see, that discussion was about margin/padding. > > > > OK, I've removed these. > > Sorry, I was wrong. We do need width and height. It is right and bottom that are > redundant here. > > When both left and right are specified, that does not influence the width of the > element, only one of them is used (depending on whether it's left-to-right or > right-to-left). Likewise for top and bottom. Oh, I remember that differently, as well. Good to know. https://codereview.adblockplus.org/29536764/diff/29562555/lib/options.js File lib/options.js (right): https://codereview.adblockplus.org/29536764/diff/29562555/lib/options.js#newc... lib/options.js:65: callback(); On 2017/10/04 03:46:49, Manish Jethani wrote: > On 2017/10/04 01:22:32, Sebastian Noack wrote: > > On 2017/10/02 08:03:14, Manish Jethani wrote: > > > I think it's better to call the callback anyway with no page object, then > let > > it > > > deal with it (either explicitly ignore the error or let it crash), but let > me > > > know if you disagree. > > > > I can think of two scenarios here: > > > > 1. chrome.runtime.openOptionsPage() or chrome.tabs.create() failed. This is a > > rather theoretical scenario, which I cannot see how it could happen other than > a > > browser bug (or bug in our code here). Moreover, there is nothing the calling > > code could do about it. Also note that chrome.runtime.lastError wouldn't be > set > > accordingly in this case, since the last API called is chrome.tabs.query(). > > 2. The user (or another extension) closed the options page again before > > chrome.tabs.query() completed. In this case doing nothing is the right thing > to > > do, and either causing an error to be logged or requiring the calling code to > > check for the argument seems unnecessary. > > In general I think that a callback should be called even if there's an error, > because there's often some cleanup code in callbacks. If this were a promise, > for instance, we would reject it here, but with no "reason" argument. There is > one instance where we'd want the callback to be called even if everything else > failed, i.e. in popup.js when the Options menu item is clicked, otherwise the UI > would seem unresponsive. > > Anyway, like you said, this is theoretical. I've removed the call to the > callback now. This would rather be an argument to return a Promise here. So that code that wouldn't handle the error anyway, could just register a success callback and in the unlikely case of an error it would just get propagated up the promise chain, or logged if this is the end of the chain. But still, if the options page just gets closed before this code is reached, this isn't an error, but any further processing would lead to one. Anyway, I think what we have now is good enough. https://codereview.adblockplus.org/29536764/diff/29563625/lib/options.js File lib/options.js (right): https://codereview.adblockplus.org/29536764/diff/29563625/lib/options.js#newc... lib/options.js:63: // This likely means that there was an error. I think this comment should rather be: This means that there was either an error or that the options page has been closed right away. Also, I'd put the comment above the if statement, and then remove the braces.
Patch Set 20: Remove redundant top and left properties https://codereview.adblockplus.org/29536764/diff/29562555/lib/options.js File lib/options.js (right): https://codereview.adblockplus.org/29536764/diff/29562555/lib/options.js#newc... lib/options.js:65: callback(); On 2017/10/04 05:33:14, Sebastian Noack wrote: > On 2017/10/04 03:46:49, Manish Jethani wrote: > > On 2017/10/04 01:22:32, Sebastian Noack wrote: > > > On 2017/10/02 08:03:14, Manish Jethani wrote: > > > > I think it's better to call the callback anyway with no page object, then > > let > > > it > > > > deal with it (either explicitly ignore the error or let it crash), but let > > me > > > > know if you disagree. > > > > > > I can think of two scenarios here: > > > > > > 1. chrome.runtime.openOptionsPage() or chrome.tabs.create() failed. This is > a > > > rather theoretical scenario, which I cannot see how it could happen other > than > > a > > > browser bug (or bug in our code here). Moreover, there is nothing the > calling > > > code could do about it. Also note that chrome.runtime.lastError wouldn't be > > set > > > accordingly in this case, since the last API called is chrome.tabs.query(). > > > 2. The user (or another extension) closed the options page again before > > > chrome.tabs.query() completed. In this case doing nothing is the right thing > > to > > > do, and either causing an error to be logged or requiring the calling code > to > > > check for the argument seems unnecessary. > > > > In general I think that a callback should be called even if there's an error, > > because there's often some cleanup code in callbacks. If this were a promise, > > for instance, we would reject it here, but with no "reason" argument. There is > > one instance where we'd want the callback to be called even if everything else > > failed, i.e. in popup.js when the Options menu item is clicked, otherwise the > UI > > would seem unresponsive. > > > > Anyway, like you said, this is theoretical. I've removed the call to the > > callback now. > > This would rather be an argument to return a Promise here. So that code that > wouldn't handle the error anyway, could just register a success callback and in > the unlikely case of an error it would just get propagated up the promise chain, > or logged if this is the end of the chain. But still, if the options page just > gets closed before this code is reached, this isn't an error, but any further > processing would lead to one. Anyway, I think what we have now is good enough. Acknowledged. https://codereview.adblockplus.org/29536764/diff/29563625/lib/options.js File lib/options.js (right): https://codereview.adblockplus.org/29536764/diff/29563625/lib/options.js#newc... lib/options.js:63: // This likely means that there was an error. On 2017/10/04 05:33:15, Sebastian Noack wrote: > I think this comment should rather be: > > This means that there was either an error or that the > options page has been closed right away. > > Also, I'd put the comment above the if statement, and then remove the braces. OK, I'd rather just lose that comment. Done. https://codereview.adblockplus.org/29536764/diff/29563625/options.html File options.html (right): https://codereview.adblockplus.org/29536764/diff/29563625/options.html#newcode35 options.html:35: top: 0; Actually even top and left are redundant since this is the topmost element on the page and the page itself has no margins or padding.
Ollie, would you like to take a look, esp. in lib/options.js where it affects Edge as well?
LGTM
LGTM On 2017/10/04 13:07:22, Manish Jethani wrote: > Ollie, would you like to take a look, esp. in lib/options.js where it affects > Edge as well? Ollie is on vacation. However, since the builds for Microsoft Edge are still released out of a bookmark, we have to merge these changes manually, anyway, in order to get them into the builds for Microsoft Edge. So Ollie can just retest this logic once he merges "master" back onto the "edge" bookmark. So feel free to move Ollie to CC, and push these changes.
Patch Set 21: Rebase One change. https://codereview.adblockplus.org/29536764/diff/29564724/popup.js File popup.js (right): https://codereview.adblockplus.org/29536764/diff/29564724/popup.js#newcode125 popup.js:125: chrome.runtime.sendMessage({type: "app.open", what: "options"}); Since we no longer have require in the popup, we can't call showOptions directly; instead we use the "app.open" message to the background page. We'll also have to make corresponding changes in adblockplusui and update the dependency here.
https://codereview.adblockplus.org/29536764/diff/29564724/popup.js File popup.js (right): https://codereview.adblockplus.org/29536764/diff/29564724/popup.js#newcode126 popup.js:126: window.close(); Did you test whether the message is still received by the background page, and the options is opened reliably, both on Chrome and Firefox, if we close this document as soon as the message is sent? If so, LGTM.
Patch Set 22: Hide content frame until loaded https://codereview.adblockplus.org/29536764/diff/29564724/popup.js File popup.js (right): https://codereview.adblockplus.org/29536764/diff/29564724/popup.js#newcode126 popup.js:126: window.close(); On 2017/10/04 21:38:49, Sebastian Noack wrote: > Did you test whether the message is still received by the background page, and > the options is opened reliably, both on Chrome and Firefox, if we close this > document as soon as the message is sent? If so, LGTM. Yes, this works reliably. https://codereview.adblockplus.org/29536764/diff/29564756/options.js File options.js (right): https://codereview.adblockplus.org/29536764/diff/29564756/options.js#newcode28 options.js:28: chrome.runtime.sendMessage({ After this change, we start to load the content a little bit after the top frame's DOMContentLoaded. This causes the entire loading process to be visible to the user: first the HTML is loaded (with no styling) [1], then the styles are loaded. I think this looks bad and we should avoid it, especially because it's the result of this iframe hack. [1]: https://en.wikipedia.org/wiki/Flash_of_unstyled_content
https://codereview.adblockplus.org/29536764/diff/29564756/options.js File options.js (right): https://codereview.adblockplus.org/29536764/diff/29564756/options.js#newcode28 options.js:28: chrome.runtime.sendMessage({ On 2017/10/04 22:03:51, Manish Jethani wrote: > After this change, we start to load the content a little bit after the top > frame's DOMContentLoaded. This causes the entire loading process to be visible > to the user: first the HTML is loaded (with no styling) [1], then the styles are > loaded. I think this looks bad and we should avoid it, especially because it's > the result of this iframe hack. > > [1]: https://en.wikipedia.org/wiki/Flash_of_unstyled_content Just to clarify, that's the point of Patch Set 22. If we show the content only after the frame's onload, the user doesn't see the FOUC.
https://codereview.adblockplus.org/29536764/diff/29564756/options.html File options.html (right): https://codereview.adblockplus.org/29536764/diff/29564756/options.html#newcode42 options.html:42: <iframe id="content" style="visibility: hidden" data-src="desktop-options.html" data-src-fennec="mobile-options.html"></iframe> Using style attributes isn't great. A more semantic (better) way to achieve the same seems to be the "hidden" attribute: https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/hidden
https://codereview.adblockplus.org/29536764/diff/29564756/options.html File options.html (right): https://codereview.adblockplus.org/29536764/diff/29564756/options.html#newcode42 options.html:42: <iframe id="content" style="visibility: hidden" data-src="desktop-options.html" data-src-fennec="mobile-options.html"></iframe> On 2017/10/04 22:36:19, Sebastian Noack wrote: > Using style attributes isn't great. A more semantic (better) way to achieve the > same seems to be the "hidden" attribute: > https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/hidden The hidden attribute doesn't seem to be as effective in avoiding the flash of unstyled content. I'm guessing this is because it has different semantics. It's more like setting the "display" style to "none". "hidden" is semantic: https://html.spec.whatwg.org/multipage/interaction.html#the-hidden-attribute It just so happens that browsers hide "hidden" elements by default. If you override the "display" style of a "hidden" element by setting it to "block", it'll be visible again (even though its Element.hidden will still be true). Anyway, we don't want to remove the element from the document's layout, and we don't want to hide the element from screen readers and such, which is what setting "hidden" would do. We only want to hide the element visually (not semantically), in this case setting the style is in fact the correct way to do it.
LGTM
https://codereview.adblockplus.org/29536764/diff/29564756/options.js File options.js (right): https://codereview.adblockplus.org/29536764/diff/29564756/options.js#newcode28 options.js:28: chrome.runtime.sendMessage({ On 2017/10/04 22:03:51, Manish Jethani wrote: > After this change, we start to load the content a little bit after the top > frame's DOMContentLoaded. This causes the entire loading process to be visible > to the user: first the HTML is loaded (with no styling) [1], then the styles are > loaded. I think this looks bad and we should avoid it, especially because it's > the result of this iframe hack. > > [1]: https://en.wikipedia.org/wiki/Flash_of_unstyled_content This workaround seems to be only necessary due to the way desktop-options.html is currently implemented. Therefore we should either ignore it since it's an issue that's unrelated to the integration of mobile-options.html or at least add a comment to clarify this fact so that we can get rid of it as soon as we replace desktop-options.html with the new options page.
https://codereview.adblockplus.org/29536764/diff/29564756/options.js File options.js (right): https://codereview.adblockplus.org/29536764/diff/29564756/options.js#newcode28 options.js:28: chrome.runtime.sendMessage({ On 2017/10/05 11:51:43, Thomas Greiner wrote: > On 2017/10/04 22:03:51, Manish Jethani wrote: > > After this change, we start to load the content a little bit after the top > > frame's DOMContentLoaded. This causes the entire loading process to be visible > > to the user: first the HTML is loaded (with no styling) [1], then the styles > are > > loaded. I think this looks bad and we should avoid it, especially because it's > > the result of this iframe hack. > > > > [1]: https://en.wikipedia.org/wiki/Flash_of_unstyled_content > > This workaround seems to be only necessary due to the way desktop-options.html > is currently implemented. Therefore we should either ignore it since it's an > issue that's unrelated to the integration of mobile-options.html or at least add > a comment to clarify this fact so that we can get rid of it as soon as we > replace desktop-options.html with the new options page. Would you say that this workaround, if it is necessary at all (since we're moving to the new options page), rather belongs in desktop-options.html? I noticed this problem does not occur on mobile, maybe we should drop this workaround in that case.
https://codereview.adblockplus.org/29536764/diff/29564756/options.js File options.js (right): https://codereview.adblockplus.org/29536764/diff/29564756/options.js#newcode28 options.js:28: chrome.runtime.sendMessage({ On 2017/10/05 12:14:25, Manish Jethani wrote: > On 2017/10/05 11:51:43, Thomas Greiner wrote: > > On 2017/10/04 22:03:51, Manish Jethani wrote: > > > After this change, we start to load the content a little bit after the top > > > frame's DOMContentLoaded. This causes the entire loading process to be > visible > > > to the user: first the HTML is loaded (with no styling) [1], then the styles > > are > > > loaded. I think this looks bad and we should avoid it, especially because > it's > > > the result of this iframe hack. > > > > > > [1]: https://en.wikipedia.org/wiki/Flash_of_unstyled_content > > > > This workaround seems to be only necessary due to the way desktop-options.html > > is currently implemented. Therefore we should either ignore it since it's an > > issue that's unrelated to the integration of mobile-options.html or at least > add > > a comment to clarify this fact so that we can get rid of it as soon as we > > replace desktop-options.html with the new options page. > > Would you say that this workaround, if it is necessary at all (since we're > moving to the new options page), rather belongs in desktop-options.html? Probably, because this issue can be reproduced in the current version of the extension we should've fixed it earlier but seems like nobody noticed it until now. I'd say that it's not worth fixing it anymore since we're going to get rid of the current options page before the next release. > I noticed this problem does not occur on mobile, maybe we should drop this > workaround in that case. Yep, it's likely related to jQuery which we're not using in either the mobile or the new options page so the workaround won't be necessary.
Patch Set 23: Remove workaround for FOUC issue and update adblockplusui dependency https://codereview.adblockplus.org/29536764/diff/29564756/options.js File options.js (right): https://codereview.adblockplus.org/29536764/diff/29564756/options.js#newcode28 options.js:28: chrome.runtime.sendMessage({ On 2017/10/05 12:34:36, Thomas Greiner wrote: > On 2017/10/05 12:14:25, Manish Jethani wrote: > > On 2017/10/05 11:51:43, Thomas Greiner wrote: > > > On 2017/10/04 22:03:51, Manish Jethani wrote: > > > > After this change, we start to load the content a little bit after the top > > > > frame's DOMContentLoaded. This causes the entire loading process to be > > visible > > > > to the user: first the HTML is loaded (with no styling) [1], then the > styles > > > are > > > > loaded. I think this looks bad and we should avoid it, especially because > > it's > > > > the result of this iframe hack. > > > > > > > > [1]: https://en.wikipedia.org/wiki/Flash_of_unstyled_content > > > > > > This workaround seems to be only necessary due to the way > desktop-options.html > > > is currently implemented. Therefore we should either ignore it since it's an > > > issue that's unrelated to the integration of mobile-options.html or at least > > add > > > a comment to clarify this fact so that we can get rid of it as soon as we > > > replace desktop-options.html with the new options page. > > > > Would you say that this workaround, if it is necessary at all (since we're > > moving to the new options page), rather belongs in desktop-options.html? > > Probably, because this issue can be reproduced in the current version of the > extension we should've fixed it earlier but seems like nobody noticed it until > now. > I'd say that it's not worth fixing it anymore since we're going to get rid of > the current options page before the next release. > > > I noticed this problem does not occur on mobile, maybe we should drop this > > workaround in that case. > > Yep, it's likely related to jQuery which we're not using in either the mobile or > the new options page so the workaround won't be necessary. Done.
LGTM
LGTM
Thanks, folks!
Message was sent while issue was closed.
https://codereview.adblockplus.org/29536764/diff/29565705/options.js File options.js (right): https://codereview.adblockplus.org/29536764/diff/29565705/options.js#newcode27 options.js:27: chrome.runtime.sendMessage({ Edge does not support the 'chrome' namespace for extensions. We should map to 'browser' here instead.
Message was sent while issue was closed.
https://codereview.adblockplus.org/29536764/diff/29565705/options.js File options.js (right): https://codereview.adblockplus.org/29536764/diff/29565705/options.js#newcode27 options.js:27: chrome.runtime.sendMessage({ On 2017/10/09 11:49:59, Oleksandr wrote: > Edge does not support the 'chrome' namespace for extensions. We should map to > 'browser' here instead. Thanks.
Message was sent while issue was closed.
https://codereview.adblockplus.org/29536764/diff/29565705/options.js File options.js (right): https://codereview.adblockplus.org/29536764/diff/29565705/options.js#newcode27 options.js:27: chrome.runtime.sendMessage({ On 2017/10/09 12:00:55, Manish Jethani wrote: > On 2017/10/09 11:49:59, Oleksandr wrote: > > Edge does not support the 'chrome' namespace for extensions. We should map to > > 'browser' here instead. > > Thanks. Note that we already do so in ext/common.js: https://hg.adblockplus.org/adblockpluschrome/file/7136ad8e321d/ext/common.js#l22 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
