Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

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.
Reviewers:
Sebastian Noack, kzar
Base URL:
https://hg.adblockplus.org/adblockpluschrome/
Visibility:
Public.

Description

Firefox 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+7 lines, -1 line) Patch
M include.preload.js View 1 2 3 4 5 6 1 chunk +7 lines, -1 line 0 comments Download

Messages

Total messages: 16
Manish Jethani
March 5, 2018, 9:47 a.m. (2018-03-05 09:47:51 UTC) #1
Manish Jethani
Patch Set 1
March 5, 2018, 9:50 a.m. (2018-03-05 09:50:59 UTC) #2
Manish Jethani
Patch Set 2: Use <content> is <shadow> is not supported Sorry, I realized we cannot ...
March 5, 2018, 12:49 p.m. (2018-03-05 12:49:42 UTC) #3
Sebastian Noack
On 2018/03/05 12:49:42, Manish Jethani wrote: > Sorry, I realized we cannot create the shadow ...
March 8, 2018, 6:34 p.m. (2018-03-08 18:34:26 UTC) #4
Manish Jethani
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#newcode381 include.preload.js:381: "shadow" : "content"; On 2018/03/08 18:34:26, Sebastian Noack wrote: ...
March 8, 2018, 7:44 p.m. (2018-03-08 19:44:54 UTC) #5
Manish Jethani
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#newcode381 include.preload.js:381: "shadow" : "content"; ...
March 8, 2018, 7:53 p.m. (2018-03-08 19:53:57 UTC) #6
Sebastian Noack
As in the comment I left on the issue, we might want to check naigator.userAgent ...
March 8, 2018, 8:36 p.m. (2018-03-08 20:36:16 UTC) #7
Manish Jethani
Patch Set 4: Use Shadow DOM v0 only on Chrome 65 and lower Patch Set ...
March 8, 2018, 9:52 p.m. (2018-03-08 21:52:55 UTC) #8
Sebastian Noack
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#newcode370 include.preload.js:370: // user style sheets. This is irrelevant. We have ...
March 8, 2018, 11:23 p.m. (2018-03-08 23:23:00 UTC) #9
Manish Jethani
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#newcode370 include.preload.js:370: // user style ...
March 8, 2018, 11:57 p.m. (2018-03-08 23:57:52 UTC) #10
Sebastian Noack
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#newcode371 include.preload.js:371: return null; I think this can be somewhat simplified: ...
March 9, 2018, 12:28 a.m. (2018-03-09 00:28:13 UTC) #11
Manish Jethani
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#newcode371 include.preload.js:371: return null; ...
March 9, 2018, 12:33 a.m. (2018-03-09 00:33:29 UTC) #12
Sebastian Noack
LGTM for the code. But please update the review title (and the respective commit message) ...
March 9, 2018, 12:47 a.m. (2018-03-09 00:47:33 UTC) #13
Manish Jethani
On 2018/03/09 00:47:33, Sebastian Noack wrote: > LGTM for the code. Thanks! > But please ...
March 9, 2018, 12:51 a.m. (2018-03-09 00:51:46 UTC) #14
Sebastian Noack
On 2018/03/09 00:51:46, Manish Jethani wrote: > Do you think we should limit this change ...
March 9, 2018, 1:09 a.m. (2018-03-09 01:09:52 UTC) #15
Manish Jethani
March 9, 2018, 1:10 a.m. (2018-03-09 01:10:04 UTC) #16
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!

Powered by Google App Engine
This is Rietveld