Issue 29349823: Fixes 4328 - Multiple shadowRoot deprecated warnings (Closed)
|
Created:
Aug. 15, 2016, 8:27 p.m. by kzar Modified:
Nov. 17, 2016, 3:59 p.m. Reviewers:
Sebastian Noack Visibility:
Public. |
DescriptionFixes 4328 - Multiple shadowRoot deprecated warnings
Patch Set 1 #Patch Set 2 : Reuse existing shadowRoot #
Total comments: 2
Patch Set 3 : Fixed indentation #MessagesTotal messages: 10
Patch Set 1
That doesn't work. We create our shadow root first hence it is always null at this point.
On 2016/08/15 20:44:43, Sebastian Noack wrote: > That doesn't work. We create our shadow root first hence it is always null at > this point. Not if AdBlock is also installed and happens to create a shadowRoot first.
On 2016/08/15 20:45:58, kzar wrote: > On 2016/08/15 20:44:43, Sebastian Noack wrote: > > That doesn't work. We create our shadow root first hence it is always null at > > this point. > > Not if AdBlock is also installed and happens to create a shadowRoot first. I see. I wonder whether we should just add our <style> into their shadow root if it exists, rather than into the light DOM.
On 2016/08/16 08:24:29, Sebastian Noack wrote: > On 2016/08/15 20:45:58, kzar wrote: > > On 2016/08/15 20:44:43, Sebastian Noack wrote: > > > That doesn't work. We create our shadow root first hence it is always null > at > > > this point. > > > > Not if AdBlock is also installed and happens to create a shadowRoot first. > > I see. I wonder whether we should just add our <style> into their shadow root if > it exists, rather than into the light DOM. Well I'm not 100% sure the best approach. Whatever happens it pretty much sucks that the user has two copies of the same / similar code running. But whatever we should do it shouldn't spam warnings I guess.
On 2016/08/16 08:35:35, kzar wrote:
> On 2016/08/16 08:24:29, Sebastian Noack wrote:
> > On 2016/08/15 20:45:58, kzar wrote:
> > > On 2016/08/15 20:44:43, Sebastian Noack wrote:
> > > > That doesn't work. We create our shadow root first hence it is always
null
> > at
> > > > this point.
> > >
> > > Not if AdBlock is also installed and happens to create a shadowRoot first.
> >
> > I see. I wonder whether we should just add our <style> into their shadow
root
> if
> > it exists, rather than into the light DOM.
>
> Well I'm not 100% sure the best approach. Whatever happens it pretty much
sucks
> that the user has two copies of the same / similar code running. But whatever
we
> should do it shouldn't spam warnings I guess.
No strong opinion either. Also it seems that it wouldn't make any difference
from the code complexity view of point. That's all you would need to change to
reuse the existing shadow root if any:
var shadow = document.documentElement.shadowRoot;
...
if (!shadow && "createShadowRoot" in document.documentElement && ...)
{
...
However, that way AdBlock and Adblock Plus will be as effective regardless
whether the other runs first.
Patch Set 2: Reuse existing shadowRoot Yea, good point. I'll do it that way, so that if both extensions run we shouldn't get warnings, but we still take care to create the shadowRoot / hide it in case the other extension didn't bother. (Sorry about the unrelated changes, I forgot to rebase as a separate patch set.)
https://codereview.adblockplus.org/29349823/diff/29350051/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29349823/diff/29350051/include.preload.js#... include.preload.js:470: document.documentElement.createShadowRoot(); Nit: It seems the indentation is off by 2 characters here.
Patch Set 3 : Fixed indentation https://codereview.adblockplus.org/29349823/diff/29350051/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29349823/diff/29350051/include.preload.js#... include.preload.js:470: document.documentElement.createShadowRoot(); On 2016/11/17 15:38:00, Sebastian Noack wrote: > Nit: It seems the indentation is off by 2 characters here. Done.
LGTM 
|
