Bug 119428 - Removing an empty style sheet shouldn't trigger style recalc.
Summary: Removing an empty style sheet shouldn't trigger 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: Andreas Kling
URL:
Keywords: InRadar, Performance
Depends on:
Blocks:
 
Reported: 2013-08-01 21:44 PDT by Andreas Kling
Modified: 2013-08-02 02:22 PDT (History)
9 users (show)

See Also:


Attachments
Proposed patch (4.81 KB, patch)
2013-08-01 22:12 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Proposed patch v2 (6.83 KB, patch)
2013-08-01 22:55 PDT, Andreas Kling
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2013-08-01 21:44:40 PDT
Removing an empty style sheet shouldn't trigger style recalc.
Comment 1 Radar WebKit Bug Importer 2013-08-01 21:44:51 PDT
<rdar://problem/14629045>
Comment 2 Andreas Kling 2013-08-01 22:12:47 PDT
Created attachment 207980 [details]
Proposed patch
Comment 3 Ryosuke Niwa 2013-08-01 22:26:21 PDT
Comment on attachment 207980 [details]
Proposed patch

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

> Source/WebCore/css/CSSStyleSheet.cpp:172
> +    owner->styleResolverChanged(RecalcStyleIfNeeded);

Maybe we want to add DeferRecalcStyleIfNeeded so that we don't synchronously do a style recalc when a non-empty style element is modified.

> Source/WebCore/dom/DocumentStyleSheetCollection.cpp:430
> -static void filterEnabledCSSStyleSheets(Vector<RefPtr<CSSStyleSheet> >& result, const Vector<RefPtr<StyleSheet> >& sheets)
> +static void filterEnabledNonemptyCSSStyleSheets(Vector<RefPtr<CSSStyleSheet> >& result, const Vector<RefPtr<StyleSheet> >& sheets)

Can we rename this to collect* or get*?
Comment 4 Andreas Kling 2013-08-01 22:55:33 PDT
Created attachment 207982 [details]
Proposed patch v2

Add a DeferRecalcStyleIfNeeded mode as suggested by rniwa.
Comment 5 Antti Koivisto 2013-08-02 02:06:17 PDT
Comment on attachment 207982 [details]
Proposed patch v2

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

> Source/WebCore/css/CSSStyleSheet.h:77
> -    virtual void clearOwnerNode() OVERRIDE { didMutate(); m_ownerNode = 0; }
> +    virtual void clearOwnerNode() OVERRIDE;

didMutate() (and so this performance regression) was added here: http://trac.webkit.org/changeset/144713

> Source/WebCore/dom/Document.h:200
> -enum StyleResolverUpdateFlag { RecalcStyleImmediately, DeferRecalcStyle, RecalcStyleIfNeeded };
> +enum StyleResolverUpdateFlag { RecalcStyleImmediately, DeferRecalcStyle, RecalcStyleIfNeeded, DeferRecalcStyleIfNeeded };

It is unfortunate that this gets even more complicated. It is hard to reason from these.
Comment 6 Andreas Kling 2013-08-02 02:22:17 PDT
Landed: http://trac.webkit.org/changeset/153641
Corrected bugzilla link: http://trac.webkit.org/changeset/153642 (Duh)