[SVG] Clearing `SVGResourceDocumentCache` during page reload [chromium/src : main]

[Divyansh Mangal (Gerrit)]

Attention needed from Divyansh Mangal

Message from Divyansh Mangal

Set Ready For Review

Open in Gerrit

Related details

Attention is currently required from:

• Divyansh Mangal

Submit Requirements:

• Code-Coverage
• Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: Ia73da95610f2d0f1fbf3c2d604c62779be966593

Gerrit-Change-Number: 6523721

Gerrit-PatchSet: 7

Gerrit-Owner: Divyansh Mangal <dma...@microsoft.com>

Gerrit-Reviewer: Divyansh Mangal <dma...@microsoft.com>

Gerrit-CC: AyeAye <android-bu...@system.gserviceaccount.com>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: Dirk Schulze <dsch...@chromium.org>

Gerrit-CC: Fredrik Söderquist <f...@opera.com>

Gerrit-CC: Nate Chapin <jap...@chromium.org>

Gerrit-CC: Stephen Chenney <sche...@chromium.org>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Divyansh Mangal <dma...@microsoft.com>

Gerrit-Comment-Date: Tue, 20 May 2025 15:46:05 +0000

Gerrit-HasComments: No

Gerrit-Has-Labels: No

[Divyansh Mangal (Gerrit)]

Gerrit-Comment-Date: Tue, 20 May 2025 15:50:42 +0000

Gerrit-HasComments: No

Gerrit-Has-Labels: No

[Vinay Singh (Gerrit)]

Attention needed from Divyansh Mangal and Virali Purbey

Vinay Singh added 1 comment

Patchset-level comments

File-level comment, Patchset 9 (Latest):

Vinay Singh . resolved

Have you checked if this works if the page is loaded as an `iframe` and that `iframe` is refreshed?

Open in Gerrit

Related details

Attention is currently required from:

• Divyansh Mangal
• Virali Purbey

Submit Requirements:

• Code-Coverage
• Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: Ia73da95610f2d0f1fbf3c2d604c62779be966593

Gerrit-Change-Number: 6523721

Gerrit-PatchSet: 9

Gerrit-Owner: Divyansh Mangal <dma...@microsoft.com>

Gerrit-Reviewer: Divyansh Mangal <dma...@microsoft.com>

Gerrit-Reviewer: Vinay Singh <vinay...@microsoft.com>

Gerrit-Reviewer: Virali Purbey <virali...@microsoft.com>

Gerrit-CC: AyeAye <android-bu...@system.gserviceaccount.com>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: Dileep Maurya <dileep...@microsoft.com>

Gerrit-CC: Dirk Schulze <dsch...@chromium.org>

Gerrit-CC: Fredrik Söderquist <f...@opera.com>

Gerrit-CC: Gaurav Kumar <gaur...@microsoft.com>

Gerrit-CC: Nate Chapin <jap...@chromium.org>

Gerrit-CC: Ragvesh Sharma <rags...@microsoft.com>

Gerrit-CC: Sejal Anand <sejal...@microsoft.com>

Gerrit-CC: Stephen Chenney <sche...@chromium.org>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Divyansh Mangal <dma...@microsoft.com>

Gerrit-Attention: Virali Purbey <virali...@microsoft.com>

Gerrit-Comment-Date: Tue, 20 May 2025 16:15:43 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Fredrik Söderquist (Gerrit)]

Attention needed from Divyansh Mangal and Virali Purbey

Fredrik Söderquist added 5 comments

Commit Message

Line 26, Patchset 9 (Latest):With this change, whenever the external resources are updated,

Fredrik Söderquist . unresolved

...and if they are not updated?

File third_party/blink/renderer/core/frame/local_frame.cc

Line 1044, Patchset 9 (Latest): if (GetPage()) {
GetPage()->ClearSVGDocumentCacheIfAvailable();
}

Fredrik Söderquist . unresolved

This ought to be scoped somehow, no?

File third_party/blink/renderer/core/loader/document_loader.cc

Line 2221, Patchset 9 (Latest): if (frame_->GetPage() && frame_->IsLocalRoot()) {
frame_->GetPage()->ClearSVGDocumentCacheIfAvailable();
}

Fredrik Söderquist . unresolved

What is this intended to achieve? This will just clear the cache on all committed navigations in local roots.

File third_party/blink/renderer/core/page/page.cc

Line 542, Patchset 9 (Latest):void Page::ClearSVGDocumentCacheIfAvailable() {

Fredrik Söderquist . unresolved

I'd suggest putting this next to (after) `Page::GetSVGResourceDocumentCache`. (Ditto for the declaration.)

File third_party/blink/renderer/core/svg/svg_resource_document_cache.cc

Line 76, Patchset 9 (Latest): WillBeDestroyed();
entries_.clear();

Fredrik Söderquist . unresolved

If anything ends up holding a reference to something that was in the cache (all resources will be by necessity), whatever they point to will be dead and unusable (thus likely breaking rendering).

Open in Gerrit

Related details

Attention is currently required from:

• Divyansh Mangal
• Virali Purbey

Submit Requirements:

• Code-Coverage
• Code-Review

• No-Unresolved-Comments

Gerrit-Comment-Date: Wed, 21 May 2025 08:57:51 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Divyansh Mangal (Gerrit)]

Divyansh Mangal abandoned this change

Related details

Attention set is empty

Submit Requirements:

• Code-Coverage
• Code-Review
• No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: abandon