[Webkit-unassigned] [Bug 78069] Introduce ShadowRootList

[bugzilla-daemon at webkit.org]

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


MORITA Hajime <morrita at google.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #126462|review?                     |review-
               Flag|                            |




--- Comment #4 from MORITA Hajime <morrita at google.com>  2012-02-10 00:19:15 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?

> 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?

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