StyleResolver should update RuleSets lazily.

StyleResolver should update RuleSets lazily.

This is a fixing patch for preformance regression caused by r159957.
Before applying r159957, removing a style element (or modifying a style element's textcontent)
destroys StyleResolver, e.g. style.textContent = '';

After applying r159957, we don't need to destroy StyleResolver.
The r159957 makes us to avoid FullStyleRecalc and full RuleSet re-creation (document,
and shadow trees).

But the patch caused Parser/css-parser-yui.html's performance regression.
Because firstly the performance test destroys StyleResolver and skip StyleResolver::appendAuthorStyleSheets.
(If we have no StyleResolver, we don't need to update RuleSet, StyleInvalidationAnalysis, and so on)

So we should update RuleSets lazily. This solution improves Parser/css-parser-yui.html's performance again.

BUG=305885, 308796
TEST=all existing tests should pass.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162187

[tasak, 2013-10-25 02:22:39]

Applying this patch, Parer/css-parser-yui.html:

Ignoring warm-up run (324.41025579304556 runs/s)
326.9072703849519 runs/s
326.2616128918332 runs/s
326.5104314772393 runs/s
325.8625464190681 runs/s
325.1617099088095 runs/s

Before:
Ignoring warm-up run (287.75508689932917 runs/s)
288.2252139679534 runs/s
288.2618465326381 runs/s
288.17311342964416 runs/s
287.6531426678328 runs/s
287.53960202569135 runs/s

[esprehn, 2013-10-25 02:37:57]

You have the checks duplicated both inside styleResolver() and outside at call
sites.

https://codereview.chromium.org/42543007/diff/1/Source/core/css/resolver/Styl...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/42543007/diff/1/Source/core/css/resolver/Styl...
Source/core/css/resolver/StyleResolver.cpp:187: bool
rulesHaveViewportOrFontFaceRule(const Vector<RefPtr<StyleRuleBase> >& rules)
These are missing static.

https://codereview.chromium.org/42543007/diff/1/Source/core/css/resolver/Styl...
Source/core/css/resolver/StyleResolver.cpp:197: bool
styleSheetHasViewportOrFontFaceRule(StyleSheetContents* sheet)
static

https://codereview.chromium.org/42543007/diff/1/Source/core/css/resolver/Styl...
Source/core/css/resolver/StyleResolver.cpp:213: bool
canAppendAuthorStyleSheetsLazily(unsigned firstNew, const
Vector<RefPtr<CSSStyleSheet> >& styleSheets)
static

https://codereview.chromium.org/42543007/diff/1/Source/core/css/resolver/Styl...
Source/core/css/resolver/StyleResolver.cpp:225: // We cannot lazy apppend
stylesheets if the sheets have @viewport and @font-face rules.
extra p in append. Also why can't we lazy append them? We should fix the system
to always allow lazy append (in a future patch)

https://codereview.chromium.org/42543007/diff/1/Source/core/css/resolver/Styl...
Source/core/css/resolver/StyleResolver.cpp:254: continue;
What's this case for?

https://codereview.chromium.org/42543007/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/42543007/diff/1/Source/core/dom/Document.cpp#...
Source/core/dom/Document.cpp:1706:
m_styleResolver->appendPendingAuthorStyleSheets();
You shouldn't be repeated this everywhere. Just like with style and layout there
should be an updateSheetsIfNeeded() like call that's inline and returns early if
no update is needed.

https://codereview.chromium.org/42543007/diff/1/Source/core/dom/Document.cpp#...
Source/core/dom/Document.cpp:1897: resolver->appendPendingAuthorStyleSheets();
This doesn't make sense. You call appendPendingAuthorStyleSheets() inside
::styleResolver(), why do you again check it here?

https://codereview.chromium.org/42543007/diff/1/Source/core/dom/Document.cpp#...
Source/core/dom/Document.cpp:1905: resolver->appendPendingAuthorStyleSheets();
Same.

https://codereview.chromium.org/42543007/diff/1/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/42543007/diff/1/Source/core/dom/Document.h#ne...
Source/core/dom/Document.h:465: appendPendingAuthorStyleSheets();
You can't put a function call in here. This is really hot, we call
styleResolver() all over the place.

[tasak, 2013-10-25 10:10:09]

Thank you for reviewing.

> You have the checks duplicated both inside styleResolver() and outside at call
sites.

I'm sorry.
I removed the check code from styleResolver() and moved into callers.

https://codereview.chromium.org/42543007/diff/1/Source/core/css/resolver/Styl...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/42543007/diff/1/Source/core/css/resolver/Styl...
Source/core/css/resolver/StyleResolver.cpp:187: bool
rulesHaveViewportOrFontFaceRule(const Vector<RefPtr<StyleRuleBase> >& rules)
On 2013/10/25 02:37:58, esprehn wrote:
> These are missing static.

Yeah, thanks.
Done.

https://codereview.chromium.org/42543007/diff/1/Source/core/css/resolver/Styl...
Source/core/css/resolver/StyleResolver.cpp:197: bool
styleSheetHasViewportOrFontFaceRule(StyleSheetContents* sheet)
On 2013/10/25 02:37:58, esprehn wrote:
> static

Done.

https://codereview.chromium.org/42543007/diff/1/Source/core/css/resolver/Styl...
Source/core/css/resolver/StyleResolver.cpp:213: bool
canAppendAuthorStyleSheetsLazily(unsigned firstNew, const
Vector<RefPtr<CSSStyleSheet> >& styleSheets)
On 2013/10/25 02:37:58, esprehn wrote:
> static

Done.

https://codereview.chromium.org/42543007/diff/1/Source/core/css/resolver/Styl...
Source/core/css/resolver/StyleResolver.cpp:225: // We cannot lazy apppend
stylesheets if the sheets have @viewport and @font-face rules.
On 2013/10/25 02:37:58, esprehn wrote:
> extra p in append. Also why can't we lazy append them? We should fix the
system
> to always allow lazy append (in a future patch)

Yeah, I agree that we should solve in a future patch. I added FIXME.

Currently I have no good solutions for @viewport and @font-face rules.

Talking about @viewport rules, I found that the following tests fail:

css3/device-adapt/viewport-height-extend-to-zoom.html
css3/device-adapt/viewport-min-width-extend-to-zoom.html
css3/device-adapt/viewport-width-extend-to-zoom.html

I couldn't find any good place to update viewport style resolver.

Talking about @font-face, I'm afraid that font-loading becomes slow.
I think, "addFontFaceRule" triggers font-loading.
If we delays, we probably need to wait more time until fonts are loaded.

For example, fast/loader/resource-request-callbacks.html fails:

-main frame - css requested 'gw432047.ttf'
 main frame - img requested 'dice.png'
 main frame - img requested 'dice.png'
 main frame - img requested 'not-existing.png'
 main frame - script requested 'js-test-post.js'
+main frame - css requested 'gw432047.ttf'

The test has @font-face rule which needs to load gw432047.ttf.

https://codereview.chromium.org/42543007/diff/1/Source/core/css/resolver/Styl...
Source/core/css/resolver/StyleResolver.cpp:254: continue;
On 2013/10/25 02:37:58, esprehn wrote:
> What's this case for?

c.f. fast/dom/shadow/remove-styles-in-shadow-crash-2.html

I would like to explain the test.

Suppose that we have a shadow tree which has more than two style elements, e.g.

div#host
    #shadow-root
        div#A
            style#B
              ...
            style#C
              ...
        ...

Now we are going to remove div#A. In this case, style#B and style#C are also
removed via ChildNodeRemovalNotifier.

So when removing style#B, StyleElement::removedFromDocument is invoked.
StyleEngine removes style#B from active stylesheets, and update active
stylesheets.

But style#C (being destroyed) is still in the active stylesheets.
style#C's inDocument() is still true, ownerNode() is not null, but style#C's
containingShadowRoot() is 0.

So style#C's rules are inserted into document's ruleset. This causes crash in
SelectorChecker.

I would like to refactor "ScopedStyleResolver::scopingNodeFor".
The method returns 0 even if a given StyleSheetContents is invalid. This is very
bad behavior.
(I think, containingShadowRoot() == 0 means that the given StyleSheetContents is
invalid)

https://codereview.chromium.org/42543007/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/42543007/diff/1/Source/core/dom/Document.cpp#...
Source/core/dom/Document.cpp:1706:
m_styleResolver->appendPendingAuthorStyleSheets();
On 2013/10/25 02:37:58, esprehn wrote:
> You shouldn't be repeated this everywhere. Just like with style and layout
there
> should be an updateSheetsIfNeeded() like call that's inline and returns early
if
> no update is needed.

I see.
I added an inline function to StyleResolver.h, and modified to sue the function.

https://codereview.chromium.org/42543007/diff/1/Source/core/dom/Document.cpp#...
Source/core/dom/Document.cpp:1897: resolver->appendPendingAuthorStyleSheets();
On 2013/10/25 02:37:58, esprehn wrote:
> This doesn't make sense. You call appendPendingAuthorStyleSheets() inside
> ::styleResolver(), why do you again check it here?

Yeah, this is my mistake.
Since I removed the call from styleResolver(), I used the inline function in
StyleResolver.h here.

https://codereview.chromium.org/42543007/diff/1/Source/core/dom/Document.cpp#...
Source/core/dom/Document.cpp:1905: resolver->appendPendingAuthorStyleSheets();
On 2013/10/25 02:37:58, esprehn wrote:
> Same.

Done.

https://codereview.chromium.org/42543007/diff/1/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/42543007/diff/1/Source/core/dom/Document.h#ne...
Source/core/dom/Document.h:465: appendPendingAuthorStyleSheets();
On 2013/10/25 02:37:58, esprehn wrote:
> You can't put a function call in here. This is really hot, we call
> styleResolver() all over the place.

I see.
I moved appendPendingAuthorStyleSheets to callers if callers need.

[tasak, 2013-11-05 10:35:55]

I added filter function to process @font-face and @viewport when appending
author stylesheets lazily.

Looking at css3/device-adapt/viewport-height-extend-to-zoom.html,
(1) adding stylesheet (containing @viewport) to pending list,
(2) destroying style resolver (caused by addUserStyleSheet),
(3) checking viewport (Internals::viewportAsText),
(4) creating style resolver.

So I think, we have the following 3 solutions:
(Solution A): updating viewport before destroying style resolver,
(Solution B): filtering @viewport in lazyAppendAuthorStyleSheets,
(Solution C): changing addUserStyleSheets not to destory StyleResolver.

My patch is (Solution B).

(Solution C) requires modifying
StyleEngine::invalidateInjectedStyleSheetCache(), but FIXME says "batch updates
lots of sheets so we can't call addedStyleSheet() or removedStyleSheet()".
So I think, we should use clearStyleResolver via styleResolverChanged.

[eseidel, 2013-11-05 10:40:38]

Sorry, too tired tonight, but I'll definitely review this tomorrow.

[eseidel, 2013-11-06 01:39:10]

I'm still not fully understanding the patch, I'll need to read it again in a few
hours. :)

https://codereview.chromium.org/42543007/diff/250001/Source/core/css/RuleSet.cpp
File Source/core/css/RuleSet.cpp (right):

https://codereview.chromium.org/42543007/diff/250001/Source/core/css/RuleSet....
Source/core/css/RuleSet.cpp:402: } else if (rule->isViewportRule() &&
!(addRuleFlags & RuleSkipsFontFaceAndViewportRule)) {
The double negative reads a bit odd.  But I guess your goal here was for the 0
value to mean "no special handling" where as currently skipping
font-face/viewport is special.

https://codereview.chromium.org/42543007/diff/250001/Source/core/css/RuleSet.h
File Source/core/css/RuleSet.h (right):

https://codereview.chromium.org/42543007/diff/250001/Source/core/css/RuleSet....
Source/core/css/RuleSet.h:38: RuleSkipsFontFaceAndViewportRule = 1 << 3
Does this mean you can have rules which ignore the fact that they're defined
inside an @font-face or @viewport?  I must be misunderstanding, as that doesn't
make sense to me...

https://codereview.chromium.org/42543007/diff/250001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/42543007/diff/250001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:193: const StyleRuleFontFace*
fontFaceRule = static_cast<StyleRuleFontFace*>(rule);
Do we have a toStyleRuleFontFace() method instead (which does necessarily
RELEASE_ASSERTs, etc.?)

https://codereview.chromium.org/42543007/diff/250001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:200: StyleRuleMedia* mediaRule =
static_cast<StyleRuleMedia*>(rule);
We've slowly been moving away from using static_cast directly in Blink when
possible and always using toFoo() functions instead since those include ASSERTS
to catch bad casts (before security vulnerabilities happen).

https://codereview.chromium.org/42543007/diff/250001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResolver.h (left):

https://codereview.chromium.org/42543007/diff/250001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.h:267: const RuleFeatureSet&
ruleFeatureSet() const { return m_features; }
I'm confused.  This can now be stale right?  Don't all callers need to call
ensureRuleFeatureSet?

[tasak, 2013-11-06 05:42:25]

Thank you for reviewing.

https://codereview.chromium.org/42543007/diff/250001/Source/core/css/RuleSet.cpp
File Source/core/css/RuleSet.cpp (right):

https://codereview.chromium.org/42543007/diff/250001/Source/core/css/RuleSet....
Source/core/css/RuleSet.cpp:402: } else if (rule->isViewportRule() &&
!(addRuleFlags & RuleSkipsFontFaceAndViewportRule)) {
On 2013/11/06 01:39:10, eseidel wrote:
> The double negative reads a bit odd.  But I guess your goal here was for the 0
> value to mean "no special handling" where as currently skipping
> font-face/viewport is special.

Yes, 0 means "no special handling". The 0 is used when creating StyleResolver.

1 means, @font-face rules and @viewport rules have been already processed, so we
don't need to process the rules now.

https://codereview.chromium.org/42543007/diff/250001/Source/core/css/RuleSet.h
File Source/core/css/RuleSet.h (right):

https://codereview.chromium.org/42543007/diff/250001/Source/core/css/RuleSet....
Source/core/css/RuleSet.h:38: RuleSkipsFontFaceAndViewportRule = 1 << 3
On 2013/11/06 01:39:10, eseidel wrote:
> Does this mean you can have rules which ignore the fact that they're defined
> inside an @font-face or @viewport?  I must be misunderstanding, as that
doesn't
> make sense to me...

I mean, if RuleSkipsFontFaceAndViewportRule is set, we will ignore @font-face
and @viewport in a given stylesheet.

Because the rules have been already processed in
filterFontFaceAndViewportRulesFromAuthorStyleSheets (invoked from
lazyAppendAuthorStyleSheets).

I'm using the flag to avoid invoking "addViewportRule" or "addFontFaceRule" from
appendPendingAuthorStyleSheets.

https://codereview.chromium.org/42543007/diff/250001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/42543007/diff/250001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:193: const StyleRuleFontFace*
fontFaceRule = static_cast<StyleRuleFontFace*>(rule);
On 2013/11/06 01:39:10, eseidel wrote:
> Do we have a toStyleRuleFontFace() method instead (which does necessarily
> RELEASE_ASSERTs, etc.?)

Done.

https://codereview.chromium.org/42543007/diff/250001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:200: StyleRuleMedia* mediaRule =
static_cast<StyleRuleMedia*>(rule);
On 2013/11/06 01:39:10, eseidel wrote:
> We've slowly been moving away from using static_cast directly in Blink when
> possible and always using toFoo() functions instead since those include
ASSERTS
> to catch bad casts (before security vulnerabilities happen).

Done.

https://codereview.chromium.org/42543007/diff/250001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResolver.h (left):

https://codereview.chromium.org/42543007/diff/250001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.h:267: const RuleFeatureSet&
ruleFeatureSet() const { return m_features; }
On 2013/11/06 01:39:10, eseidel wrote:
> I'm confused.  This can now be stale right?  Don't all callers need to call
> ensureRuleFeatureSet?

I left the method for fast-path, e.g. Document::createStyleResolver or
something.
However, I agree that it is very difficult to find which method we should use.
So I removed "ruleFeatureSet" and replaced "ruleFeatureSet" with
"ensureRuleFeatureSet"  (or just m_features in StyleResolver.cpp).

[tasak, 2013-11-13 11:19:29]

Would you review this CL?

[dglazkov, 2013-11-13 16:29:22]

I think the reason you're having problems getting people to review this is
because the patch is too large and does too many things.

I looked over it, and I had problems understanding all of the effects.

I don't like that you're requiring a appendPendingStyleSheetsIfNeeded call at
every callsite that needs to look at rules.

Filtering viewport rules seems a bit like a workaround.

Also, in https://codereview.chromium.org/63713002/ ksakamoto@  made fonts load
more lazily. I wonder if this helps you in any way?

https://codereview.chromium.org/42543007/diff/520001/Source/core/css/RuleSet.h
File Source/core/css/RuleSet.h (right):

https://codereview.chromium.org/42543007/diff/520001/Source/core/css/RuleSet....
Source/core/css/RuleSet.h:113: void addRulesFromSheet(StyleSheetContents*, const
MediaQueryEvaluator&, StyleResolver* = 0, const ContainerNode* = 0, bool
viewportRuleIsProcessed = false);
Let's avoid bools as params. Enums are teh cool.

https://codereview.chromium.org/42543007/diff/520001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/42543007/diff/520001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:237: // If we delays
"addFontFaceRule", we probably wait more time until fonts are loaded.
/delays/delay/s ... /probably/could/s

[tasak, 2013-11-14 09:24:20]

Thank you for comments.

On 2013/11/13 16:29:22, Dimitri Glazkov wrote:
> I think the reason you're having problems getting people to review this is
> because the patch is too large and does too many things.
> 
> I looked over it, and I had problems understanding all of the effects.
> 
> I don't like that you're requiring a appendPendingStyleSheetsIfNeeded call at
> every callsite that needs to look at rules.

I know. 

Firstly I added the method to createStyleResolver and styleResolverIfExists.

But the solution was rejected. So I added the method to callers.

I would like to ask your advice.

> Filtering viewport rules seems a bit like a workaround.

I agree. However, I know little about @viewport. So I added FIXME to solve
@viewport in the future.

[esprehn, 2013-11-14 09:44:01]

I think I was wrong and we should put it into styleResolver() and
styleResolverIfExists() having had more time to look this over, though I would
like to see this patch broken up if possible.

[tasak, 2013-11-14 11:25:45]

I revised the first patch, i.e. modifying Document::styleResolver().

https://codereview.chromium.org/42543007/diff/520001/Source/core/css/RuleSet.h
File Source/core/css/RuleSet.h (right):

https://codereview.chromium.org/42543007/diff/520001/Source/core/css/RuleSet....
Source/core/css/RuleSet.h:113: void addRulesFromSheet(StyleSheetContents*, const
MediaQueryEvaluator&, StyleResolver* = 0, const ContainerNode* = 0, bool
viewportRuleIsProcessed = false);
On 2013/11/13 16:29:22, Dimitri Glazkov wrote:
> Let's avoid bools as params. Enums are teh cool.

Done.

https://codereview.chromium.org/42543007/diff/520001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/42543007/diff/520001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:237: // If we delays
"addFontFaceRule", we probably wait more time until fonts are loaded.
On 2013/11/13 16:29:22, Dimitri Glazkov wrote:
> /delays/delay/s ... /probably/could/s

I removed this, because now we can lazily add @font-face rules.

[esprehn, 2013-11-15 10:36:32]

Bunch of nits, but this is looking really good.

https://codereview.chromium.org/42543007/diff/590001/Source/core/css/RuleSet.h
File Source/core/css/RuleSet.h (right):

https://codereview.chromium.org/42543007/diff/590001/Source/core/css/RuleSet....
Source/core/css/RuleSet.h:126: void addRulesFromSheet(StyleSheetContents*, const
MediaQueryEvaluator&, AddRuleFlags = RuleHasNoSpecialState);
What is special state?

https://codereview.chromium.org/42543007/diff/590001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/42543007/diff/590001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:113: return !scope ||
scope->isDocumentNode();
We should really fix this so scope is never null, it's either a ShadowRoot or a
Document. We can do that in future patches though.

https://codereview.chromium.org/42543007/diff/590001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:189: void
StyleResolver::filterViewportRules(const Vector<RefPtr<StyleRuleBase> >& rules,
bool& needsResolveViewport)
Return a boolean instead.

https://codereview.chromium.org/42543007/diff/590001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:205: void
StyleResolver::filterViewportRulesFromSheet(StyleSheetContents* sheet, bool&
needsResolveViewport)
Ditto, return a boolean.

https://codereview.chromium.org/42543007/diff/590001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:210: if (sheet->importRules().size()
> 0) {
There's no reason to check if the Vector size is > 0, the loop will just be a
noop if it is.

https://codereview.chromium.org/42543007/diff/590001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/42543007/diff/590001/Source/core/dom/Document...
Source/core/dom/Document.cpp:1963: StyleResolver* Document::styleResolver()
We might need to make this inline later, but this is fine for this patch. Lets
micro optimize in the future.

https://codereview.chromium.org/42543007/diff/590001/Source/core/dom/Document...
Source/core/dom/Document.cpp:1968: if
(m_styleResolver->hasPendingAuthorStyleSheets())
else if

[tasak, 2013-11-18 08:30:52]

Thank you for reviewing.

https://codereview.chromium.org/42543007/diff/590001/Source/core/css/RuleSet.h
File Source/core/css/RuleSet.h (right):

https://codereview.chromium.org/42543007/diff/590001/Source/core/css/RuleSet....
Source/core/css/RuleSet.h:126: void addRulesFromSheet(StyleSheetContents*, const
MediaQueryEvaluator&, AddRuleFlags = RuleHasNoSpecialState);
On 2013/11/15 10:36:32, esprehn wrote:
> What is special state?

One is RuleHasDocumentSecurityOrigin.

https://codereview.chromium.org/42543007/diff/590001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/42543007/diff/590001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:113: return !scope ||
scope->isDocumentNode();
On 2013/11/15 10:36:32, esprehn wrote:
> We should really fix this so scope is never null, it's either a ShadowRoot or
a
> Document. We can do that in future patches though.

I added FIXME.

https://codereview.chromium.org/42543007/diff/590001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:189: void
StyleResolver::filterViewportRules(const Vector<RefPtr<StyleRuleBase> >& rules,
bool& needsResolveViewport)
On 2013/11/15 10:36:32, esprehn wrote:
> Return a boolean instead.

Done.

https://codereview.chromium.org/42543007/diff/590001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:205: void
StyleResolver::filterViewportRulesFromSheet(StyleSheetContents* sheet, bool&
needsResolveViewport)
On 2013/11/15 10:36:32, esprehn wrote:
> Ditto, return a boolean.

Done.

https://codereview.chromium.org/42543007/diff/590001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:210: if (sheet->importRules().size()
> 0) {
On 2013/11/15 10:36:32, esprehn wrote:
> There's no reason to check if the Vector size is > 0, the loop will just be a
> noop if it is.

Done.

https://codereview.chromium.org/42543007/diff/590001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/42543007/diff/590001/Source/core/dom/Document...
Source/core/dom/Document.cpp:1963: StyleResolver* Document::styleResolver()
On 2013/11/15 10:36:32, esprehn wrote:
> We might need to make this inline later, but this is fine for this patch. Lets
> micro optimize in the future.

I made this inline.

https://codereview.chromium.org/42543007/diff/590001/Source/core/dom/Document...
Source/core/dom/Document.cpp:1968: if
(m_styleResolver->hasPendingAuthorStyleSheets())
On 2013/11/15 10:36:32, esprehn wrote:
> else if

Done.

[esprehn, 2013-11-18 08:49:11]

lgtm

[commit-bot: I haz the power, 2013-11-18 08:58:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tasak@google.com/42543007/720001

[commit-bot: I haz the power, 2013-11-18 10:43:50]

Change committed as 162187

[ojan, 2013-11-19 17:35:05]

lgtm

https://codereview.chromium.org/42543007/diff/720001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/42543007/diff/720001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:271: for (ListHashSet<CSSStyleSheet*,
16>::iterator it = m_pendingStyleSheets.begin(); it !=
m_pendingStyleSheets.end(); ++it) {
Is it safe for StyleResolver to hold raw pointers to CSSStyleSheets like this?
What happens if you append a stylesheet to the document and then remove it
before we've processed the pending sheets? Won't that use-after-free?