Summary: | Move attribute storage from NamedNodeMap to ElementAttributeData | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Caio Marcelo de Oliveira Filho <cmarcelo> | ||||||||
Component: | New Bugs | Assignee: | Caio Marcelo de Oliveira Filho <cmarcelo> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | dglazkov, kling, macpherson, menard, rniwa, tkent, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 77800 | ||||||||||
Bug Blocks: | 75069 | ||||||||||
Attachments: |
|
Description
Caio Marcelo de Oliveira Filho
2012-02-02 13:02:04 PST
Created attachment 125170 [details]
Patch
Comment on attachment 125170 [details] Patch Attachment 125170 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11389972 Created attachment 125316 [details]
Patch
Comment on attachment 125316 [details]
Patch
After talking with rniwa and kling on IRC, we concluded this patch is too big.
So the plan is to first remove the noise from it by making many of the clients of NamedNodeMap to interact with Element itself. IOW, expose a way to get Attribute* from Element. Then this patch for moving attribute storage will be smaller.
Created attachment 126495 [details]
Patch
rniwa, as discussed I made this patch smaller by taking care of the noise in another patch. Compared to the previous one, I let two more things for separate changes: implementation of DOM methods (because I think they can move to Element itself, even make the code less redundant in some cases) and the parsing code that interacts with NamedNodeMap. Note that the first of those separate changes will "resolve" kling's issues with ExceptionCode appearing in ElementAttributeData in the previous patch. Could you take a look when you have time? Comment on attachment 126495 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126495&action=review r=me As suggested by Darin, we should rename ElementAttributeData to ElementAttributes at some point. > Source/WebCore/dom/Attr.h:43 > friend class NamedNodeMap; > + friend class ElementAttributeData; Nit: Alphabetical order. > Source/WebCore/dom/Element.h:242 > + ElementAttributeData* ensureAttributeData() const; > + ElementAttributeData* updatedAttributeData() const; > + ElementAttributeData* ensureUpdatedAttributeData() const; As discussed on IRC, we have way too many of these. It should be relatively easy to remove the need for ensureUpdatedAttributeData(), though not in the scope of this patch. > Source/WebCore/dom/ElementAttributeData.cpp:170 > +void ElementAttributeData::clearAttributes() > +{ > + clearClass(); > + detachAttributesFromElement(); > + m_attributes.clear(); > +} This method doesn't clear idForStyleResolution! I know you are just moving code around, so it's fine for this patch, but we need to fix this. > Source/WebCore/dom/ElementAttributeData.cpp:172 > +void ElementAttributeData::replaceAttribute(size_t index, PassRefPtr<Attribute> prpAttribute, WebCore::Element* element) No need for the WebCore:: prefix. > Source/WebCore/dom/ElementAttributeData.h:57 > + bool isEmpty() const { return !length(); } return m_attributes.isEmpty(); seems more straightforward. (In reply to comment #7) > (From update of attachment 126495 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=126495&action=review > > As suggested by Darin, we should rename ElementAttributeData to ElementAttributes at some point. Actually, let's hold off on that as it clashes with the NamedNodeMap* Node::attributes() we expose to the web. Committed r107570: <http://trac.webkit.org/changeset/107570> |