Summary: | Avoid unnecessary work when removing attributes from an element | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adam Klein <adamk> | ||||||||||||||
Component: | New Bugs | Assignee: | Adam Klein <adamk> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | abarth, darin, ojan, rafaelw, rniwa, tkent, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Adam Klein
2011-12-20 13:56:51 PST
Created attachment 120076 [details]
Patch
I'm seeing inspector test failures which I'm now investigating... Created attachment 120077 [details]
More changelog detail
(In reply to comment #2) > I'm seeing inspector test failures which I'm now investigating... Never mind, just the usual debug timeouts. Ready for review. Comment on attachment 120077 [details] More changelog detail Attachment 120077 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10993335 Created attachment 120088 [details]
Fix Qt build
Comment on attachment 120077 [details] More changelog detail View in context: https://bugs.webkit.org/attachment.cgi?id=120077&action=review > Source/WebCore/dom/Element.cpp:202 > + willModifyAttribute(name, m_attributeMap->attributeItem(index)->value(), nullAtom); > + > + m_attributeMap->removeAttribute(index); It's not great that some of these functions call willModifyAttribute themselves but not attributeChanged. We should try to make them symmetric as possible to make the interface less obscure. Asymmetric interfaces like this one makes the correctness harder to verify if it doesn't introduce more bugs. e.g. why is it okay not to call InspectorInstrumentation::willModifyDOMAttr here? > Source/WebCore/dom/Element.cpp:207 > void Element::setBooleanAttribute(const QualifiedName& name, bool b) > { > if (b) Could you rename b? e.g. value. > Source/WebCore/dom/NamedNodeMap.cpp:154 > RefPtr<Attr> attr = attribute->createAttrIfNeeded(m_element); Do we need to create attr here or can we get away by creating on return? > Source/WebCore/dom/NamedNodeMap.cpp:263 > -void NamedNodeMap::removeAttribute(const QualifiedName& name) > +void NamedNodeMap::replaceAttribute(size_t index, PassRefPtr<Attribute> attribute) Could you define replaceAttribute after removeAttribute so that the diff would look saner? > Source/WebCore/dom/NamedNodeMap.cpp:271 > + if (Attr* attr = m_attributes[index]->attr()) > + attr->m_element = m_element; It seems like we should define attr outside of the if condition to avoid having to duplicate "m_attributes[index]->attr()" Created attachment 120093 [details]
Update setAttribute to use indices as well
Comment on attachment 120093 [details] Update setAttribute to use indices as well View in context: https://bugs.webkit.org/attachment.cgi?id=120093&action=review > Source/WebCore/dom/NamedNodeMap.cpp:263 > -void NamedNodeMap::removeAttribute(const QualifiedName& name) > +void NamedNodeMap::replaceAttribute(size_t index, PassRefPtr<Attribute> attribute) Please re-odering these two functions if that improves the readability of diff. Comment on attachment 120077 [details] More changelog detail View in context: https://bugs.webkit.org/attachment.cgi?id=120077&action=review >> Source/WebCore/dom/Element.cpp:202 >> + m_attributeMap->removeAttribute(index); > > It's not great that some of these functions call willModifyAttribute themselves but not attributeChanged. We should try to make them symmetric as possible to make the interface less obscure. Asymmetric interfaces like this one makes the correctness harder to verify if it doesn't introduce more bugs. e.g. why is it okay not to call InspectorInstrumentation::willModifyDOMAttr here? Agreed that it's not ideal. Once I've pruned NamedNodeMap down to the simplest possible interface I aim to fix up who calls attributeChanged and willModifyAttribute appropriately. As for InspectorInstrumentation, I didn't want to change any behavior in this change: it's basically totally broken and needs fixing. I guess I should file a bug with the inspector folks. >> Source/WebCore/dom/Element.cpp:207 >> if (b) > > Could you rename b? e.g. value. Done. >> Source/WebCore/dom/NamedNodeMap.cpp:154 >> RefPtr<Attr> attr = attribute->createAttrIfNeeded(m_element); > > Do we need to create attr here or can we get away by creating on return? The Attribute* would be invalid by the time we get to the return. We're not doing any refcount churn here (since we return attr.release()), so I'm not sure I see a problem with this construction. >> Source/WebCore/dom/NamedNodeMap.cpp:263 >> +void NamedNodeMap::replaceAttribute(size_t index, PassRefPtr<Attribute> attribute) > > Could you define replaceAttribute after removeAttribute so that the diff would look saner? Moved, we'll see if it helps. >> Source/WebCore/dom/NamedNodeMap.cpp:271 >> + attr->m_element = m_element; > > It seems like we should define attr outside of the if condition to avoid having to duplicate "m_attributes[index]->attr()" These are different m_attributes[index]->attr()s: the first one is the one we're about to remove, and the second is the new one. Created attachment 120096 [details]
Patch for landing
Comment on attachment 120096 [details] Patch for landing Attachment 120096 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10993359 Created attachment 120106 [details]
Patch for landing
Committed r103373: <http://trac.webkit.org/changeset/103373> |