Bug 106679 – Move functions from NodeRareData to ElementRareData and other classes

• Component: DOM
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Make NodeRareData simpler to prepare to fix bugs 79740 and 106586.

[Ryosuke Niwa]

Created attachment 182374 [details]
refactoring

[EFL EWS Bot]

Comment on attachment 182374 [details]
refactoring

Attachment 182374 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15804458

[Ryosuke Niwa]

Created attachment 182383 [details]
Fixed micro data build

[Build Bot]

Comment on attachment 182383 [details]
Fixed micro data build

Attachment 182383 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15832003

New failing tests:
platform/mac/accessibility/progressbar.html

[Build Bot]

Comment on attachment 182383 [details]
Fixed micro data build

Attachment 182383 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15822030

New failing tests:
platform/mac/accessibility/progressbar.html

[Benjamin Poulain]

Comment on attachment 182383 [details]
Fixed micro data build

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 :)

[Ryosuke Niwa]

Comment on attachment 182383 [details]
Fixed micro data build

View in context: https://bugs.webkit.org/attachment.cgi?id=182383&action=review

>> Source/WebCore/dom/ElementRareData.h:51
>> +    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.

That's because of WebVTT stuff but that has been refactored in the bug 106714 so I can upload a new patch that moves all bit fields from NodeRareData to ElementRareData.

>> Source/WebCore/dom/Node.cpp:2195
>> +    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).

This isn't about moving responsibilities from NodeRareData to Node. It's about isolating details of NodeMutationObserverData from NodeRareData.
I'm going to write a follow up patch, which will move most of this code and related code into NodeMutationObserverData.
That'll allow us to detect when NodeMutationObserverData is no longer needed, which allows us to remove NodeRareData.
I'll clarify this in a new change log.

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

Oops, I need WTF_MAKE_FAST_ALLOCATED and WTF_MAKE_NONCOPYABLE.

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

Will do.

>> Source/WebCore/dom/NodeRareData.h:252
>> +        m_itemProp->setValue(value);
> 
> itemProp()->setValue(value)?

That's a good point. Will do.

At some point, we'll need to move more code into this class so that we can detect when the object is no longer needed.

[Ryosuke Niwa]

Created attachment 182629 [details]
Addressed Ben's comments; merged to ToT

[Benjamin Poulain]

Comment on attachment 182629 [details]
Addressed Ben's comments; merged to ToT

View in context: https://bugs.webkit.org/attachment.cgi?id=182629&action=review

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

int -> short

> Source/WebCore/dom/NodeRareData.cpp:43
>  struct SameSizeAsNodeRareData {
>      void* m_pointer[4];
> -    unsigned m_indicesAndBitfields[2];
> -
>  #if ENABLE(MICRODATA)

It would be nice to have a similar compile assertion for ElementRareData.

[Ryosuke Niwa]

Comment on attachment 182629 [details]
Addressed Ben's comments; merged to ToT

View in context: https://bugs.webkit.org/attachment.cgi?id=182629&action=review

>> Source/WebCore/dom/NodeRareData.cpp:43
>>  #if ENABLE(MICRODATA)
> 
> It would be nice to have a similar compile assertion for ElementRareData.

Yeah, I'm doing that in a follow up.

[EFL EWS Bot]

Comment on attachment 182629 [details]
Addressed Ben's comments; merged to ToT

Attachment 182629 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15872628

[Ryosuke Niwa]

Committed r139681: <http://trac.webkit.org/changeset/139681>

[Dongseong Hwang]

After this patch, it seems build fail on efl port.


/media/WDDisk/workspace/WebKit/WebKit/Source/WebCore/dom/NodeRareData.h:269:26: error: ISO C++ forbids declaration of ‘DOMSettableTokenList’ with no type [-fpermissive]
/media/WDDisk/workspace/WebKit/WebKit/Source/WebCore/dom/NodeRareData.h:269:30: error: declaration of ‘int WebCore::NodeMicroDataTokenLists::DOMSettableTokenList()’ [-fpermissive]
/media/WDDisk/workspace/WebKit/WebKit/Source/WebCore/html/DOMSettableTokenList.h:38:7: error: changes meaning of ‘DOMSettableTokenList’ from ‘class WebCore::DOMSettableTokenList’ [-fpermissive]
/media/WDDisk/workspace/WebKit/WebKit/Source/WebCore/dom/NodeRareData.h:271:40: error: type/value mismatch at argument 1 in template parameter list for ‘template<class T> class WTF::RefPtr’
/media/WDDisk/workspace/WebKit/WebKit/Source/WebCore/dom/NodeRareData.h:271:40: error:   expected a type, got ‘WebCore::NodeMicroDataTokenLists::DOMSettableTokenList’
/media/WDDisk/workspace/WebKit/WebKit/Source/WebCore/dom/NodeRareData.h:272:40: error: type/value mismatch at argument 1 in template parameter list for ‘template<class T> class WTF::RefPtr’
/media/WDDisk/workspace/WebKit/WebKit/Source/WebCore/dom/NodeRareData.h:272:40: error:   expected a type, got ‘WebCore::NodeMicroDataTokenLists::DOMSettableTokenList’
/media/WDDisk/workspace/WebKit/WebKit/Source/WebCore/dom/NodeRareData.h:273:40: error: type/value mismatch at argument 1 in template parameter list for ‘template<class T> class WTF::RefPtr’
/media/WDDisk/workspace/WebKit/WebKit/Source/WebCore/dom/NodeRareData.h:273:40: error:   expected a type, got ‘WebCore::NodeMicroDataTokenLists::DOMSettableTokenList’
/media/WDDisk/workspace/WebKit/WebKit/Source/WebCore/dom/NodeRareData.h: In static member function ‘static WTF::PassOwnPtr<WebCore::NodeMicroDataTokenLists> WebCore::NodeMicroDataTokenLists::create()’:
/media/WDDisk/workspace/WebKit/WebKit/Source/WebCore/dom/NodeRareData.h:245:79: error: no matching function for call to ‘WebCore::NodeMicroDataTokenLists::NodeMicroDataTokenLists()’
/media/WDDisk/workspace/WebKit/WebKit/Source/WebCore/dom/NodeRareData.h:245:79: note: candidate is:
/media/WDDisk/workspace/WebKit/WebKit/Source/WebCore/dom/NodeRareData.h:243:5: note: WebCore::NodeMicroDataTokenLists::NodeMicroDataTokenLists(const WebCore::NodeMicroDataTokenLists&)
/media/WDDisk/workspace/WebKit/WebKit/Source/WebCore/dom/NodeRareData.h:243:5: note:   candidate expects 1 argument, 0 provided
/media/WDDisk/workspace/WebKit/WebKit/Source/WebCore/dom/NodeRareData.h:243:5: note:   candidate expects 1 argument, 0 provided
/media/WDDisk/workspace/WebKit/WebKit/Source/WebCore/dom/NodeRareData.h: In member function ‘WebCore::DOMSettableTokenList* WebCore::NodeMicroDataTokenLists::itemProp() const’:
/media/WDDisk/workspace/WebKit/WebKit/Source/WebCore/dom/NodeRareData.h:250:55: error: cannot convert ‘WTF::PassRefPtr<WebCore::DOMSettableTokenList>’ to ‘int’ in assignment
/media/WDDisk/workspace/WebKit/WebKit/Source/WebCore/dom/NodeRareData.h:251:27: error: request for member ‘get’ in ‘((const WebCore::NodeMicroDataTokenLists*)this)->WebCore::NodeMicroDataTokenLists::m_itemProp’, w
hich is of non-class type ‘int’
/media/WDDisk/workspace/WebKit/WebKit/Source/WebCore/dom/NodeRareData.h: In member function ‘WebCore::DOMSettableTokenList* WebCore::NodeMicroDataTokenLists::itemRef() const’:
/media/WDDisk/workspace/WebKit/WebKit/Source/WebCore/dom/NodeRareData.h:257:54: error: cannot convert ‘WTF::PassRefPtr<WebCore::DOMSettableTokenList>’ to ‘int’ in assignment
/media/WDDisk/workspace/WebKit/WebKit/Source/WebCore/dom/NodeRareData.h:258:26: error: request for member ‘get’ in ‘((const WebCore::NodeMicroDataTokenLists*)this)->WebCore::NodeMicroDataTokenLists::m_itemRef’, wh
ich is of non-class type ‘int’
/media/WDDisk/workspace/WebKit/WebKit/Source/WebCore/dom/NodeRareData.h: In member function ‘WebCore::DOMSettableTokenList* WebCore::NodeMicroDataTokenLists::itemType() const’:
/media/WDDisk/workspace/WebKit/WebKit/Source/WebCore/dom/NodeRareData.h:264:55: error: cannot convert ‘WTF::PassRefPtr<WebCore::DOMSettableTokenList>’ to ‘int’ in assignment
/media/WDDisk/workspace/WebKit/WebKit/Source/WebCore/dom/NodeRareData.h:265:27: error: request for member ‘get’ in ‘((const WebCore::NodeMicroDataTokenLists*)this)->WebCore::NodeMicroDataTokenLists::m_itemType’, w
hich is of non-class type ‘int’
/media/WDDisk/workspace/WebKit/WebKit/Source/WebCore/dom/NodeRareData.h: In member function ‘int WebCore::NodeMicroDataTokenLists::DOMSettableTokenList()’:
/media/WDDisk/workspace/WebKit/WebKit/Source/WebCore/dom/NodeRareData.h:269:30: error: no return statement in function returning non-void [-Werror=return-type]
In file included from /media/WDDisk/workspace/WebKit/WebKit/Source/WebCore/dom/ShadowRoot.cpp:43:0:
/media/WDDisk/workspace/WebKit/WebKit/Source/WebCore/dom/NodeRareData.h:269:26: error: ISO C++ forbids declaration of ‘DOMSettableTokenList’ with no type [-fpermissive]
/media/WDDisk/workspace/WebKit/WebKit/Source/WebCore/dom/NodeRareData.h:269:30: error: declaration of ‘int WebCore::NodeMicroDataTokenLists::DOMSettableTokenList()’ [-fpermissive]
/media/WDDisk/workspace/WebKit/WebKit/Source/WebCore/html/DOMSettableTokenList.h:38:7: error: changes meaning of ‘DOMSettableTokenList’ from ‘class WebCore::DOMSettableTokenList’ [-fpermissive]
/media/WDDisk/workspace/WebKit/WebKit/Source/WebCore/dom/NodeRareData.h:271:40: error: type/value mismatch at argument 1 in template parameter list for ‘template<class T> class WTF::RefPtr’
/media/WDDisk/workspace/WebKit/WebKit/Source/WebCore/dom/NodeRareData.h:271:40: error:   expected a type, got ‘WebCore::NodeMicroDataTokenLists::DOMSettableTokenList’

[Benjamin Poulain]

(In reply to comment #13)
> After this patch, it seems build fail on efl port.

+ http://trac.webkit.org/changeset/139690