[Webkit-unassigned] [Bug 78069] Introduce ShadowRootList
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Feb 10 02:02:55 PST 2012
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.
More information about the webkit-unassigned
mailing list