Bug 143922 - Setting inline style to the same value it already has triggers a style recalc
Summary: Setting inline style to the same value it already has triggers a style recalc
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-04-18 13:02 PDT by Simon Fraser (smfr)
Modified: 2015-04-20 11:34 PDT (History)
5 users (show)

See Also:


Attachments
Patch (33.39 KB, patch)
2015-04-18 17:33 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (33.43 KB, patch)
2015-04-18 18:27 PDT, Simon Fraser (smfr)
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2015-04-18 13:02:34 PDT
PropertySetCSSStyleDeclaration::setPropertyInternal() does:

    bool changed = m_propertySet->setProperty(propertyID, value, important, contextStyleSheet());

but as far as I can see, setProperty() returns true simply when parsing succeeded, not necessarily when the property changed.

This causes lots of extra style recalcs when scrolling Facebook.
Comment 1 Radar WebKit Bug Importer 2015-04-18 17:22:03 PDT
<rdar://problem/20603831>
Comment 2 Simon Fraser (smfr) 2015-04-18 17:33:23 PDT
Created attachment 251102 [details]
Patch
Comment 3 Simon Fraser (smfr) 2015-04-18 18:27:47 PDT
Created attachment 251104 [details]
Patch
Comment 4 Antti Koivisto 2015-04-19 02:39:46 PDT
Comment on attachment 251104 [details]
Patch

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

Ok though I would prefer a factoring that does not introduce a new bool& return value.

> Source/WebCore/css/CSSParser.cpp:1323
> +bool CSSParser::parseValue(MutableStyleProperties* declaration, CSSPropertyID propertyID, const String& string, bool important, StyleSheetContents* contextStyleSheet, bool& changed)

It would be nicer if these function returned an enum instead of two bools:

enum class ParseResult { Changed, Unchanged, Error }

or similar.

> Source/WebCore/css/CSSProperty.h:95
> +        if (!m_value && !other.m_value)
> +            return true;

You should do

if (m_value == other.m_value)
   return true;

instead. Values may be shared.

> Source/WebCore/css/StyleProperties.cpp:683
>      // When replacing an existing property value, this moves the property to the end of the list.
>      // Firefox preserves the position, and MSIE moves the property to the beginning.

Is this comment true anymore? The setProperty code looks like it replaces properties in-place.
Comment 5 Simon Fraser (smfr) 2015-04-20 11:33:29 PDT
https://trac.webkit.org/r183017
Comment 6 Simon Fraser (smfr) 2015-04-20 11:34:13 PDT
> > Source/WebCore/css/StyleProperties.cpp:683
> >      // When replacing an existing property value, this moves the property to the end of the list.
> >      // Firefox preserves the position, and MSIE moves the property to the beginning.
> 
> Is this comment true anymore? The setProperty code looks like it replaces
> properties in-place.

Didn't change this because I didn't have time to investigate.