| Summary: | StyledElement: Make handling the "style" attribute a litte faster. | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Andreas Kling <kling> | ||||||
| Component: | DOM | Assignee: | Andreas Kling <kling> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | cmarcelo, koivisto, ojan, webkit.review.bot | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 102598 | ||||||||
| Attachments: |
|
||||||||
|
Description
Andreas Kling
2012-11-18 16:39:14 PST
Created attachment 174861 [details]
Patch
Comment on attachment 174861 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174861&action=review > Source/WebCore/dom/Element.cpp:776 > + } if (name == classAttr) This is missing an else? Please fix the else issue. Otherwise, looks good. Is this actually addressing the issues that caused the regression or just fixing the top issues in the sample for that test? (In reply to comment #3) > Please fix the else issue. Otherwise, looks good. > > Is this actually addressing the issues that caused the regression or just fixing the top issues in the sample for that test? The regression is caused by taking the longer, arguably more correct, path for notifying the element that its "style" attribute has changed. (Previously, the function that clones all attributes from one element to another below Node.cloneNode would check for the "style" attribute and bypass Element::attributeChanged(), calling StyledElement::styleAttributeChanged() directly in an effort to avoid invoking the CSS parser.) I'd rather not keep that hack in there so I'm posting a couple of patches to lubricate the normal attributeChanged() path. :) Created attachment 174866 [details]
Patch for landing
Comment on attachment 174866 [details] Patch for landing Clearing flags on attachment: 174866 Committed r135101: <http://trac.webkit.org/changeset/135101> All reviewed patches have been landed. Closing bug. Makes sense. Wasn't questioning the value of these patches. Just trying to understand. Thanks! FWIW, in addition to being a performance gain, I think the code just makes more sense after this patch. |