Bug 79008 - Manage node distributions in ShadowRootList, instead of each ShadowRoots.
Summary: Manage node distributions in ShadowRootList, instead of each ShadowRoots.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hayato Ito
URL:
Keywords:
Depends on:
Blocks: 77503
  Show dependency treegraph
 
Reported: 2012-02-19 21:14 PST by Hayato Ito
Modified: 2012-02-22 21:14 PST (History)
6 users (show)

See Also:


Attachments
wip. Some layouttests becomes broken (13.35 KB, patch)
2012-02-19 21:23 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
ready for reviews (13.24 KB, patch)
2012-02-19 22:46 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
update (13.15 KB, patch)
2012-02-19 22:57 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
get rid of shadowRootListForThis to make responsibility clear (13.29 KB, patch)
2012-02-20 02:51 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
fix debug build (13.21 KB, patch)
2012-02-20 21:26 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
rebased to ToT (12.66 KB, patch)
2012-02-20 23:00 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
Patch for landing (15.30 KB, patch)
2012-02-22 18:25 PST, Hayato Ito
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hayato Ito 2012-02-19 21:14:25 PST
To support multiple shadow roots, we have to manage lifecycle of node distributions in ShadowRootList instead of ShadowRoot.
Comment 1 Hayato Ito 2012-02-19 21:23:23 PST
Created attachment 127757 [details]
wip. Some layouttests becomes broken
Comment 2 Hayato Ito 2012-02-19 22:46:50 PST
Created attachment 127764 [details]
ready for reviews
Comment 3 Shinya Kawanaka 2012-02-19 22:51:57 PST
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.
Comment 4 Hayato Ito 2012-02-19 22:57:52 PST
Created attachment 127766 [details]
update
Comment 5 Hayato Ito 2012-02-19 22:58:04 PST
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 6 Hajime Morrita 2012-02-20 01:21:16 PST
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 7 Hayato Ito 2012-02-20 02:50:52 PST
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.
Comment 8 Hayato Ito 2012-02-20 02:51:27 PST
Created attachment 127793 [details]
get rid of shadowRootListForThis to make responsibility clear
Comment 9 Shinya Kawanaka 2012-02-20 21:07:15 PST
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.
Comment 10 Hayato Ito 2012-02-20 21:26:21 PST
Created attachment 127913 [details]
fix debug build
Comment 11 Hayato Ito 2012-02-20 21:26:58 PST
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.
Comment 12 Shinya Kawanaka 2012-02-20 22:50:33 PST
Maybe conflicted da patch for Issue 78778.
Comment 13 Hayato Ito 2012-02-20 23:00:42 PST
Created attachment 127924 [details]
rebased to ToT
Comment 14 Hayato Ito 2012-02-20 23:01:49 PST
Since first-in should win, I've rebased this patch to ToT.

(In reply to comment #12)
> Maybe conflicted da patch for Issue 78778.
Comment 15 Shinya Kawanaka 2012-02-22 05:06:09 PST
Can someone review this?
Comment 16 Dimitri Glazkov (Google) 2012-02-22 09:56:53 PST
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?
Comment 17 Hayato Ito 2012-02-22 17:41:29 PST
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.
Comment 18 Hayato Ito 2012-02-22 17:59:56 PST
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.
Comment 19 Hayato Ito 2012-02-22 18:25:07 PST
Created attachment 128359 [details]
Patch for landing
Comment 20 WebKit Review Bot 2012-02-22 21:14:04 PST
Comment on attachment 128359 [details]
Patch for landing

Clearing flags on attachment: 128359

Committed r108605: <http://trac.webkit.org/changeset/108605>
Comment 21 WebKit Review Bot 2012-02-22 21:14:10 PST
All reviewed patches have been landed.  Closing bug.