Bug 79008 – Manage node distributions in ShadowRootList, instead of each ShadowRoots.

• Component: DOM
• Status: RESOLVED FIXED

[Hayato Ito]

To support multiple shadow roots, we have to manage lifecycle of node distributions in ShadowRootList instead of ShadowRoot.

[Hayato Ito]

Created attachment 127757 [details]
wip. Some layouttests becomes broken

[Hayato Ito]

Created attachment 127764 [details]
ready for reviews

[Shinya Kawanaka]

Comment on attachment 127764 [details]
ready for reviews

View in context: https://bugs.webkit.org/attachment.cgi?id=127764&action=review

> Source/WebCore/dom/ShadowRoot.h:63
> +    // HTMLContentElement* insertionPointFor(Node*) const;

We can remove this?

> Source/WebCore/dom/ShadowRoot.h:65
> +    // bool isSelectorActive() const;

ditto.

[Hayato Ito]

Created attachment 127766 [details]
update

[Hayato Ito]

Comment on attachment 127764 [details]
ready for reviews

View in context: https://bugs.webkit.org/attachment.cgi?id=127764&action=review

>> Source/WebCore/dom/ShadowRoot.h:63
>> +    // HTMLContentElement* insertionPointFor(Node*) const;
> 
> We can remove this?

Ops. Done.

>> Source/WebCore/dom/ShadowRoot.h:65
>> +    // bool isSelectorActive() const;
> 
> ditto.

Done.

[Hajime Morrita]

Comment on attachment 127766 [details]
update

View in context: https://bugs.webkit.org/attachment.cgi?id=127766&action=review

> Source/WebCore/ChangeLog:3
> +        Make ShadowRootList manage a node distribution.

Could you elaborate?

> Source/WebCore/dom/ShadowRoot.h:31
> +#include "Element.h"

This dependency is unfortunate. Why do we need this? It looks we can just modify removed functions as delegates.

[Hayato Ito]

Comment on attachment 127766 [details]
update

View in context: https://bugs.webkit.org/attachment.cgi?id=127766&action=review

>> Source/WebCore/ChangeLog:3
>> +        Make ShadowRootList manage a node distribution.
> 
> Could you elaborate?

Sure. I've added.

>> Source/WebCore/dom/ShadowRoot.h:31
>> +#include "Element.h"
> 
> This dependency is unfortunate. Why do we need this? It looks we can just modify removed functions as delegates.

This is required due to the inline shadowRootListForThis() function.
Although I intended to get rid of this inline function in followup patches, I got rid of this function in this patch to make clear that clients should acquire ShadowRootList by themselves.

[Hayato Ito]

Created attachment 127793 [details]
get rid of shadowRootListForThis to make responsibility clear

[Shinya Kawanaka]

Comment on attachment 127793 [details]
get rid of shadowRootListForThis to make responsibility clear

View in context: https://bugs.webkit.org/attachment.cgi?id=127793&action=review

> Source/WebCore/dom/ShadowRoot.cpp:132
> +    ASSERT(element->shadowRootList() == shadowRoot->shadowRootListForThis());

You have gotten rid of shadowRootListForThis(), right?
This will make win bot red.

[Hayato Ito]

Created attachment 127913 [details]
fix debug build

[Hayato Ito]

Thank you. Right. Fixed.

(In reply to comment #9)
> (From update of attachment 127793 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127793&action=review
> 
> > Source/WebCore/dom/ShadowRoot.cpp:132
> > +    ASSERT(element->shadowRootList() == shadowRoot->shadowRootListForThis());
> 
> You have gotten rid of shadowRootListForThis(), right?
> This will make win bot red.

[Shinya Kawanaka]

Maybe conflicted da patch for Issue 78778.

[Hayato Ito]

Created attachment 127924 [details]
rebased to ToT

[Hayato Ito]

Since first-in should win, I've rebased this patch to ToT.

(In reply to comment #12)
> Maybe conflicted da patch for Issue 78778.

[Shinya Kawanaka]

Can someone review this?

[Dimitri Glazkov (Google)]

Comment on attachment 127924 [details]
rebased to ToT

View in context: https://bugs.webkit.org/attachment.cgi?id=127924&action=review

> Source/WebCore/dom/ShadowRootList.h:50
> +    bool hasShadowRoot() const;
> +    ShadowRoot* youngestShadowRoot() const;
> +    ShadowRoot* oldestShadowRoot() const;

These changes look unrelated to the nature of the patch. Can you split them up into a separate patch?

[Hayato Ito]

Thank you for the review.

(In reply to comment #16)
> (From update of attachment 127924 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127924&action=review
> 
> > Source/WebCore/dom/ShadowRootList.h:50
> > +    bool hasShadowRoot() const;
> > +    ShadowRoot* youngestShadowRoot() const;
> > +    ShadowRoot* oldestShadowRoot() const;
> 
> These changes look unrelated to the nature of the patch. Can you split them up into a separate patch?

I've added these const for some reasons, but I don't remember. To recall the reasons, I've tried to build without these const. The build was successful :) You are right.

I guess the original big patch from which this patch was separated requires these const.

Let me land this patch after removing these const from this patch. Thank you.

[Hayato Ito]

I am wrong. It seems I tried to build on an irrelevant branch.

The build failed without these const.

The reason is 'ShadowRootList::host const' member function. 'hasShadowRoot()' and 'youngestShadowRoot()' are called from this const member_function.

So let me land this patch without removing these const.

(In reply to comment #17)
> Thank you for the review.
> 
> (In reply to comment #16)
> > (From update of attachment 127924 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=127924&action=review
> > 
> > > Source/WebCore/dom/ShadowRootList.h:50
> > > +    bool hasShadowRoot() const;
> > > +    ShadowRoot* youngestShadowRoot() const;
> > > +    ShadowRoot* oldestShadowRoot() const;
> > 
> > These changes look unrelated to the nature of the patch. Can you split them up into a separate patch?
> 
> I've added these const for some reasons, but I don't remember. To recall the reasons, I've tried to build without these const. The build was successful :) You are right.
> 
> I guess the original big patch from which this patch was separated requires these const.
> 
> Let me land this patch after removing these const from this patch. Thank you.

[Hayato Ito]

Created attachment 128359 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 128359 [details]
Patch for landing

Clearing flags on attachment: 128359

Committed r108605: <http://trac.webkit.org/changeset/108605>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.