Bug 115116

Summary: CSS attribute selectors cause unnecessary style recalc when setting attribute to same value.
Product: WebKit Reporter: Andreas Kling <kling>
Component: CSSAssignee: Andreas Kling <kling>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, commit-queue, esprehn+autocc, kling, koivisto, webkit-bug-importer
Priority: P2 Keywords: InRadar, Performance
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Proposed patch simon.fraser: review+

Description Andreas Kling 2013-04-24 10:06:31 PDT
Setting an attribute to the same value it already has should never trigger a style recalc due to attribute selectors.
The code is currently too dumb to figure this out, and ends up restyling the same stuff over and over.
Comment 1 Radar WebKit Bug Importer 2013-04-24 10:07:27 PDT
<rdar://problem/13727709>
Comment 2 Andreas Kling 2013-04-24 10:26:13 PDT
Created attachment 199498 [details]
Proposed patch
Comment 3 Simon Fraser (smfr) 2013-04-24 10:38:28 PDT
Comment on attachment 199498 [details]
Proposed patch

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

> Source/WebCore/ChangeLog:13
> +        This reduces unnecessary style recalculation in MAS content.

"MAS" is pretty meaningless in this context.
Comment 4 Antti Koivisto 2013-04-24 10:41:33 PDT
Comment on attachment 199498 [details]
Proposed patch

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

Better fixes involve some significant refactoring, this is ok for now.

> Source/WebCore/dom/Element.cpp:-878
>      else if (name == HTMLNames::pseudoAttr)
>          shouldInvalidateStyle |= testShouldInvalidateStyle && isInShadowTree();
>  
> -    shouldInvalidateStyle |= testShouldInvalidateStyle && styleResolver->hasSelectorForAttribute(name.localName());

It seem wrong that attributeChanged() is called when attribute does not change. It would be better to fix that.

> Source/WebCore/dom/Element.cpp:2726
> +    if (oldValue != newValue) {
> +        if (attached() && document()->styleResolver() && document()->styleResolver()->hasSelectorForAttribute(name.localName()))
> +            setNeedsStyleRecalc();
> +    }

Bit sad that the invalidation code gets spread around like this.
Comment 5 Andreas Kling 2013-04-24 10:42:21 PDT
Committed r149047: <http://trac.webkit.org/changeset/149047>