Bug 106679

Summary: Move functions from NodeRareData to ElementRareData and other classes
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: DOMAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: adamk, andersca, benjamin, buildbot, cmarcelo, dongseong.hwang, esprehn, kling, koivisto, ojan.autocc, philn, rniwa, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 106714    
Bug Blocks: 106850    
Attachments:
Description Flags
refactoring
none
Fixed micro data build
none
Addressed Ben's comments; merged to ToT benjamin: review+, benjamin: commit-queue-

Description Ryosuke Niwa 2013-01-11 10:53:47 PST 
Make NodeRareData simpler to prepare to fix bugs 79740 and 106586.
Comment 1 Ryosuke Niwa 2013-01-11 11:11:43 PST 
Created attachment 182374 [details]
refactoring
Comment 2 EFL EWS Bot 2013-01-11 11:29:07 PST 
Comment on attachment 182374 [details]
refactoring

Attachment 182374 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15804458
Comment 3 Ryosuke Niwa 2013-01-11 11:33:05 PST 
Created attachment 182383 [details]
Fixed micro data build
Comment 4 Build Bot 2013-01-11 14:21:14 PST 
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
Comment 5 Build Bot 2013-01-11 15:24:28 PST 
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
Comment 6 Benjamin Poulain 2013-01-11 22:59:19 PST 
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 :)
Comment 7 Ryosuke Niwa 2013-01-11 23:37:54 PST 
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.
Comment 8 Ryosuke Niwa 2013-01-14 14:34:34 PST 
Created attachment 182629 [details]
Addressed Ben's comments; merged to ToT
Comment 9 Benjamin Poulain 2013-01-14 14:56:19 PST 
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.
Comment 10 Ryosuke Niwa 2013-01-14 15:00:39 PST 
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.
Comment 11 EFL EWS Bot 2013-01-14 15:03:31 PST 
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
Comment 12 Ryosuke Niwa 2013-01-14 15:58:57 PST 
Committed r139681: <http://trac.webkit.org/changeset/139681>
Comment 13 Dongseong Hwang 2013-01-14 17:00:56 PST 
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’
Comment 14 Benjamin Poulain 2013-01-14 17:12:07 PST 
(In reply to comment #13)
> After this patch, it seems build fail on efl port.

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