[webkit-reviews] review denied: [Bug 106679] Move functions from NodeRareData to ElementRareData and other classes : [Attachment 182383] Fixed micro data build

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Ryosuke Niwa
<rniwa at webkit.org>'s request for review:
Bug 106679: Move functions from NodeRareData to ElementRareData and other
classes
https://bugs.webkit.org/show_bug.cgi?id=106679

Attachment 182383: Fixed micro data build
https://bugs.webkit.org/attachment.cgi?id=182383&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=182383&action=review


Gorgeous. :)

I think the ChangeLog is a bit terse on why you do some changes.
Some comments bellow:

> Source/WebCore/dom/Element.cpp:237
> +void Element::setTabIndexExplicitly(int tabIndex)

m_tabIndex type is short.

> Source/WebCore/dom/ElementRareData.h:51
> +    
> +    short tabIndex() const { return m_tabIndex; }
> +    void setTabIndexExplicitly(short index) { m_tabIndex = index;
m_tabIndexWasSetExplicitly = true; }
> +    bool tabIndexSetExplicitly() const { return m_tabIndexWasSetExplicitly;
}
> +    void clearTabIndexExplicitly() { m_tabIndex = 0;
m_tabIndexWasSetExplicitly = false; }

I am not sure why you did not move both attribute from NodeRareData.h to
ElementRareData.h.

Is it because the structure is already completely packed?
If so, it would be nice to say so in the ChangeLog.

> Source/WebCore/dom/Node.cpp:2195
> -    return hasRareData() ? rareData()->mutationObserverRegistry() : 0;
> +    if (!hasRareData())
> +	   return 0;
> +    NodeMutationObserverData* data = rareData()->mutationObserverData();
> +    if (!data)
> +	   return 0;
> +    return &data->registry;

I don't get the reason for this change. This is essentially shifting some
responsibilities from NodeRareData to Node. I don't really see the benefit
(something about the why in the ChangeLog would be nice).

> Source/WebCore/dom/Node.cpp:2205
> -    return hasRareData() ? rareData()->transientMutationObserverRegistry() :
0;
> +    if (!hasRareData())
> +	   return 0;
> +    NodeMutationObserverData* data = rareData()->mutationObserverData();
> +    if (!data)
> +	   return 0;
> +    return &data->transientRegistry;

ditto.

> Source/WebCore/dom/NodeRareData.h:229
> +struct NodeMutationObserverData {

No WTF_MAKE_FAST_ALLOCATED?

> Source/WebCore/dom/NodeRareData.h:237
> +class NodeMicroDataTokenLists {

Ditto.

> Source/WebCore/dom/NodeRareData.h:252
> +	   if (!m_itemProp)
> +	       m_itemProp = DOMSettableTokenList::create();
> +	   m_itemProp->setValue(value);

itemProp()->setValue(value)?

> Source/WebCore/dom/NodeRareData.h:266
> +	   if (!m_itemRef)
> +	       m_itemRef = DOMSettableTokenList::create();
> +	   m_itemRef->setValue(value);

itemRef()->setValue(value)?

> Source/WebCore/dom/NodeRareData.h:280
> +	   if (!m_itemType)
> +	       m_itemType = DOMSettableTokenList::create();
> +	   m_itemType->setValue(value);

etc :)