Bug 102623

Summary: StyledElement: Make handling the "style" attribute a litte faster.
Product: WebKit Reporter: Andreas Kling <kling>
Component: DOMAssignee: 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 Flags
Patch
ojan: review+, ojan: commit-queue-
Patch for landing none

Andreas Kling
Reported 2012-11-18 16:39:14 PST
We know that "style" is never a presentation attribute, so we could avoid the isPresentationAttribute() virtual dispatch.
Attachments
Patch (4.78 KB, patch)
2012-11-18 16:40 PST, Andreas Kling 		ojan: review+
ojan: commit-queue-
DetailsFormatted DiffDiff
Patch for landing (4.78 KB, patch)
2012-11-18 17:41 PST, Andreas Kling 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Andreas Kling
Comment 1 2012-11-18 16:40:55 PST
Created attachment 174861 [details] Patch
Ojan Vafai
Comment 2 2012-11-18 16:56:11 PST
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?
Ojan Vafai
Comment 3 2012-11-18 16:59:07 PST
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?
Andreas Kling
Comment 4 2012-11-18 17:04:19 PST
(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. :)
Andreas Kling
Comment 5 2012-11-18 17:41:02 PST
Created attachment 174866 [details] Patch for landing
WebKit Review Bot
Comment 6 2012-11-18 19:17:33 PST
Comment on attachment 174866 [details] Patch for landing Clearing flags on attachment: 174866 Committed r135101: <http://trac.webkit.org/changeset/135101>
WebKit Review Bot
Comment 7 2012-11-18 19:17:37 PST
All reviewed patches have been landed. Closing bug.
Ojan Vafai
Comment 8 2012-11-18 19:18:47 PST
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.
Note You need to log in before you can comment on or make changes to this bug.