Bug 79008

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

Created attachment 127764 [details] ready for reviews

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.

Created attachment 127766 [details] update

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.

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.

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.

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

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.

Created attachment 127913 [details] fix debug build

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.

Maybe conflicted da patch for Issue 78778.

Created attachment 127924 [details] rebased to ToT

Since first-in should win, I've rebased this patch to ToT. (In reply to comment #12) > Maybe conflicted da patch for Issue 78778.

Can someone review this?

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?

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.

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.

Created attachment 128359 [details] Patch for landing

Comment on attachment 128359 [details] Patch for landing Clearing flags on attachment: 128359 Committed r108605: <http://trac.webkit.org/changeset/108605>

All reviewed patches have been landed. Closing bug.