-
Notifications
You must be signed in to change notification settings - Fork 6.6k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Add document-wide cache for StyleFetchedImage
StyleFetchedImage is cached on the CSSImageValue object and re-used each time a declaration applies to an element's ComputedStyle. The problem arises when the same url is used in different declarations, or even worse, when the CSSImageValue is regenerated every time when a custom property url is substituted into another property. In those cases we create a new StyleFetchedImage and re-fetch the resource. Use the same StyleFetchedImage for the cached image url for all CSSImageValue objects for the same Document. The cache is a map from url into a weak StyleFetchedImage such that it will be garbage collected once there are no more CSSImageValues referencing the StyleFetchedImage. Bug: 961600 Change-Id: I5978c9b6befc9b0aa2f01e7e764866e2d80a8011 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3565368 Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org> Commit-Queue: Rune Lillesveen <futhark@chromium.org> Cr-Commit-Position: refs/heads/main@{#988488}
- Loading branch information
Rune Lillesveen
authored and
Chromium LUCI CQ
committed
Apr 4, 2022
1 parent
6e6b034
commit 288ae84
Showing
8 changed files
with
213 additions
and
6 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| // Copyright 2022 The Chromium Authors. All rights reserved. | ||
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| #include "third_party/blink/renderer/core/css/style_image_cache.h" | ||
|
|
||
| #include "third_party/blink/renderer/core/dom/document.h" | ||
| #include "third_party/blink/renderer/core/loader/resource/image_resource_content.h" | ||
| #include "third_party/blink/renderer/core/style/style_fetched_image.h" | ||
| #include "third_party/blink/renderer/platform/loader/fetch/fetch_parameters.h" | ||
|
|
||
| namespace blink { | ||
|
|
||
| StyleFetchedImage* StyleImageCache::CacheStyleImage(Document& document, | ||
| FetchParameters& params, | ||
| OriginClean origin_clean, | ||
| bool is_ad_related) { | ||
| auto result = fetched_image_map_.insert(params.Url(), nullptr); | ||
| if (result.is_new_entry || !result.stored_value->value) { | ||
| result.stored_value->value = MakeGarbageCollected<StyleFetchedImage>( | ||
| ImageResourceContent::Fetch(params, document.Fetcher()), document, | ||
| params.GetImageRequestBehavior() == FetchParameters::kDeferImageLoad, | ||
| origin_clean == OriginClean::kTrue, is_ad_related, params.Url()); | ||
| } | ||
| return result.stored_value->value; | ||
| } | ||
|
|
||
| void StyleImageCache::Trace(Visitor* visitor) const { | ||
| visitor->Trace(fetched_image_map_); | ||
| } | ||
|
|
||
| } // namespace blink |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| // Copyright 2022 The Chromium Authors. All rights reserved. | ||
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| #ifndef THIRD_PARTY_BLINK_RENDERER_CORE_CSS_STYLE_IMAGE_CACHE_H_ | ||
| #define THIRD_PARTY_BLINK_RENDERER_CORE_CSS_STYLE_IMAGE_CACHE_H_ | ||
|
|
||
| #include "third_party/blink/renderer/core/core_export.h" | ||
| #include "third_party/blink/renderer/core/css/css_origin_clean.h" | ||
| #include "third_party/blink/renderer/platform/heap/collection_support/heap_hash_map.h" | ||
| #include "third_party/blink/renderer/platform/weborigin/kurl.h" | ||
| #include "third_party/blink/renderer/platform/weborigin/kurl_hash.h" | ||
| #include "third_party/blink/renderer/platform/wtf/allocator/allocator.h" | ||
|
|
||
| namespace blink { | ||
|
|
||
| class Document; | ||
| class FetchParameters; | ||
| class StyleFetchedImage; | ||
|
|
||
| // A per-StyleEngine cache for StyleImages. A CSSImageValue points to a | ||
| // StyleImage, but different CSSImageValue objects with the same URL would not | ||
| // have shared the same StyleImage without this cache. | ||
| class CORE_EXPORT StyleImageCache { | ||
| DISALLOW_NEW(); | ||
|
|
||
| public: | ||
| StyleImageCache() = default; | ||
|
|
||
| // Look up an existing StyleFetchedImage in the cache, or create a new one, | ||
| // add it to the cache, and start the fetch. | ||
| StyleFetchedImage* CacheStyleImage(Document&, | ||
| FetchParameters&, | ||
| OriginClean, | ||
| bool is_ad_related); | ||
|
|
||
| void Trace(Visitor*) const; | ||
|
|
||
| private: | ||
| // Map from URL to style image. A weak reference makes sure the entry is | ||
| // removed when no style declarations nor computed styles have a reference to | ||
| // the image. | ||
| HeapHashMap<KURL, WeakMember<StyleFetchedImage>> fetched_image_map_; | ||
|
|
||
| friend class StyleImageCacheTest; | ||
| }; | ||
|
|
||
| } // namespace blink | ||
|
|
||
| #endif // THIRD_PARTY_BLINK_RENDERER_CORE_CSS_STYLE_IMAGE_CACHE_H_ |
109 changes: 109 additions & 0 deletions
109
third_party/blink/renderer/core/css/style_image_cache_test.cc
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,109 @@ | ||
| // Copyright 2022 The Chromium Authors. All rights reserved. | ||
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| #include "third_party/blink/renderer/core/css/style_image_cache.h" | ||
|
|
||
| #include "third_party/blink/renderer/core/css/style_engine.h" | ||
| #include "third_party/blink/renderer/core/dom/document.h" | ||
| #include "third_party/blink/renderer/core/dom/element.h" | ||
| #include "third_party/blink/renderer/core/dom/node_computed_style.h" | ||
| #include "third_party/blink/renderer/core/style/style_fetched_image.h" | ||
| #include "third_party/blink/renderer/core/testing/page_test_base.h" | ||
| #include "third_party/blink/renderer/platform/heap/thread_state.h" | ||
|
|
||
| namespace blink { | ||
|
|
||
| class StyleImageCacheTest : public PageTestBase { | ||
| protected: | ||
| void SetUp() override { | ||
| PageTestBase::SetUp(); | ||
| GetDocument().SetBaseURLOverride(KURL("http://test.com")); | ||
| } | ||
| const HeapHashMap<KURL, WeakMember<StyleFetchedImage>>& FetchedImageMap() { | ||
| return GetDocument().GetStyleEngine().style_image_cache_.fetched_image_map_; | ||
| } | ||
| }; | ||
|
|
||
| TEST_F(StyleImageCacheTest, DuplicateBackgroundImageURLs) { | ||
| SetBodyInnerHTML(R"HTML( | ||
| <style> | ||
| .rule1 { background-image: url(url.png) } | ||
| .rule2 { background-image: url(url.png) } | ||
| </style> | ||
| <div id="target"></div> | ||
| )HTML"); | ||
|
|
||
| Element* target = GetDocument().getElementById("target"); | ||
| ASSERT_TRUE(target); | ||
| ASSERT_FALSE(target->ComputedStyleRef().BackgroundLayers().GetImage()); | ||
|
|
||
| target->setAttribute(blink::html_names::kClassAttr, "rule1"); | ||
| UpdateAllLifecyclePhasesForTest(); | ||
|
|
||
| StyleImage* rule1_image = | ||
| target->ComputedStyleRef().BackgroundLayers().GetImage(); | ||
| EXPECT_TRUE(rule1_image); | ||
|
|
||
| target->setAttribute(blink::html_names::kClassAttr, "rule2"); | ||
| UpdateAllLifecyclePhasesForTest(); | ||
|
|
||
| StyleImage* rule2_image = | ||
| target->ComputedStyleRef().BackgroundLayers().GetImage(); | ||
| EXPECT_EQ(rule1_image, rule2_image); | ||
| } | ||
|
|
||
| TEST_F(StyleImageCacheTest, CustomPropertyURL) { | ||
| SetBodyInnerHTML(R"HTML( | ||
| <style> | ||
| :root { --bg: url(url.png) } | ||
| #target { background-image: var(--bg) } | ||
| .green { background-color: green } | ||
| </style> | ||
| <div id="target"></div> | ||
| )HTML"); | ||
|
|
||
| Element* target = GetDocument().getElementById("target"); | ||
|
|
||
| StyleImage* initial_image = | ||
| target->ComputedStyleRef().BackgroundLayers().GetImage(); | ||
| EXPECT_TRUE(initial_image); | ||
|
|
||
| target->setAttribute(blink::html_names::kClassAttr, "green"); | ||
| UpdateAllLifecyclePhasesForTest(); | ||
|
|
||
| StyleImage* image_after_recalc = | ||
| target->ComputedStyleRef().BackgroundLayers().GetImage(); | ||
| EXPECT_EQ(initial_image, image_after_recalc); | ||
| } | ||
|
|
||
| TEST_F(StyleImageCacheTest, WeakReferenceGC) { | ||
| SetBodyInnerHTML(R"HTML( | ||
| <style id="sheet"> | ||
| #target1 { background-image: url(url.png) } | ||
| #target2 { background-image: url(url2.png) } | ||
| </style> | ||
| <div id="target1"></div> | ||
| <div id="target2"></div> | ||
| )HTML"); | ||
| UpdateAllLifecyclePhasesForTest(); | ||
|
|
||
| EXPECT_TRUE(FetchedImageMap().Contains(KURL("http://test.com/url.png"))); | ||
| EXPECT_TRUE(FetchedImageMap().Contains(KURL("http://test.com/url2.png"))); | ||
| EXPECT_EQ(FetchedImageMap().size(), 2u); | ||
|
|
||
| Element* sheet = GetDocument().getElementById("sheet"); | ||
| ASSERT_TRUE(sheet); | ||
| sheet->remove(); | ||
| UpdateAllLifecyclePhasesForTest(); | ||
| ThreadState::Current()->CollectAllGarbageForTesting(); | ||
|
|
||
| // After the sheet has been removed, the lifecycle update and garbage | ||
| // collection have been run, the weak references in the cache should have been | ||
| // collected. | ||
| EXPECT_FALSE(FetchedImageMap().Contains(KURL("http://test.com/url.png"))); | ||
| EXPECT_FALSE(FetchedImageMap().Contains(KURL("http://test.com/url2.png"))); | ||
| EXPECT_EQ(FetchedImageMap().size(), 0u); | ||
| } | ||
|
|
||
| } // namespace blink |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters