Move inline stylesheet cache decision making from StyleSheetContents to StyleEngine

Move inline stylesheet cache decision making from StyleSheetContents to StyleEngine

This separates the decisions about caching StyleSheetContents for <style> elements
from those related to <link> elements. This makes the code clearer as there are
fewer conditions that could cause uncacheability (for example, the StyleSheetContents
for <style> elements could never be for an @import rule).

And since the <style> cache is per-StyleEngine (and thus per-Document), the
<style> case can now safely cache stylesheets that include @media rules.
This speeds up the included performance test from ~90ms on my Z620 to ~50ms,
and improves the biggest block of chromestatus.com load performance by ~20%.

This patch also cleans up StyleSheetContents a bit, collapsing the maybeCacheable()
method into isCacheable() and making appropriate accessors const.

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

[esprehn, 2014-03-19 22:32:14]

lgtm, one of the names is a bit confusing but you can leave it if you want.

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

https://codereview.chromium.org/196133034/diff/1/Source/core/dom/StyleEngine....
Source/core/dom/StyleEngine.cpp:565: static bool isCacheableForInlineStyle(const
StyleSheetContents& contents)
inline style actually means the style attribute, I might do
isCachableHTMLStyleElementSheet()

[adamk, 2014-03-19 22:40:17]

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

https://codereview.chromium.org/196133034/diff/1/Source/core/dom/StyleEngine....
Source/core/dom/StyleEngine.cpp:565: static bool isCacheableForInlineStyle(const
StyleSheetContents& contents)
On 2014/03/19 22:32:14, esprehn wrote:
> inline style actually means the style attribute, I might do
> isCachableHTMLStyleElementSheet()

You're right, when I re-read this name it sounded wrong. Renamed to
isCacheableForStyleElement (I don't want to refer to a StyleSheetContents as a
"sheet").

[esprehn, 2014-03-20 00:52:27]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2014-03-20 00:52:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/adamk@chromium.org/196133034/20001

[commit-bot: I haz the power, 2014-03-20 00:52:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-20 00:52:43]

Failed to apply patch for Source/core/css/StyleSheetContents.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/css/StyleSheetContents.cpp
  Hunk #3 FAILED at 583.
  1 out of 3 hunks FAILED -- saving rejects to file
Source/core/css/StyleSheetContents.cpp.rej

Patch:       Source/core/css/StyleSheetContents.cpp
Index: Source/core/css/StyleSheetContents.cpp
diff --git a/Source/core/css/StyleSheetContents.cpp
b/Source/core/css/StyleSheetContents.cpp
index
98f378dd6abe174e4e4f6569ac238c65a092ba23..ab2038075ef8c5c8300bb439742900ce266dbabd
100644
--- a/Source/core/css/StyleSheetContents.cpp
+++ b/Source/core/css/StyleSheetContents.cpp
@@ -113,8 +113,11 @@ void
StyleSheetContents::setHasSyntacticallyValidCSSHeader(bool isValidCss)
     m_hasSyntacticallyValidCSSHeader = isValidCss;
 }
 
-bool StyleSheetContents::maybeCacheable() const
+bool StyleSheetContents::isCacheable() const
 {
+    // This would require dealing with multiple clients for load callbacks.
+    if (!loadCompleted())
+        return false;
     // FIXME: StyleSheets with media queries can't be cached because their
RuleSet
     // is processed differently based off the media queries, which might
resolve
     // differently depending on the context of the parent CSSStyleSheet (e.g.
@@ -140,14 +143,6 @@ bool StyleSheetContents::maybeCacheable() const
     return true;
 }
 
-bool StyleSheetContents::isCacheable() const
-{
-    // This would require dealing with multiple clients for load callbacks.
-    if (!loadCompleted())
-        return false;
-    return maybeCacheable();
-}
-
 void StyleSheetContents::parserAppendRule(PassRefPtrWillBeRawPtr<StyleRuleBase>
rule)
 {
     ASSERT(!rule->isCharsetRule());
@@ -588,8 +583,6 @@ void StyleSheetContents::clientLoadStarted(CSSStyleSheet*
sheet)
 
 void StyleSheetContents::removeSheetFromCache(Document* document)
 {
-    if (!maybeCacheable())
-        return;
     if (!document || !document->isActive())
         return;
     document->styleEngine()->removeSheet(this);

[adamk, 2014-03-20 00:54:45]

The CQ bit was checked by adamk@chromium.org

[commit-bot: I haz the power, 2014-03-20 00:54:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/adamk@chromium.org/196133034/40001

[commit-bot: I haz the power, 2014-03-20 00:58:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-20 00:58:38]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_rel

[esprehn, 2014-03-20 00:59:28]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2014-03-20 00:59:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/adamk@chromium.org/196133034/40001

[commit-bot: I haz the power, 2014-03-20 01:02:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-20 01:02:46]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_rel

[adamk, 2014-03-20 01:04:40]

The CQ bit was checked by adamk@chromium.org

[commit-bot: I haz the power, 2014-03-20 01:04:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/adamk@chromium.org/196133034/40001

[commit-bot: I haz the power, 2014-03-20 04:03:56]

Change committed as 169610