Bug 78069

Summary: Introduce ShadowRootList
Product: WebKit Reporter: Shinya Kawanaka <shinyak>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 78318    
Bug Blocks: 77931, 78313    
Attachments:
Description Flags
W.I.P.
none
W.I.P.
none
Patch
none
Patch
none
Test
none
Test
none
Patch
none
Patch none

Description Shinya Kawanaka 2012-02-07 19:08:15 PST
To support multiple shadow subtrees, it's better to have a structure having a list of shadow root.

Considering rendering stuffs, we should know where each light child is distributed. ShadowRootList will also work for them.
Comment 1 Shinya Kawanaka 2012-02-09 22:24:22 PST
Created attachment 126455 [details]
W.I.P.
Comment 2 Shinya Kawanaka 2012-02-09 22:41:27 PST
Created attachment 126457 [details]
W.I.P.
Comment 3 Shinya Kawanaka 2012-02-09 23:13:47 PST
Created attachment 126462 [details]
Patch
Comment 4 Hajime Morrita 2012-02-10 00:19:15 PST
Comment on attachment 126462 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        which are a previous element and a next element respectively.

Could you elaborate why we need dually-linked list instead of single?
When prev() is going to be used for example?

> Source/WebCore/dom/Element.cpp:64
> +#include "ShadowRootList.h"

You don't need this?

> Source/WebCore/dom/Element.cpp:1160
> +        return list->hasShadowRoot();
> +    return false;

Is this false case possible?
Is it feasible to prevent this from happening?

> Source/WebCore/dom/Element.cpp:1173
> +    ElementRareData* data = ensureRareData();

Why not ElementRareData::ensureShadowList() ?
In general, the list related method should be on ElementRareData because it owns the list.

Also, Is it really worth doing to allocating an extra heap chunk only for implementing dually-linked list?

> Source/WebCore/dom/Element.cpp:1187
> +void Element::setShadowRoot(PassRefPtr<ShadowRoot> prpShadowRoot, ExceptionCode& ec)

Please don't use abbreviation.

> Source/WebCore/dom/ElementRareData.h:74
> +    ShadowRootList* m_shadowRootList;

You can use OwnPtr.

> Source/WebCore/dom/ShadowRoot.h:42
> +    friend class WTF::DoublyLinkedListNode<ShadowRoot>;

Is it possible to assert some invariant on the ShadowRoot destructor?
For example, Is it possible to say that the instance is already unchained on destruction?

> Source/WebCore/dom/ShadowRootList.cpp:65
> +void ShadowRootList::addShadowRoot(PassRefPtr<ShadowRoot> prpShadowRoot, ExceptionCode& ec)

Please don't use abbreviation.

> Source/WebCore/dom/ShadowRootList.cpp:67
> +    // FIXME: We don't support multiple shadow root subtrees yet.

Pelase note bug#.

> Source/WebCore/dom/ShadowRootList.cpp:71
> +    if (!validateShadowRoot(host()->document(), shadowRoot.get(), ec))

I hope this check also happens before ShadowRootList instance is allocated.

> Source/WebCore/dom/ShadowRootList.cpp:83
> +    }

I feel that this kind of per-ShadowRoot cleanup doesn't fit into a method of the list.

> Source/WebCore/dom/ShadowRootList.cpp:91
> +    if (RefPtr<ShadowRoot> oldRoot = m_shadowRoots.removeHead()) {

while()? Or let's have an assertion to make it clear that we have only one list item.

> Source/WebCore/dom/ShadowRootList.cpp:93
> +

Same feeling with addShadowRoot() vs ShadowRoot.

> Source/WebCore/dom/ShadowRootList.cpp:94
> +        // Remove from rendering tree

We don't need this comment. It's obvious.

> Source/WebCore/dom/ShadowRootList.h:55
> +    Element* m_host;

Do we need this?
Comment 5 Shinya Kawanaka 2012-02-10 02:02:17 PST
Created attachment 126479 [details]
Patch
Comment 6 Shinya Kawanaka 2012-02-10 02:02:55 PST
Comment on attachment 126462 [details]
Patch

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

>> Source/WebCore/ChangeLog:12
>> +        which are a previous element and a next element respectively.
> 
> Could you elaborate why we need dually-linked list instead of single?
> When prev() is going to be used for example?

prev() will be used by a visual tree traversal. I will mention it in the ChangeLog.

>> Source/WebCore/dom/Element.cpp:1160
>> +    return false;
> 
> Is this false case possible?
> Is it feasible to prevent this from happening?

Element won't have rare data every time. So I don't think we can remove this.

>> Source/WebCore/dom/Element.cpp:1173
>> +    ElementRareData* data = ensureRareData();
> 
> Why not ElementRareData::ensureShadowList() ?
> In general, the list related method should be on ElementRareData because it owns the list.
> 
> Also, Is it really worth doing to allocating an extra heap chunk only for implementing dually-linked list?

Let's change not to allocate on the heap.

>> Source/WebCore/dom/Element.cpp:1187
>> +void Element::setShadowRoot(PassRefPtr<ShadowRoot> prpShadowRoot, ExceptionCode& ec)
> 
> Please don't use abbreviation.

Done.

>> Source/WebCore/dom/ElementRareData.h:74
>> +    ShadowRootList* m_shadowRootList;
> 
> You can use OwnPtr.

Let's not allocate another heap chunk.

>> Source/WebCore/dom/ShadowRoot.h:42
>> +    friend class WTF::DoublyLinkedListNode<ShadowRoot>;
> 
> Is it possible to assert some invariant on the ShadowRoot destructor?
> For example, Is it possible to say that the instance is already unchained on destruction?

I've added an assertion to ensure unchained.

>> Source/WebCore/dom/ShadowRootList.cpp:65
>> +void ShadowRootList::addShadowRoot(PassRefPtr<ShadowRoot> prpShadowRoot, ExceptionCode& ec)
> 
> Please don't use abbreviation.

Done.

>> Source/WebCore/dom/ShadowRootList.cpp:67
>> +    // FIXME: We don't support multiple shadow root subtrees yet.
> 
> Pelase note bug#.

Done.

>> Source/WebCore/dom/ShadowRootList.cpp:71
>> +    if (!validateShadowRoot(host()->document(), shadowRoot.get(), ec))
> 
> I hope this check also happens before ShadowRootList instance is allocated.

Done.

>> Source/WebCore/dom/ShadowRootList.cpp:83
>> +    }
> 
> I feel that this kind of per-ShadowRoot cleanup doesn't fit into a method of the list.

Done.

>> Source/WebCore/dom/ShadowRootList.cpp:91
>> +    if (RefPtr<ShadowRoot> oldRoot = m_shadowRoots.removeHead()) {
> 
> while()? Or let's have an assertion to make it clear that we have only one list item.

Done.

>> Source/WebCore/dom/ShadowRootList.cpp:93
>> +
> 
> Same feeling with addShadowRoot() vs ShadowRoot.

Done.

>> Source/WebCore/dom/ShadowRootList.cpp:94
>> +        // Remove from rendering tree
> 
> We don't need this comment. It's obvious.

Done.
Comment 7 Shinya Kawanaka 2012-02-10 03:45:18 PST
Created attachment 126492 [details]
Test
Comment 8 Shinya Kawanaka 2012-02-10 04:24:10 PST
Created attachment 126496 [details]
Test
Comment 9 Shinya Kawanaka 2012-02-10 04:35:11 PST
Created attachment 126498 [details]
Patch
Comment 10 Hajime Morrita 2012-02-12 17:13:11 PST
Comment on attachment 126498 [details]
Patch

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

> Source/WebCore/dom/ShadowRootList.h:38
> +class ShadowRootList {

Let's make this uncopyable.
Comment 11 Shinya Kawanaka 2012-02-12 17:56:50 PST
Created attachment 126695 [details]
Patch
Comment 12 Shinya Kawanaka 2012-02-12 17:57:29 PST
(In reply to comment #10)
> (From update of attachment 126498 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126498&action=review
> 
> > Source/WebCore/dom/ShadowRootList.h:38
> > +class ShadowRootList {
> 
> Let's make this uncopyable.

Done. Thank for mentioning this!
Comment 13 WebKit Review Bot 2012-02-12 20:16:05 PST
Comment on attachment 126695 [details]
Patch

Clearing flags on attachment: 126695

Committed r107525: <http://trac.webkit.org/changeset/107525>
Comment 14 WebKit Review Bot 2012-02-12 20:16:10 PST
All reviewed patches have been landed.  Closing bug.