[webkit-reviews] review granted: [Bug 128321] Check selectors exactly when invalidating style : [Attachment 223362] patch

[bugzilla-daemon at webkit.org]

Andreas Kling <akling at apple.com> has granted Antti Koivisto <koivisto at iki.fi>'s
request for review:
Bug 128321: Check selectors exactly when invalidating style
https://bugs.webkit.org/show_bug.cgi?id=128321

Attachment 223362: patch
https://bugs.webkit.org/attachment.cgi?id=223362&action=review

------- Additional Comments from Andreas Kling <akling at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=223362&action=review


r=me with some tweaks.

Q: Can we piggyback on the already massaged data in the Document's
StyleResolver here?

> Source/WebCore/ChangeLog:9
> +	   With selectors are really fast to match with the JIT. Take advantage
of it by invalidating
> +	   document style exactly when new stylesheets arrive (instead of
heuristics).

I don't think you need the "With" here. We should remove it to achieve 60fps on
mobile in 2014.

> Source/WebCore/css/StyleInvalidationAnalysis.cpp:40
> +static bool shouldDirtyAllStyle(const Vector<RefPtr<StyleRuleBase>> rules)

We should pass the rules vector by (const) reference here.

> Source/WebCore/css/StyleInvalidationAnalysis.cpp:42
> +    for (auto rule : rules) {

Ref churn here. auto& would avoid it.

> Source/WebCore/css/StyleInvalidationAnalysis.cpp:57
> +    for (auto import : sheet.importRules()) {

Same here.

> Source/WebCore/css/StyleInvalidationAnalysis.cpp:70
> +    for (auto sheet : sheets) {

Sam has convinced me to start writing "auto*" to clarify that there's no
by-value copy.

> Source/WebCore/css/StyleInvalidationAnalysis.cpp:84
> +    for (auto sheet : sheets)

Same comment here.