[webkit-reviews] review denied: [Bug 78069] Introduce ShadowRootList : [Attachment 126462] Patch

[bugzilla-daemon at webkit.org]

MORITA Hajime <morrita at google.com> has denied Shinya Kawanaka
<shinyak at chromium.org>'s request for review:
Bug 78069: Introduce ShadowRootList
https://bugs.webkit.org/show_bug.cgi?id=78069

Attachment 126462: Patch
https://bugs.webkit.org/attachment.cgi?id=126462&action=review

------- Additional Comments from MORITA Hajime <morrita at google.com>
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?