Issue 29714555: Issue 6441 - Avoid unnecessary shadow root (Closed)
|
Created:
March 5, 2018, 9:47 a.m. by Manish Jethani Modified:
March 9, 2018, 1:10 a.m. Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
DescriptionFirefox dropped support for <shadow> last year:
https://hg.mozilla.org/releases/mozilla-release/rev/e8d213d3265f
If HTMLShadowElement is not available, we can just use <content> instead.
Patch Set 1 #Patch Set 2 : Use <content> if <shadow> is not supported #
Total comments: 4
Patch Set 3 : Use <content> only #Patch Set 4 : Use Shadow DOM v0 only on Chrome 65 and lower #Patch Set 5 : Still switch to <content> #
Total comments: 6
Patch Set 6 : Check navigator.userAgent specifically #
Total comments: 2
Patch Set 7 : Improve user agent check #MessagesTotal messages: 16
Patch Set 1
Patch Set 2: Use <content> is <shadow> is not supported Sorry, I realized we cannot create the shadow DOM lazily. The real issue is that Firefox does not support <shadow> anymore. We can fall back to <content> instead.
On 2018/03/05 12:49:42, Manish Jethani wrote: > Sorry, I realized we cannot create the shadow DOM lazily. The real issue is that > Firefox does not support <shadow> anymore. We can fall back to <content> > instead. Yes. For reference, as per the comment in the source code, we have to create the shadow root as early as possible, since creating the shadow root interferes with active CSS transitions (and potentially has other visual side effects). https://codereview.adblockplus.org/29714555/diff/29714565/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29714555/diff/29714565/include.preload.js#... include.preload.js:381: "shadow" : "content"; I wonder whether we should just use <content> no matter what. Since we only create the shadow root if there is no shadow root yet (which is only possible if it was created by another extension, anyway, due to the timing), <content> and <shadow> have essentially the same effect.
https://codereview.adblockplus.org/29714555/diff/29714565/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29714555/diff/29714565/include.preload.js#... include.preload.js:381: "shadow" : "content"; On 2018/03/08 18:34:26, Sebastian Noack wrote: > I wonder whether we should just use <content> no matter what. Since we only > create the shadow root if there is no shadow root yet (which is only possible if > it was created by another extension, anyway, due to the timing), <content> and > <shadow> have essentially the same effect. Are you sure that Chrome 49 supports <content>? I don't know, but I could check. Yes it would be nice to use <content> always.
Patch Set 3: Use <content> only https://codereview.adblockplus.org/29714555/diff/29714565/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29714555/diff/29714565/include.preload.js#... include.preload.js:381: "shadow" : "content"; On 2018/03/08 19:44:54, Manish Jethani wrote: > On 2018/03/08 18:34:26, Sebastian Noack wrote: > > I wonder whether we should just use <content> no matter what. Since we only > > create the shadow root if there is no shadow root yet (which is only possible > if > > it was created by another extension, anyway, due to the timing), <content> and > > <shadow> have essentially the same effect. > > Are you sure that Chrome 49 supports <content>? I don't know, but I could check. > Yes it would be nice to use <content> always. I checked the Chromium commit log and entries mentioning <content> go way back several years. I'm pretty sure it's supported in 49. Done.
As in the comment I left on the issue, we might want to check naigator.userAgent for Chromium <66, in order to avoid creating unnecessary shadow roots. Switching to <content> might make sense regardless, so that it doesn't explode if Firefox users mess with their user agent string. https://codereview.adblockplus.org/29714555/diff/29714565/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29714555/diff/29714565/include.preload.js#... include.preload.js:381: "shadow" : "content"; On 2018/03/08 19:53:57, Manish Jethani wrote: > On 2018/03/08 19:44:54, Manish Jethani wrote: > > On 2018/03/08 18:34:26, Sebastian Noack wrote: > > > I wonder whether we should just use <content> no matter what. Since we only > > > create the shadow root if there is no shadow root yet (which is only > possible > > if > > > it was created by another extension, anyway, due to the timing), <content> > and > > > <shadow> have essentially the same effect. > > > > Are you sure that Chrome 49 supports <content>? I don't know, but I could > check. > > Yes it would be nice to use <content> always. > > I checked the Chromium commit log and entries mentioning <content> go way back > several years. I'm pretty sure it's supported in 49. > > Done. Yes, I'm pretty sure <content> already existed when we initially started using Shadow DOM. The reason we didn't use it back then, was because we initially created the shadow root lazily, and wanted to avoid breaking websites that created their own shadow root for the document element.
Patch Set 4: Use Shadow DOM v0 only on Chrome 65 and lower Patch Set 5: Still switch to <content> On 2018/03/08 20:36:16, Sebastian Noack wrote: > As in the comment I left on the issue, we might want to check naigator.userAgent > for Chromium <66, in order to avoid creating unnecessary shadow roots. Done. > Switching to <content> might make sense regardless, so that it doesn't explode > if Firefox users mess with their user agent string. Done.
https://codereview.adblockplus.org/29714555/diff/29717627/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29714555/diff/29717627/include.preload.js#... include.preload.js:370: // user style sheets. This is irrelevant. We have feature detection above, and with the change to <content>, it should be compatible with Firefox as well. The important point, however, is that we want to avoid creating an unnecessary shadow root, if we can use user style sheets. https://codereview.adblockplus.org/29714555/diff/29717627/include.preload.js#... include.preload.js:371: let {application, applicationVersion} = require("info"); Do we use the "info" module in any other content script? If it's just for the check here, I'm not sure if it justifies having it added to the bundle. https://codereview.adblockplus.org/29714555/diff/29717627/include.preload.js#... include.preload.js:372: if (application != "chrome" || parseInt(applicationVersion) > 65) We don't care about the application, but about the platform.
Patch Set 6: Check navigator.userAgent specifically https://codereview.adblockplus.org/29714555/diff/29717627/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29714555/diff/29717627/include.preload.js#... include.preload.js:370: // user style sheets. On 2018/03/08 23:22:59, Sebastian Noack wrote: > This is irrelevant. We have feature detection above, and with the change to > <content>, it should be compatible with Firefox as well. The important point, > however, is that we want to avoid creating an unnecessary shadow root, if we can > use user style sheets. You're right, I've updated the comment. https://codereview.adblockplus.org/29714555/diff/29717627/include.preload.js#... include.preload.js:371: let {application, applicationVersion} = require("info"); On 2018/03/08 23:22:59, Sebastian Noack wrote: > Do we use the "info" module in any other content script? If it's just for the > check here, I'm not sure if it justifies having it added to the bundle. We don't. OK, I'm checking the navigator.userAgent property directly now. https://codereview.adblockplus.org/29714555/diff/29717627/include.preload.js#... include.preload.js:372: if (application != "chrome" || parseInt(applicationVersion) > 65) On 2018/03/08 23:22:59, Sebastian Noack wrote: > We don't care about the application, but about the platform. Acknowledged.
https://codereview.adblockplus.org/29714555/diff/29717629/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29714555/diff/29717629/include.preload.js#... include.preload.js:371: return null; I think this can be somewhat simplified: let match = /\bChrome\/(\d+)/.exec(navigator.userAgent); if (!match || match[1] >= 66) return null; Also note that I didn't care to check whether there is a minor version number.
Patch Set 7: Improve user agent check https://codereview.adblockplus.org/29714555/diff/29717629/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29714555/diff/29717629/include.preload.js#... include.preload.js:371: return null; On 2018/03/09 00:28:13, Sebastian Noack wrote: > I think this can be somewhat simplified: > > let match = /\bChrome\/(\d+)/.exec(navigator.userAgent); > if (!match || match[1] >= 66) > return null; > > Also note that I didn't care to check whether there is a minor version number. Done.
LGTM for the code. But please update the review title (and the respective commit message) to reflect the final change more accurately. BTW, I think it would be appropriate to still land this change in master, despite code freeze. If Firefox will ship a version with Web Components enabled by default, before we release this fix (and who knows when we release 3.0.4), this could end up ugly. Also since with the upcoming release we will start using user style sheets on Chrome 66+, it would be great if we'd stop creating shadow roots at the same time.
On 2018/03/09 00:47:33, Sebastian Noack wrote: > LGTM for the code. Thanks! > But please update the review title (and the respective commit message) to > reflect the final change more accurately. OK. > BTW, I think it would be appropriate to still land this change in master, > despite code freeze. If Firefox will ship a version with Web Components enabled > by default, before we release this fix (and who knows when we release 3.0.4), > [...] Do you think we should limit this change to just the switch to <content> and add the version check later? i.e. <content> goes into master, the check for version to avoid creating an unnecessary shadow root goes into next. The unnecessary shadow root fix is just a bit of an optimization, anyway we'll be using the shadow DOM for emulation filters until we have tabs.removeCSS.
On 2018/03/09 00:51:46, Manish Jethani wrote: > Do you think we should limit this change to just the switch to <content> and add > the version check later? i.e. <content> goes into master, the check for version > to avoid creating an unnecessary shadow root goes into next. The unnecessary > shadow root fix is just a bit of an optimization, anyway we'll be using the > shadow DOM for emulation filters until we have tabs.removeCSS. Should be OK. It's still a while until the release, to find any issues. And we shouldn't use user style sheets and shadow root at the same time.
On 2018/03/09 00:51:46, Manish Jethani wrote: > On 2018/03/09 00:47:33, Sebastian Noack wrote: > > > But please update the review title (and the respective commit message) to > > reflect the final change more accurately. > > OK. Done. > > BTW, I think it would be appropriate to still land this change in master, > > despite code freeze. If Firefox will ship a version with Web Components > enabled > > by default, before we release this fix (and who knows when we release 3.0.4), > > [...] > > Do you think we should limit this change to just the switch to <content> and add > the version check later? i.e. <content> goes into master, the check for version > to avoid creating an unnecessary shadow root goes into next. The unnecessary > shadow root fix is just a bit of an optimization, anyway we'll be using the > shadow DOM for emulation filters until we have tabs.removeCSS. OK, never mind. I have committed it now. Thanks! 
|
