Commit e2a55742 authored by Darren Shen's avatar Darren Shen Committed by Commit Bot

Avoid reparsing stylesheets that contain media queries.

When we fill in the RuleSet for a StyleSheetContents object, a rule
in a media query would only be added if the media query evaluate to true.
Hence, a StyleSheetContents may have different RuleSets depending on
the current media. This is why we do not cache StyleSheetContents that
contain media queries, since the RuleSet needs to be different.

This patch enables (hidden behind a flag) caching of StyleSheetContents
with media queries, but when they are retrieved from that cache, they are cloned.

Design doc:
https://docs.google.com/document/d/1IpyqChzXDJczfsxnbv0kh-gAx6HBGGfIpSu4P6nKI1s/edit?usp=sharing

Bug: 472923
Change-Id: I61778a9e50fc11881a3cb5f5ddfe358876c43fc6
Reviewed-on: https://chromium-review.googlesource.com/704474Reviewed-by: default avatarKentaro Hara <haraken@chromium.org>
Reviewed-by: default avatarRune Lillesveen <rune@opera.com>
Commit-Queue: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509291}
parent 2ba8ec09
...@@ -125,12 +125,8 @@ bool StyleSheetContents::IsCacheableForResource() const { ...@@ -125,12 +125,8 @@ bool StyleSheetContents::IsCacheableForResource() const {
// This would require dealing with multiple clients for load callbacks. // This would require dealing with multiple clients for load callbacks.
if (!LoadCompleted()) if (!LoadCompleted())
return false; return false;
// FIXME: StyleSheets with media queries can't be cached because their RuleSet if (has_media_queries_ &&
// is processed differently based off the media queries, which might resolve !RuntimeEnabledFeatures::CacheStyleSheetWithMediaQueriesEnabled())
// differently depending on the context of the parent CSSStyleSheet (e.g.
// if they are in differently sized iframes). Once RuleSets are media query
// agnostic, we can restore sharing of StyleSheetContents with medea queries.
if (has_media_queries_)
return false; return false;
// FIXME: Support copying import rules. // FIXME: Support copying import rules.
if (!import_rules_.IsEmpty()) if (!import_rules_.IsEmpty())
......
...@@ -98,22 +98,19 @@ void LinkStyle::SetCSSStyleSheet( ...@@ -98,22 +98,19 @@ void LinkStyle::SetCSSStyleSheet(
CSSParserContext* parser_context = CSSParserContext::Create( CSSParserContext* parser_context = CSSParserContext::Create(
GetDocument(), base_url, referrer_policy, charset); GetDocument(), base_url, referrer_policy, charset);
if (StyleSheetContents* restored_sheet = if (StyleSheetContents* parsed_sheet =
const_cast<CSSStyleSheetResource*>(cached_style_sheet) const_cast<CSSStyleSheetResource*>(cached_style_sheet)
->RestoreParsedStyleSheet(parser_context)) { ->CreateParsedStyleSheetFromCache(parser_context)) {
DCHECK(restored_sheet->IsCacheableForResource());
DCHECK(!restored_sheet->IsLoading());
if (sheet_) if (sheet_)
ClearSheet(); ClearSheet();
sheet_ = CSSStyleSheet::Create(restored_sheet, *owner_); sheet_ = CSSStyleSheet::Create(parsed_sheet, *owner_);
sheet_->SetMediaQueries(MediaQuerySet::Create(owner_->Media())); sheet_->SetMediaQueries(MediaQuerySet::Create(owner_->Media()));
if (owner_->IsInDocumentTree()) if (owner_->IsInDocumentTree())
SetSheetTitle(owner_->title()); SetSheetTitle(owner_->title());
SetCrossOriginStylesheetStatus(sheet_.Get()); SetCrossOriginStylesheetStatus(sheet_.Get());
loading_ = false; loading_ = false;
restored_sheet->CheckLoaded(); parsed_sheet->CheckLoaded();
return; return;
} }
......
...@@ -36,6 +36,7 @@ ...@@ -36,6 +36,7 @@
#include "platform/loader/fetch/ResourceFetcher.h" #include "platform/loader/fetch/ResourceFetcher.h"
#include "platform/loader/fetch/ResourceLoaderOptions.h" #include "platform/loader/fetch/ResourceLoaderOptions.h"
#include "platform/loader/fetch/TextResourceDecoderOptions.h" #include "platform/loader/fetch/TextResourceDecoderOptions.h"
#include "platform/runtime_enabled_features.h"
#include "platform/weborigin/SecurityPolicy.h" #include "platform/weborigin/SecurityPolicy.h"
#include "platform/wtf/CurrentTime.h" #include "platform/wtf/CurrentTime.h"
#include "platform/wtf/text/TextEncoding.h" #include "platform/wtf/text/TextEncoding.h"
...@@ -213,7 +214,7 @@ bool CSSStyleSheetResource::CanUseSheet(MIMETypeCheck mime_type_check) const { ...@@ -213,7 +214,7 @@ bool CSSStyleSheetResource::CanUseSheet(MIMETypeCheck mime_type_check) const {
"application/x-unknown-content-type"); "application/x-unknown-content-type");
} }
StyleSheetContents* CSSStyleSheetResource::RestoreParsedStyleSheet( StyleSheetContents* CSSStyleSheetResource::CreateParsedStyleSheetFromCache(
const CSSParserContext* context) { const CSSParserContext* context) {
if (!parsed_style_sheet_cache_) if (!parsed_style_sheet_cache_)
return nullptr; return nullptr;
...@@ -230,6 +231,15 @@ StyleSheetContents* CSSStyleSheetResource::RestoreParsedStyleSheet( ...@@ -230,6 +231,15 @@ StyleSheetContents* CSSStyleSheetResource::RestoreParsedStyleSheet(
if (*parsed_style_sheet_cache_->ParserContext() != *context) if (*parsed_style_sheet_cache_->ParserContext() != *context)
return nullptr; return nullptr;
DCHECK(!parsed_style_sheet_cache_->IsLoading());
// If the stylesheet has a media query, we need to clone the cached sheet
// due to potential differences in the rule set.
if (RuntimeEnabledFeatures::CacheStyleSheetWithMediaQueriesEnabled() &&
parsed_style_sheet_cache_->HasMediaQueries()) {
return parsed_style_sheet_cache_->Copy();
}
return parsed_style_sheet_cache_; return parsed_style_sheet_cache_;
} }
......
...@@ -56,7 +56,7 @@ class CORE_EXPORT CSSStyleSheetResource final : public StyleSheetResource { ...@@ -56,7 +56,7 @@ class CORE_EXPORT CSSStyleSheetResource final : public StyleSheetResource {
void DidAddClient(ResourceClient*) override; void DidAddClient(ResourceClient*) override;
StyleSheetContents* RestoreParsedStyleSheet(const CSSParserContext*); StyleSheetContents* CreateParsedStyleSheetFromCache(const CSSParserContext*);
void SaveParsedStyleSheet(StyleSheetContents*); void SaveParsedStyleSheet(StyleSheetContents*);
void AppendData(const char* data, size_t length) override; void AppendData(const char* data, size_t length) override;
......
...@@ -56,6 +56,19 @@ class CSSStyleSheetResourceTest : public ::testing::Test { ...@@ -56,6 +56,19 @@ class CSSStyleSheetResourceTest : public ::testing::Test {
ReplaceMemoryCacheForTesting(original_memory_cache_.Release()); ReplaceMemoryCacheForTesting(original_memory_cache_.Release());
} }
CSSStyleSheetResource* CreateAndSaveTestStyleSheetResource() {
const char kUrl[] = "https://localhost/style.css";
KURL css_url(NullURL(), kUrl);
CSSStyleSheetResource* css_resource =
CSSStyleSheetResource::CreateForTest(css_url, UTF8Encoding());
css_resource->ResponseReceived(
ResourceResponse(css_url, "style/css", 0, g_null_atom), nullptr);
css_resource->FinishForTest();
GetMemoryCache()->Add(css_resource);
return css_resource;
}
Document& GetDocument() { return page_->GetDocument(); } Document& GetDocument() { return page_->GetDocument(); }
Persistent<MemoryCache> original_memory_cache_; Persistent<MemoryCache> original_memory_cache_;
...@@ -95,7 +108,71 @@ TEST_F(CSSStyleSheetResourceTest, DuplicateResourceNotCached) { ...@@ -95,7 +108,71 @@ TEST_F(CSSStyleSheetResourceTest, DuplicateResourceNotCached) {
EXPECT_TRUE(GetMemoryCache()->Contains(image_resource)); EXPECT_TRUE(GetMemoryCache()->Contains(image_resource));
EXPECT_FALSE(GetMemoryCache()->Contains(css_resource)); EXPECT_FALSE(GetMemoryCache()->Contains(css_resource));
EXPECT_FALSE(contents->IsReferencedFromResource()); EXPECT_FALSE(contents->IsReferencedFromResource());
EXPECT_FALSE(css_resource->RestoreParsedStyleSheet(parser_context)); EXPECT_FALSE(css_resource->CreateParsedStyleSheetFromCache(parser_context));
}
TEST_F(CSSStyleSheetResourceTest, CreateFromCacheRestoresOriginalSheet) {
CSSStyleSheetResource* css_resource = CreateAndSaveTestStyleSheetResource();
CSSParserContext* parser_context =
CSSParserContext::Create(kHTMLStandardMode);
StyleSheetContents* contents = StyleSheetContents::Create(parser_context);
CSSStyleSheet* sheet = CSSStyleSheet::Create(contents, GetDocument());
ASSERT_TRUE(sheet);
contents->ParseString("div { color: red; }");
contents->NotifyLoadedSheet(css_resource);
contents->CheckLoaded();
EXPECT_TRUE(contents->IsCacheableForResource());
css_resource->SaveParsedStyleSheet(contents);
EXPECT_TRUE(GetMemoryCache()->Contains(css_resource));
EXPECT_TRUE(contents->IsReferencedFromResource());
StyleSheetContents* parsed_stylesheet =
css_resource->CreateParsedStyleSheetFromCache(parser_context);
ASSERT_EQ(contents, parsed_stylesheet);
}
TEST_F(CSSStyleSheetResourceTest,
CreateFromCacheWithMediaQueriesCopiesOriginalSheet) {
CSSStyleSheetResource* css_resource = CreateAndSaveTestStyleSheetResource();
CSSParserContext* parser_context =
CSSParserContext::Create(kHTMLStandardMode);
StyleSheetContents* contents = StyleSheetContents::Create(parser_context);
CSSStyleSheet* sheet = CSSStyleSheet::Create(contents, GetDocument());
ASSERT_TRUE(sheet);
contents->ParseString("@media { div { color: red; } }");
contents->NotifyLoadedSheet(css_resource);
contents->CheckLoaded();
EXPECT_TRUE(contents->IsCacheableForResource());
contents->EnsureRuleSet(MediaQueryEvaluator(), kRuleHasNoSpecialState);
EXPECT_TRUE(contents->HasRuleSet());
css_resource->SaveParsedStyleSheet(contents);
EXPECT_TRUE(GetMemoryCache()->Contains(css_resource));
EXPECT_TRUE(contents->IsReferencedFromResource());
StyleSheetContents* parsed_stylesheet =
css_resource->CreateParsedStyleSheetFromCache(parser_context);
ASSERT_TRUE(parsed_stylesheet);
sheet->ClearOwnerNode();
sheet = CSSStyleSheet::Create(parsed_stylesheet, GetDocument());
ASSERT_TRUE(sheet);
EXPECT_TRUE(contents->HasSingleOwnerDocument());
EXPECT_EQ(0U, contents->ClientSize());
EXPECT_TRUE(contents->IsReferencedFromResource());
EXPECT_TRUE(contents->HasRuleSet());
EXPECT_TRUE(parsed_stylesheet->HasSingleOwnerDocument());
EXPECT_TRUE(parsed_stylesheet->HasOneClient());
EXPECT_FALSE(parsed_stylesheet->IsReferencedFromResource());
EXPECT_FALSE(parsed_stylesheet->HasRuleSet());
} }
} // namespace } // namespace
......
...@@ -142,6 +142,10 @@ ...@@ -142,6 +142,10 @@
origin_trial_feature_name: "BudgetQuery", origin_trial_feature_name: "BudgetQuery",
status: "experimental", status: "experimental",
}, },
{
name: "CacheStyleSheetWithMediaQueries",
status: "experimental",
},
{ {
name: "Canvas2dFixedRenderingMode", name: "Canvas2dFixedRenderingMode",
status: "test", status: "test",
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment