[Webkit-unassigned] [Bug 78069] Introduce ShadowRootList

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=78069





--- Comment #6 from Shinya Kawanaka <shinyak at chromium.org>  2012-02-10 02:02:55 PST ---
(From update of attachment 126462)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.