Issue 29458601: Issue 5315 - Add support for Microsoft Edge (Closed)
|
Created:
June 7, 2017, 12:41 p.m. by Oleksandr Modified:
July 28, 2017, 4:08 p.m. Visibility:
Public. |
DescriptionGraft required patches from edge bookmark into master
Patch Set 1 #
Total comments: 24
Patch Set 2 : Rebase the README.md, compress images, adress the comments #
Total comments: 7
Patch Set 3 : Handle webRequest.ResourceType (#5321) and RTCPeerConnection.generateCertificate, address comments #
Total comments: 8
Patch Set 4 : Make copyProperties more generic #
Total comments: 7
Patch Set 5 : Address the nits #
Total comments: 1
Patch Set 6 : Make checks for 'chrome' object existence consistent #
Total comments: 3
Patch Set 7 : Future proof in case Edge supports OBJECT request interception #Patch Set 8 : Workaround CSS.excape, i18n issue and use typeof for 'undefined' detection #
Total comments: 13
Patch Set 9 : Use messaging for CSS escaping #
Total comments: 2
Patch Set 10 : Use quoteCSS. Rephrase the comment. #Patch Set 11 : Rephrase the comment #Patch Set 12 : Rebase to current tip #
Total comments: 4
Patch Set 13 : Cosmetic changes #
MessagesTotal messages: 39
This is meant to be pushed into master of adblockpluschrome.
If this is all what it takes meanwhile to support Microsoft Edge, I agree to squash these changes, and merge them upstream. https://codereview.adblockplus.org/29458601/diff/29458602/README.md File README.md (right): https://codereview.adblockplus.org/29458601/diff/29458602/README.md#newcode1 README.md:1: Adblock Plus for Chrome, Opera and Edge I wonder whether we should spell out "Micorosft Edge" as it is not as well known as Chrome and Opera yet. Also we already seem to that everywhere else. https://codereview.adblockplus.org/29458601/diff/29458602/README.md#newcode26 README.md:26: ./build.py -t edge build Perhaps we should add a note below, that the generated build will be unsigned, hence is only useful for upload to the Windows Store, but won't load as-is in Microsoft Edge. https://codereview.adblockplus.org/29458601/diff/29458602/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29458601/diff/29458602/ext/common.js#newco... ext/common.js:25: window.chrome = window.browser; Firefox actually has the "chrome" namespace as well, and we use it there, but it has some minor inconsistencies with "browser". So this change would potentially change behavior on Firefox. At some point we should, use "browser" everywhere (and alias "chrome" to "browser for Chrome). But for now, to keep this change as simple and as least intrusive as possible, just check for the absence of "chrome" instead. https://codereview.adblockplus.org/29458601/diff/29458602/ext/popup.js File ext/popup.js (right): https://codereview.adblockplus.org/29458601/diff/29458602/ext/popup.js#newcode6 ext/popup.js:6: (typeof chrome.extension == "undefined")) Why is explicit checking for chrome.extension necessary? https://codereview.adblockplus.org/29458601/diff/29458602/ext/popup.js#newcode7 ext/popup.js:7: chrome = browser; Is this variable meant to be global? If so, we should probably assign to window.chrome like we do elsewhere. If we only need it for the line below, consider using a local variable. https://codereview.adblockplus.org/29458601/diff/29458602/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29458601/diff/29458602/lib/prefs.js#newcod... lib/prefs.js:277: if (typeof chrome == "object" && "managed" in chrome.storage) Won't typeof chrome == "object" always evaluate to true? https://codereview.adblockplus.org/29458601/diff/29458602/metadata.edge File metadata.edge (right): https://codereview.adblockplus.org/29458601/diff/29458602/metadata.edge#newco... metadata.edge:14: windows = 10.0.14332.0/12.0.0.0 Does the build number has been updated yet to reflect the corresponding minimum version that is actually required to have these combination of changes work? You said, you want to do it differently, but if we squash all of these changes anyway, I guess, we should as well avoid an inconsistent state here. https://codereview.adblockplus.org/29458601/diff/29458602/metadata.edge#newco... metadata.edge:19: logo_150.png = icons/abp-150.png Since I cannot comment on binary files, I do so here: The file size of these images can be optimized using zopflipng: https://adblockplus.org/coding-style#general The Debian/Ubuntu package is named "zopfli", however, "zopflipng" was just added to the package recently. But you can download the latest package from here and install it with "dpkg -i": http://ftp.debian.org/debian/pool/main/z/zopfli/zopfli_1.0.1+git160527-1_amd6...
> If this is all what it takes meanwhile to support Microsoft > Edge, I agree to squash these changes, and merge them > upstream. Me too, they aren't as bad as I expected. I don't think this should be a Noissue, since this will be landing in the master branch we need a proper issue for it which explains that we're adding Edge support for real. Its important anyway, but also since the changes touch a few different places in the code which aren't too obvious the QA team will need some testing hints to ensure we didn't cause a regression on Chrome for example. I also think the commit message should be something more like "Issue XXX - Add support for Microsoft Edge" since the edge bookmark is eventually going away, this commit will be what's left. Also I remember there being workarounds needed for the icon animations, since Edge didn't support the same sizes as modern versions of Chrome and therefore threw. Do we not have that problem any more? Finally I think this probably shouldn't land until after the release if we're still planning to do a release soon, what do you think Sebastian? https://codereview.adblockplus.org/29458601/diff/29458602/README.md File README.md (right): https://codereview.adblockplus.org/29458601/diff/29458602/README.md#newcode1 README.md:1: Adblock Plus for Chrome, Opera and Edge On 2017/06/07 14:42:14, Sebastian Noack wrote: > I wonder whether we should spell out "Micorosft Edge" as it is not as well known > as Chrome and Opera yet. Also we already seem to that everywhere else. FWIW I agree. https://codereview.adblockplus.org/29458601/diff/29458602/README.md#newcode22 README.md:22: Run one of the following commands in the project directory, depending on your Note: If the README changes for Firefox webext[1] land first you'll need to rebase this again. [1] https://codereview.adblockplus.org/29458635/ https://codereview.adblockplus.org/29458601/diff/29458602/README.md#newcode41 README.md:41: This will create a _devenv.platform_ directory in the repository. In Chrome you Nit: Blank line before "In Chrome"? I figure the first sentence applies to all platforms, but the following paragraphs are platform specific. https://codereview.adblockplus.org/29458601/diff/29458602/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29458601/diff/29458602/lib/prefs.js#newcod... lib/prefs.js:277: if (typeof chrome == "object" && "managed" in chrome.storage) On 2017/06/07 14:42:14, Sebastian Noack wrote: > Won't typeof chrome == "object" always evaluate to true? I thought so too, but maybe we're missing something Also Ollie, not totally relevant, and you probably already know this, but `typeof null` is "object", but `null.storage` would result in an exception. Just mentioning it since I found that surprising in the past.
https://codereview.adblockplus.org/29458601/diff/29458602/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29458601/diff/29458602/lib/prefs.js#newcod... lib/prefs.js:277: if (typeof chrome == "object" && "managed" in chrome.storage) On 2017/06/08 10:50:27, kzar wrote: > On 2017/06/07 14:42:14, Sebastian Noack wrote: > > Won't typeof chrome == "object" always evaluate to true? > > I thought so too, but maybe we're missing something Most likely, when this change was intially authored, upstream code still supported Safari in which case this check would made sense, but not anymore. > Also Ollie, not totally relevant, and you probably already know this, but > `typeof null` is "object", but `null.storage` would result in an exception. Just > mentioning it since I found that surprising in the past. I think it it is safe to assume that if "chrome" is defined that it is not null. Somebody would have to deliberately put "chrome = null" somewhere in the code, to cause this to happen, which would not only produce failures here.
https://codereview.adblockplus.org/29458601/diff/29458602/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29458601/diff/29458602/lib/prefs.js#newcod... lib/prefs.js:277: if (typeof chrome == "object" && "managed" in chrome.storage) On 2017/06/08 10:59:19, Sebastian Noack wrote: > I think it it is safe to assume that if "chrome" is defined that > it is not null. Yea, that's true. Just thought I'd mention it since I found it surprising that null is considered an object in the past.
https://codereview.adblockplus.org/29458601/diff/29458602/README.md File README.md (right): https://codereview.adblockplus.org/29458601/diff/29458602/README.md#newcode1 README.md:1: Adblock Plus for Chrome, Opera and Edge On 2017/06/07 14:42:14, Sebastian Noack wrote: > I wonder whether we should spell out "Micorosft Edge" as it is not as well known > as Chrome and Opera yet. Also we already seem to that everywhere else. Done. https://codereview.adblockplus.org/29458601/diff/29458602/README.md#newcode26 README.md:26: ./build.py -t edge build On 2017/06/07 14:42:14, Sebastian Noack wrote: > Perhaps we should add a note below, that the generated build will be unsigned, > hence is only useful for upload to the Windows Store, but won't load as-is in > Microsoft Edge. Done. https://codereview.adblockplus.org/29458601/diff/29458602/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29458601/diff/29458602/ext/common.js#newco... ext/common.js:25: window.chrome = window.browser; On 2017/06/07 14:42:14, Sebastian Noack wrote: > Firefox actually has the "chrome" namespace as well, and we use it there, but it > has some minor inconsistencies with "browser". So this change would potentially > change behavior on Firefox. > > At some point we should, use "browser" everywhere (and alias "chrome" to > "browser for Chrome). But for now, to keep this change as simple and as least > intrusive as possible, just check for the absence of "chrome" instead. There is actually a `chrome` object defined in Edge. It has only one property 'app' and nothing else. I don't know what it is for, but I think it wouldn't hurt preserving it. https://codereview.adblockplus.org/29458601/diff/29458602/ext/popup.js File ext/popup.js (right): https://codereview.adblockplus.org/29458601/diff/29458602/ext/popup.js#newcode6 ext/popup.js:6: (typeof chrome.extension == "undefined")) On 2017/06/07 14:42:14, Sebastian Noack wrote: > Why is explicit checking for chrome.extension necessary? There is a 'chrome' object defined in Edge, which has only one property 'app'. https://codereview.adblockplus.org/29458601/diff/29458602/ext/popup.js#newcode7 ext/popup.js:7: chrome = browser; On 2017/06/07 14:42:14, Sebastian Noack wrote: > Is this variable meant to be global? If so, we should probably assign to > window.chrome like we do elsewhere. If we only need it for the line below, > consider using a local variable. Done. https://codereview.adblockplus.org/29458601/diff/29458602/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29458601/diff/29458602/lib/prefs.js#newcod... lib/prefs.js:277: if (typeof chrome == "object" && "managed" in chrome.storage) On 2017/06/08 11:37:43, kzar wrote: > On 2017/06/08 10:59:19, Sebastian Noack wrote: > > I think it it is safe to assume that if "chrome" is defined that > > it is not null. > > Yea, that's true. Just thought I'd mention it since I found it surprising that > null is considered an object in the past. Yes. This was for issue #3705. It doesn't look like this change is needed indeed. require("info").platform in Edge is 'edgehtml', and chrome.storage.managed is undefined. Maybe even the platform check is redundant. https://codereview.adblockplus.org/29458601/diff/29458602/metadata.edge File metadata.edge (right): https://codereview.adblockplus.org/29458601/diff/29458602/metadata.edge#newco... metadata.edge:14: windows = 10.0.14332.0/12.0.0.0 On 2017/06/07 14:42:14, Sebastian Noack wrote: > Does the build number has been updated yet to reflect the corresponding minimum > version that is actually required to have these combination of changes work? You > said, you want to do it differently, but if we squash all of these changes > anyway, I guess, we should as well avoid an inconsistent state here. Updated the build number. Also extension_id_release, as described in #4616. https://codereview.adblockplus.org/29458601/diff/29458602/metadata.edge#newco... metadata.edge:19: logo_150.png = icons/abp-150.png On 2017/06/07 14:42:14, Sebastian Noack wrote: > Since I cannot comment on binary files, I do so here: The file size of these > images can be optimized using zopflipng: > https://adblockplus.org/coding-style#general > > The Debian/Ubuntu package is named "zopfli", however, "zopflipng" was just added > to the package recently. But you can download the latest package from here and > install it with "dpkg -i": > http://ftp.debian.org/debian/pool/main/z/zopfli/zopfli_1.0.1+git160527-1_amd6... Done.
Didn't you mention, that usage of webRequest.ResourceType has to be adapted as well? https://codereview.adblockplus.org/29458601/diff/29458602/ext/popup.js File ext/popup.js (right): https://codereview.adblockplus.org/29458601/diff/29458602/ext/popup.js#newcode6 ext/popup.js:6: (typeof chrome.extension == "undefined")) On 2017/06/14 03:41:01, Oleksandr wrote: > On 2017/06/07 14:42:14, Sebastian Noack wrote: > > Why is explicit checking for chrome.extension necessary? > > There is a 'chrome' object defined in Edge, which has only one property 'app'. That's why we only check for `typeof chrome.extension` in ext/common.js, now, I guess. Can we perhaps make the check here consistent? https://codereview.adblockplus.org/29458601/diff/29458602/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29458601/diff/29458602/lib/prefs.js#newcod... lib/prefs.js:277: if (typeof chrome == "object" && "managed" in chrome.storage) On 2017/06/14 03:41:01, Oleksandr wrote: > On 2017/06/08 11:37:43, kzar wrote: > > On 2017/06/08 10:59:19, Sebastian Noack wrote: > > > I think it it is safe to assume that if "chrome" is defined that > > > it is not null. > > > > Yea, that's true. Just thought I'd mention it since I found it surprising that > > null is considered an object in the past. > > Yes. This was for issue #3705. It doesn't look like this change is needed > indeed. require("info").platform in Edge is 'edgehtml', and > chrome.storage.managed is undefined. Maybe even the platform check is redundant. Yeah, please remove the platform check, so that should Microsoft Edge (or Firefox), ever support storage.managed in the future, it will just work. https://codereview.adblockplus.org/29458601/diff/29464713/README.md File README.md (left): https://codereview.adblockplus.org/29458601/diff/29464713/README.md#oldcode2 README.md:2: ========================================== Nit: Please use as many equal signs here as the line above is long. While this isn't strictly necessary in markdown, it makes it easier to read as plain text. https://codereview.adblockplus.org/29458601/diff/29464713/README.md File README.md (right): https://codereview.adblockplus.org/29458601/diff/29464713/README.md#newcode32 README.md:32: ./build.py -t edge build Nit: In the human-readable description we refer to Microsoft Edge first, then Firefox. With the sample commands here and below, Firefox is mentioned first, then Microsoft Edge. Please make it consistent. https://codereview.adblockplus.org/29458601/diff/29464713/README.md#newcode35 README.md:35: _adblockpluschrome-1.2.3.nnnn.crx_ or _adblockplusfirefox-1.2.3.nnnn.xpi_ or Nit: Don't use multiple "or", use "a, b or c" instead. https://codereview.adblockplus.org/29458601/diff/29464713/README.md#newcode60 README.md:60: under _about:debugging_ in Firefox or in Extensions menu in Edge, after enabling "... in the _Extensions_ menu in Microsoft Edge ..." https://codereview.adblockplus.org/29458601/diff/29464713/README.md#newcode65 README.md:65: Edge build does not automatically detect changes, so after rebuilding the "Builds for Microsoft Edge do not ..." https://codereview.adblockplus.org/29458601/diff/29464713/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29458601/diff/29464713/ext/common.js#newco... ext/common.js:29: if (typeof chrome.app == "undefined") We don't use chrome.app anyway, so I guess we can simplify this code by removing the 3 lines that copy it over.
https://codereview.adblockplus.org/29458601/diff/29464713/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29458601/diff/29464713/ext/common.js#newco... ext/common.js:29: if (typeof chrome.app == "undefined") On 2017/06/14 05:13:39, Sebastian Noack wrote: > We don't use chrome.app anyway, so I guess we can simplify this code by removing > the 3 lines that copy it over. Done. https://codereview.adblockplus.org/29458601/diff/29466594/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29458601/diff/29466594/ext/common.js#newco... ext/common.js:25: if ((typeof chrome == "undefined") || 'chrome' is undefined in content script, but in backgroind page it is an object with one 'app' parameter. That's why both checks are needed.
https://codereview.adblockplus.org/29458601/diff/29466594/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29458601/diff/29466594/ext/common.js#newco... ext/common.js:25: if ((typeof chrome == "undefined") || Nit: The inner parenthesis are redundant. https://codereview.adblockplus.org/29458601/diff/29466594/inject.preload.js File inject.preload.js (right): https://codereview.adblockplus.org/29458601/diff/29466594/inject.preload.js#n... inject.preload.js:378: // NOTE: Edge does not support generateCertificate But what is about the other properties copied here? https://codereview.adblockplus.org/29458601/diff/29466594/lib/requestBlocker.js File lib/requestBlocker.js (left): https://codereview.adblockplus.org/29458601/diff/29466594/lib/requestBlocker.... lib/requestBlocker.js:33: if (!("OBJECT_SUBREQUEST" in chrome.webRequest.ResourceType)) This check was necessary to account for Firefox which distinguishes these requests. (The comment below is wrong.)
https://codereview.adblockplus.org/29458601/diff/29466594/inject.preload.js File inject.preload.js (right): https://codereview.adblockplus.org/29458601/diff/29466594/inject.preload.js#n... inject.preload.js:378: // NOTE: Edge does not support generateCertificate On 2017/06/19 11:07:02, Sebastian Noack wrote: > But what is about the other properties copied here? Yea, this is not good. Please instead modify the copyProperties function to check if the property exists. Something like this (untested): function copyProperties(src, dest, properties) { for (let name of properties) { if (src.hasOwnProperty(name)) { Object.defineProperty(dest, name, Object.getOwnPropertyDescriptor(src, name)); } } }
https://codereview.adblockplus.org/29458601/diff/29466594/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29458601/diff/29466594/ext/common.js#newco... ext/common.js:25: if ((typeof chrome == "undefined") || On 2017/06/19 11:07:02, Sebastian Noack wrote: > Nit: The inner parenthesis are redundant. Done. https://codereview.adblockplus.org/29458601/diff/29466594/inject.preload.js File inject.preload.js (right): https://codereview.adblockplus.org/29458601/diff/29466594/inject.preload.js#n... inject.preload.js:378: // NOTE: Edge does not support generateCertificate On 2017/06/19 14:13:38, kzar wrote: > On 2017/06/19 11:07:02, Sebastian Noack wrote: > > But what is about the other properties copied here? > > Yea, this is not good. Please instead modify the copyProperties function to > check if the property exists. Something like this (untested): > > function copyProperties(src, dest, properties) > { > for (let name of properties) > { > if (src.hasOwnProperty(name)) > { > Object.defineProperty(dest, name, > Object.getOwnPropertyDescriptor(src, name)); > } > } > } Done. https://codereview.adblockplus.org/29458601/diff/29466594/lib/requestBlocker.js File lib/requestBlocker.js (left): https://codereview.adblockplus.org/29458601/diff/29466594/lib/requestBlocker.... lib/requestBlocker.js:33: if (!("OBJECT_SUBREQUEST" in chrome.webRequest.ResourceType)) On 2017/06/19 11:07:02, Sebastian Noack wrote: > This check was necessary to account for Firefox which distinguishes these > requests. (The comment below is wrong.) This was just accidental reverting of a patch. Fixed now. Edge supports neither OBJECT nor OBJECT_SUBREQUEST. See "Chrome incompatibilities" on webRequest here: https://docs.microsoft.com/en-us/microsoft-edge/extensions/api-support/suppor...
https://codereview.adblockplus.org/29458601/diff/29470697/README.md File README.md (right): https://codereview.adblockplus.org/29458601/diff/29470697/README.md#newcode60 README.md:60: after enabling extension development features in _about:flags_. Nit: I think the part about enabling the features in about:flags should be broken into a second sentence. As it stands that is kind of confusing, the reader might think they need to enable that feature in Firefox and Chrome too. https://codereview.adblockplus.org/29458601/diff/29470697/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29458601/diff/29470697/ext/background.js#n... ext/background.js:704: // NOTE: we expect this else branch to run only on Edge. Isn't this comment kind of the same as the one on line 698? If so maybe just add the details about the Edge issue to that one? https://codereview.adblockplus.org/29458601/diff/29470697/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29458601/diff/29470697/ext/common.js#newco... ext/common.js:25: if (typeof chrome == "undefined" || Nit: Personally I would have just done `if (!chrome || !chrome.extension)`. I guess it's subjective which is better, but note that would be consistent with your checks for `chrome.webRequest.ResourceType`. https://codereview.adblockplus.org/29458601/diff/29470697/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29458601/diff/29470697/lib/csp.js#newcode20 lib/csp.js:20: // Before Chrome 58, the webRequest API did not intercept WebSocket Maybe reword something like this? The webRequest API doesn't support WebSocket connection blocking in Microsoft Edge and versions of Chrome before 58. Therefore for those we inject CSP headers below as a workaround. See https://crbug.com/129353 and https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/10297376/
https://codereview.adblockplus.org/29458601/diff/29470697/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29458601/diff/29470697/ext/background.js#n... ext/background.js:704: // NOTE: we expect this else branch to run only on Edge. On 2017/06/22 11:23:37, kzar wrote: > Isn't this comment kind of the same as the one on line 698? If so maybe just add > the details about the Edge issue to that one? Done. https://codereview.adblockplus.org/29458601/diff/29470697/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29458601/diff/29470697/ext/common.js#newco... ext/common.js:25: if (typeof chrome == "undefined" || On 2017/06/22 11:23:38, kzar wrote: > Nit: Personally I would have just done `if (!chrome || !chrome.extension)`. I > guess it's subjective which is better, but note that would be consistent with > your checks for `chrome.webRequest.ResourceType`. Done. https://codereview.adblockplus.org/29458601/diff/29470697/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29458601/diff/29470697/lib/csp.js#newcode20 lib/csp.js:20: // Before Chrome 58, the webRequest API did not intercept WebSocket On 2017/06/22 11:23:38, kzar wrote: > Maybe reword something like this? > > The webRequest API doesn't support WebSocket connection blocking in Microsoft > Edge and versions of Chrome before 58. Therefore for those we inject CSP headers > below as a workaround. See https://crbug.com/129353 and > https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/10297376/ Done.
https://codereview.adblockplus.org/29458601/diff/29471558/ext/popup.js File ext/popup.js (right): https://codereview.adblockplus.org/29458601/diff/29471558/ext/popup.js#newcode5 ext/popup.js:5: if ((typeof chrome == "undefined") || Nit: This seems inconsistent with the check in ext/common.js now.
LGTM
https://codereview.adblockplus.org/29458601/diff/29471570/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29458601/diff/29471570/lib/requestBlocker.... lib/requestBlocker.js:33: if (chrome.webRequest.ResourceType && Assuming that Microsoft Edge does not report requests as object_subrequest, this logic is incorrect. In this case the check should rather be: if (!chrome.webRequest.ResourceType || !("OBJECT_SUBREQUEST" in chrome.webRequest.ResourceType)) Also the comment above would have to be updated in order to mention Microsoft Edge along Chrome.
https://codereview.adblockplus.org/29458601/diff/29471570/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29458601/diff/29471570/lib/requestBlocker.... lib/requestBlocker.js:33: if (chrome.webRequest.ResourceType && On 2017/06/23 16:02:01, Sebastian Noack wrote: > Assuming that Microsoft Edge does not report requests as object_subrequest, this > logic is incorrect. In this case the check should rather be: > > if (!chrome.webRequest.ResourceType || > !("OBJECT_SUBREQUEST" in chrome.webRequest.ResourceType)) > > Also the comment above would have to be updated in order to mention Microsoft > Edge along Chrome. Not only does Edge not report object_subrequest, it also does not report object_request, as it is mentioned in the webRequest section here: https://docs.microsoft.com/en-us/microsoft-edge/extensions/api-support/suppor.... So as I see it this logic is irrelevant for Edge. This change is merely to avoid an exception in Edge.
https://codereview.adblockplus.org/29458601/diff/29471570/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29458601/diff/29471570/lib/requestBlocker.... lib/requestBlocker.js:33: if (chrome.webRequest.ResourceType && On 2017/06/26 07:28:08, Oleksandr wrote: > On 2017/06/23 16:02:01, Sebastian Noack wrote: > > Assuming that Microsoft Edge does not report requests as object_subrequest, > this > > logic is incorrect. In this case the check should rather be: > > > > if (!chrome.webRequest.ResourceType || > > !("OBJECT_SUBREQUEST" in chrome.webRequest.ResourceType)) > > > > Also the comment above would have to be updated in order to mention Microsoft > > Edge along Chrome. > > Not only does Edge not report object_subrequest, it also does not report > object_request, as it is mentioned in the webRequest section here: > https://docs.microsoft.com/en-us/microsoft-edge/extensions/api-support/suppor.... > So as I see it this logic is irrelevant for Edge. This change is merely to avoid > an exception in Edge. The question is how likely is it that they start reporting requests as "object" in the future? And how likely will they distinguish between "object_subrequest" (before implementing webRequest.RecourseType)? I feel it might be slightly more robust, to go with the logic I suggested above.
LGTM, but please DO NOT PUSH anything to master before Adblock Plus 1.12.3 for Chrome is out, we are currently in code freeze.
LGTM
https://codereview.adblockplus.org/29458601/diff/29490647/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29458601/diff/29490647/ext/common.js#newco... ext/common.js:25: if (typeof chrome == "undefined" || typeof chrome.extension == "undefined") When 'chrome' is undefined, calling 'if (!chrome)' raises an exception.
https://codereview.adblockplus.org/29458601/diff/29490647/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29458601/diff/29490647/ext/common.js#newco... ext/common.js:25: if (typeof chrome == "undefined" || typeof chrome.extension == "undefined") On 2017/07/17 12:52:40, Oleksandr wrote: > When 'chrome' is undefined, calling 'if (!chrome)' raises an exception. Acknowledged. https://codereview.adblockplus.org/29458601/diff/29490647/ext/popup.js File ext/popup.js (right): https://codereview.adblockplus.org/29458601/diff/29490647/ext/popup.js#newcode15 ext/popup.js:15: // Edge needs the i18n to be local, otherwise it can't process substitutions How about this? "Calling i18n from the background page causes Edge to throw an exception. LINK" https://codereview.adblockplus.org/29458601/diff/29490647/options.js File options.js (right): https://codereview.adblockplus.org/29458601/diff/29490647/options.js#newcode540 options.js:540: for (let i = 0; i < list.length; i++) This will be kinda slow when there are lots of filters, but I don't have a better idea other than attempting to reimplement CSS.escape. Any reason you didn't use for ... of? I imagined it something like this (untested): for (let option of list.options) { if (option.value == text) list.removeChild(option); }
https://codereview.adblockplus.org/29458601/diff/29490647/options.js File options.js (right): https://codereview.adblockplus.org/29458601/diff/29490647/options.js#newcode540 options.js:540: for (let i = 0; i < list.length; i++) On 2017/07/17 14:08:19, kzar wrote: > This will be kinda slow when there are lots of filters, but I don't have a > better idea other than attempting to reimplement CSS.escape. > > Any reason you didn't use for ... of? I imagined it something like this > (untested): > > for (let option of list.options) > { > if (option.value == text) > list.removeChild(option); > } Couldn't we just do this instead of CSS.escape(): var selector = "option[value='" + text.replace(/'/g, "\\'") + "']"; Of course using CSS.escape() would be better, but in this case I think it would be more preferable to have the same code path on all platforms.
https://codereview.adblockplus.org/29458601/diff/29490647/options.js File options.js (right): https://codereview.adblockplus.org/29458601/diff/29490647/options.js#newcode540 options.js:540: for (let i = 0; i < list.length; i++) On 2017/07/17 14:17:21, Sebastian Noack wrote: > On 2017/07/17 14:08:19, kzar wrote: > > This will be kinda slow when there are lots of filters, but I don't have a > > better idea other than attempting to reimplement CSS.escape. > > > > Any reason you didn't use for ... of? I imagined it something like this > > (untested): > > > > for (let option of list.options) > > { > > if (option.value == text) > > list.removeChild(option); > > } > > Couldn't we just do this instead of CSS.escape(): > > var selector = "option[value='" + text.replace(/'/g, "\\'") + "']"; > > Of course using CSS.escape() would be better, but in this case I think it would > be more preferable to have the same code path on all platforms. I'm not sure that would always work, for example what if the string contains "\'", wouldn't it become "\\'"? But I do agree if we can find a reasonably simple way to always use the same code path here it would be nice, and using querySelectorAll here will be much faster for large numbers of filters.
https://codereview.adblockplus.org/29458601/diff/29490647/options.js File options.js (right): https://codereview.adblockplus.org/29458601/diff/29490647/options.js#newcode540 options.js:540: for (let i = 0; i < list.length; i++) On 2017/07/17 14:27:02, kzar wrote: > On 2017/07/17 14:17:21, Sebastian Noack wrote: > > On 2017/07/17 14:08:19, kzar wrote: > > > This will be kinda slow when there are lots of filters, but I don't have a > > > better idea other than attempting to reimplement CSS.escape. > > > > > > Any reason you didn't use for ... of? I imagined it something like this > > > (untested): > > > > > > for (let option of list.options) > > > { > > > if (option.value == text) > > > list.removeChild(option); > > > } > > > > Couldn't we just do this instead of CSS.escape(): > > > > var selector = "option[value='" + text.replace(/'/g, "\\'") + "']"; > > > > Of course using CSS.escape() would be better, but in this case I think it > would > > be more preferable to have the same code path on all platforms. > > I'm not sure that would always work, for example what if the string contains > "\'", wouldn't it become "\\'"? But I do agree if we can find a reasonably > simple way to always use the same code path here it would be nice, and using > querySelectorAll here will be much faster for large numbers of filters. It seems that we have already implemented the same logic in a feature complete and reliable way that doesn't rely on CSS.escape() here: https://hg.adblockplus.org/adblockpluschrome/file/efc63b673869/lib/filterComp... Perhaps we can just reuse that code here (ideally without duplication).
https://codereview.adblockplus.org/29458601/diff/29490647/options.js File options.js (right): https://codereview.adblockplus.org/29458601/diff/29490647/options.js#newcode540 options.js:540: for (let i = 0; i < list.length; i++) On 2017/07/17 14:31:25, Sebastian Noack wrote: > On 2017/07/17 14:27:02, kzar wrote: > > On 2017/07/17 14:17:21, Sebastian Noack wrote: > > > On 2017/07/17 14:08:19, kzar wrote: > > > > This will be kinda slow when there are lots of filters, but I don't have a > > > > better idea other than attempting to reimplement CSS.escape. > > > > > > > > Any reason you didn't use for ... of? I imagined it something like this > > > > (untested): > > > > > > > > for (let option of list.options) > > > > { > > > > if (option.value == text) > > > > list.removeChild(option); > > > > } > > > > > > Couldn't we just do this instead of CSS.escape(): > > > > > > var selector = "option[value='" + text.replace(/'/g, "\\'") + "']"; > > > > > > Of course using CSS.escape() would be better, but in this case I think it > > would > > > be more preferable to have the same code path on all platforms. > > > > I'm not sure that would always work, for example what if the string contains > > "\'", wouldn't it become "\\'"? But I do agree if we can find a reasonably > > simple way to always use the same code path here it would be nice, and using > > querySelectorAll here will be much faster for large numbers of filters. > > It seems that we have already implemented the same logic in a feature complete > and reliable way that doesn't rely on CSS.escape() here: > https://hg.adblockplus.org/adblockpluschrome/file/efc63b673869/lib/filterComp... > > Perhaps we can just reuse that code here (ideally without duplication). Sounds good to me. Though to avoid duplicating the logic the only (non-hack) way I can think of doing it would be to have filterComposer.js combined with options.js at build time so that require("filterComposer").quoteCSS would work here. But that seems like kind of overkill, what do you think?
https://codereview.adblockplus.org/29458601/diff/29490647/options.js File options.js (right): https://codereview.adblockplus.org/29458601/diff/29490647/options.js#newcode540 options.js:540: for (let i = 0; i < list.length; i++) On 2017/07/17 14:41:37, kzar wrote: > On 2017/07/17 14:31:25, Sebastian Noack wrote: > > On 2017/07/17 14:27:02, kzar wrote: > > > On 2017/07/17 14:17:21, Sebastian Noack wrote: > > > > On 2017/07/17 14:08:19, kzar wrote: > > > > > This will be kinda slow when there are lots of filters, but I don't have > a > > > > > better idea other than attempting to reimplement CSS.escape. > > > > > > > > > > Any reason you didn't use for ... of? I imagined it something like this > > > > > (untested): > > > > > > > > > > for (let option of list.options) > > > > > { > > > > > if (option.value == text) > > > > > list.removeChild(option); > > > > > } > > > > > > > > Couldn't we just do this instead of CSS.escape(): > > > > > > > > var selector = "option[value='" + text.replace(/'/g, "\\'") + "']"; > > > > > > > > Of course using CSS.escape() would be better, but in this case I think it > > > would > > > > be more preferable to have the same code path on all platforms. > > > > > > I'm not sure that would always work, for example what if the string contains > > > "\'", wouldn't it become "\\'"? But I do agree if we can find a reasonably > > > simple way to always use the same code path here it would be nice, and using > > > querySelectorAll here will be much faster for large numbers of filters. > > > > It seems that we have already implemented the same logic in a feature complete > > and reliable way that doesn't rely on CSS.escape() here: > > > https://hg.adblockplus.org/adblockpluschrome/file/efc63b673869/lib/filterComp... > > > > Perhaps we can just reuse that code here (ideally without duplication). > > Sounds good to me. > > Though to avoid duplicating the logic the only (non-hack) way I can think of > doing it would be to have filterComposer.js combined with options.js at build > time so that require("filterComposer").quoteCSS would work here. But that seems > like kind of overkill, what do you think? How about using messaging?
On 2017/07/17 15:49:57, Sebastian Noack wrote: > https://codereview.adblockplus.org/29458601/diff/29490647/options.js > File options.js (right): > > https://codereview.adblockplus.org/29458601/diff/29490647/options.js#newcode540 > options.js:540: for (let i = 0; i < list.length; i++) > On 2017/07/17 14:41:37, kzar wrote: > > On 2017/07/17 14:31:25, Sebastian Noack wrote: > > > On 2017/07/17 14:27:02, kzar wrote: > > > > On 2017/07/17 14:17:21, Sebastian Noack wrote: > > > > > On 2017/07/17 14:08:19, kzar wrote: > > > > > > This will be kinda slow when there are lots of filters, but I don't > have > > a > > > > > > better idea other than attempting to reimplement CSS.escape. > > > > > > > > > > > > Any reason you didn't use for ... of? I imagined it something like > this > > > > > > (untested): > > > > > > > > > > > > for (let option of list.options) > > > > > > { > > > > > > if (option.value == text) > > > > > > list.removeChild(option); > > > > > > } > > > > > > > > > > Couldn't we just do this instead of CSS.escape(): > > > > > > > > > > var selector = "option[value='" + text.replace(/'/g, "\\'") + "']"; > > > > > > > > > > Of course using CSS.escape() would be better, but in this case I think > it > > > > would > > > > > be more preferable to have the same code path on all platforms. > > > > > > > > I'm not sure that would always work, for example what if the string > contains > > > > "\'", wouldn't it become "\\'"? But I do agree if we can find a reasonably > > > > simple way to always use the same code path here it would be nice, and > using > > > > querySelectorAll here will be much faster for large numbers of filters. > > > > > > It seems that we have already implemented the same logic in a feature > complete > > > and reliable way that doesn't rely on CSS.escape() here: > > > > > > https://hg.adblockplus.org/adblockpluschrome/file/efc63b673869/lib/filterComp... > > > > > > Perhaps we can just reuse that code here (ideally without duplication). > > > > Sounds good to me. > > > > Though to avoid duplicating the logic the only (non-hack) way I can think of > > doing it would be to have filterComposer.js combined with options.js at build > > time so that require("filterComposer").quoteCSS would work here. But that > seems > > like kind of overkill, what do you think? > > How about using messaging? Oh yea good idea, that would work.
https://codereview.adblockplus.org/29458601/diff/29490647/ext/popup.js File ext/popup.js (right): https://codereview.adblockplus.org/29458601/diff/29490647/ext/popup.js#newcode15 ext/popup.js:15: // Edge needs the i18n to be local, otherwise it can't process substitutions On 2017/07/17 14:08:19, kzar wrote: > How about this? "Calling i18n from the background page causes Edge to throw an > exception. LINK" Rephrased to be more precise. Calling i18n alone does not throw... https://codereview.adblockplus.org/29458601/diff/29490647/options.js File options.js (right): https://codereview.adblockplus.org/29458601/diff/29490647/options.js#newcode540 options.js:540: for (let i = 0; i < list.length; i++) On 2017/07/17 15:49:56, Sebastian Noack wrote: > On 2017/07/17 14:41:37, kzar wrote: > > On 2017/07/17 14:31:25, Sebastian Noack wrote: > > > On 2017/07/17 14:27:02, kzar wrote: > > > > On 2017/07/17 14:17:21, Sebastian Noack wrote: > > > > > On 2017/07/17 14:08:19, kzar wrote: > > > > > > This will be kinda slow when there are lots of filters, but I don't > have > > a > > > > > > better idea other than attempting to reimplement CSS.escape. > > > > > > > > > > > > Any reason you didn't use for ... of? I imagined it something like > this > > > > > > (untested): > > > > > > > > > > > > for (let option of list.options) > > > > > > { > > > > > > if (option.value == text) > > > > > > list.removeChild(option); > > > > > > } > > > > > > > > > > Couldn't we just do this instead of CSS.escape(): > > > > > > > > > > var selector = "option[value='" + text.replace(/'/g, "\\'") + "']"; > > > > > > > > > > Of course using CSS.escape() would be better, but in this case I think > it > > > > would > > > > > be more preferable to have the same code path on all platforms. > > > > > > > > I'm not sure that would always work, for example what if the string > contains > > > > "\'", wouldn't it become "\\'"? But I do agree if we can find a reasonably > > > > simple way to always use the same code path here it would be nice, and > using > > > > querySelectorAll here will be much faster for large numbers of filters. > > > > > > It seems that we have already implemented the same logic in a feature > complete > > > and reliable way that doesn't rely on CSS.escape() here: > > > > > > https://hg.adblockplus.org/adblockpluschrome/file/efc63b673869/lib/filterComp... > > > > > > Perhaps we can just reuse that code here (ideally without duplication). > > > > Sounds good to me. > > > > Though to avoid duplicating the logic the only (non-hack) way I can think of > > doing it would be to have filterComposer.js combined with options.js at build > > time so that require("filterComposer").quoteCSS would work here. But that > seems > > like kind of overkill, what do you think? > > How about using messaging? Done.
https://codereview.adblockplus.org/29458601/diff/29490647/ext/popup.js File ext/popup.js (right): https://codereview.adblockplus.org/29458601/diff/29490647/ext/popup.js#newcode15 ext/popup.js:15: // Edge needs the i18n to be local, otherwise it can't process substitutions On 2017/07/17 23:52:09, Oleksandr wrote: > On 2017/07/17 14:08:19, kzar wrote: > > How about this? "Calling i18n from the background page causes Edge to throw an > > exception. LINK" > > Rephrased to be more precise. Calling i18n alone does not throw... "Calling i18n.getMessage from the background page causes Edge to throw an exception."? https://codereview.adblockplus.org/29458601/diff/29490717/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29458601/diff/29490717/lib/filterComposer.... lib/filterComposer.js:252: port.on("utils.escapeCSS", (message, sender) => Wouldn't the message name be composer.quoteCSS? https://codereview.adblockplus.org/29458601/diff/29490717/lib/filterComposer.... lib/filterComposer.js:254: return escapeCSS(message.CSS); I think Sebastian was suggesting to use quoteCSS.
Otherwise LGTM, assuming that you've tested the latest option page changes. https://codereview.adblockplus.org/29458601/diff/29490647/ext/popup.js File ext/popup.js (right): https://codereview.adblockplus.org/29458601/diff/29490647/ext/popup.js#newcode15 ext/popup.js:15: // Edge needs the i18n to be local, otherwise it can't process substitutions On 2017/07/18 09:30:49, kzar wrote: > On 2017/07/17 23:52:09, Oleksandr wrote: > > On 2017/07/17 14:08:19, kzar wrote: > > > How about this? "Calling i18n from the background page causes Edge to throw > an > > > exception. LINK" > > > > Rephrased to be more precise. Calling i18n alone does not throw... > > "Calling i18n.getMessage from the background page causes Edge to throw an > exception."? It seems like your latest improvement to this comment isn't included in the patch set?
Rebased and ready for push. To test this I have applied https://codereview.adblockplus.org/29367148/ to buildtools to produce devenv build and loaded into Edge. The extension is *very* slow, due to storage.local issue, but the logic on options page works as expected. Adding and removing custom filters works.
Just a few more nits, otherwise looking good. https://codereview.adblockplus.org/29458601/diff/29499752/README.md File README.md (right): https://codereview.adblockplus.org/29458601/diff/29499752/README.md#newcode35 README.md:35: _adblockpluschrome-1.2.3.nnnn.crx_, _adblockplusedge-1.2.3.nnnn.appx_ or _adblockplusfirefox-1.2.3.nnnn.xpi_. Nit: Please can you fix this long line? https://codereview.adblockplus.org/29458601/diff/29499752/inject.preload.js File inject.preload.js (right): https://codereview.adblockplus.org/29458601/diff/29499752/inject.preload.js#n... inject.preload.js:381: // NOTE: Edge does not support generateCertificate Nit: Mind removing this comment? I don't think it adds much. https://codereview.adblockplus.org/29458601/diff/29499752/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29458601/diff/29499752/lib/requestBlocker.... lib/requestBlocker.js:35: RegExpFilter.typeMap.OBJECT_SUBREQUEST = RegExpFilter.typeMap.OBJECT; Nit: Please uses braces for this since the the clause spans multiple lines. https://codereview.adblockplus.org/29458601/diff/29499752/lib/requestBlocker.... lib/requestBlocker.js:193: (msg.requestType.toUpperCase() in chrome.webRequest.ResourceType)) Nit: Same here.
All done.
LGTM! Go ahead and push when you're ready. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
