Bug 108195 – Move ElementShadow creation to ElementRareData

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Elliott Sprehn]

Move ElementShadow creation to ElementRareData

[Elliott Sprehn]

Created attachment 185248 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 185248 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        * dom/Element.cpp:

It's good WebKit practice to describe changes inline here. Darin Adler once told me: "If you're not putting anything here, might as well delete this text".

[Elliott Sprehn]

Comment on attachment 185248 [details]
Patch

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

>> Source/WebCore/ChangeLog:13
>> +        * dom/Element.cpp:
> 
> It's good WebKit practice to describe changes inline here. Darin Adler once told me: "If you're not putting anything here, might as well delete this text".

Okay.

[Elliott Sprehn]

Created attachment 185257 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 185257 [details]
Patch for landing

Clearing flags on attachment: 185257

Committed r141132: <http://trac.webkit.org/changeset/141132>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

This kind of change is OK, but the name “data” in the names of classes like NodeRareData and ElementRareData is supposed to indicate that those classes hold data but not policy, and generally speaking we want most of the code to be in the Node and Element class, not the NodeRareData and ElementRareData classes. As soon as the code was anything more than ElementShadow::create() I think we’d not want to make this sort of change.

[Elliott Sprehn]

(In reply to comment #7)
> This kind of change is OK, but the name “data” in the names of classes like NodeRareData and ElementRareData is supposed to indicate that those classes hold data but not policy, and generally speaking we want most of the code to be in the Node and Element class, not the NodeRareData and ElementRareData classes. As soon as the code was anything more than ElementShadow::create() I think we’d not want to make this sort of change.

Okay. My change is inline with all the code in NodeRareData though: see ensureNodeLists, ensureMutationObserverData, ensureMicroDataTokenLists etc.

[Darin Adler]

(In reply to comment #8)
> My change is inline with all the code in NodeRareData though: see ensureNodeLists, ensureMutationObserverData, ensureMicroDataTokenLists etc.

Sure. Yours was just the one I happened to spot. I haven’t been watching all the check-ins as much as I used to.