Avoid no-op filter invalidations with multiple filters [chromium/src : main]

1 view

Skip to first unread message

Philip Rogers (Gerrit)

unread,

May 24, 2026, 5:34:12 PM (2 days ago) May 24

to Fredrik Söderquist, Chromium LUCI CQ, Menard, Alexis, chromium...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org

Attention needed from Fredrik Söderquist

Philip Rogers voted Commit-Queue+1

Commit-Queue

Open in Gerrit

Related details

Attention is currently required from:

Fredrik Söderquist

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Fredrik Söderquist (Gerrit)

unread,

May 25, 2026, 5:03:23 AM (22 hours ago) May 25

to Philip Rogers, Chromium LUCI CQ, Menard, Alexis, chromium...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org

Attention needed from Philip Rogers

Fredrik Söderquist voted and added 2 comments

Votes added by Fredrik Söderquist

Code-Review

2 comments

Patchset-level comments

File-level comment, Patchset 3 (Latest):

Fredrik Söderquist . resolved

LGTM w/ suggestion

File third_party/blink/renderer/core/css/resolver/filter_operation_resolver.cc

Line 192, Patchset 3 (Latest):

      // To avoid unnecessary filter invalidations, preserve the filter pointer
      // from the old style if the URL and resource match.
      const AtomicString& url = url_value->ValueForSerialization();
      SVGResource* resource = state.GetSVGResource(property_id, *url_value);
      Filter* filter = nullptr;
      if (state.OldStyle() && state.OldStyle()->HasFilter()) {
        const auto& old_ops = state.OldStyle()->Filter().Operations();
        size_t index = operations.size();
        if (index < old_ops.size()) {
          if (auto* old_ref =
                  DynamicTo<ReferenceFilterOperation>(old_ops[index].Get())) {
            if (old_ref->Url() == url && old_ref->Resource() == resource) {
              filter = old_ref->GetFilter();
            }
          }
        }
      }
      auto* new_op =
          MakeGarbageCollected<ReferenceFilterOperation>(url, resource);
      new_op->SetFilter(filter);
      operations.Operations().push_back(new_op);

Fredrik Söderquist . unresolved

Could you perhaps restructure this a bit given that we always want to create a new `ReferenceFilterOperation` (although technically I guess we don't need to...). Something like:
```
Filter* GetFilterFromOldStyle(const ComputedStyle* old_style, const ReferenceFilterOperation* new_ref, size_t index, ...) {
  ...
}
...
SVGResource* resource = state.GetSVGResource(property_id, *url_value);
auto* new_op =
    MakeGarbageCollected<ReferenceFilterOperation>(url_value->ValueForSerialization(), resource);
new_op->SetFilter(GetFilterFromOldStyle(state.OldStyle(), ...);
operations.Operations().push_back(new_op);
```
WDYT?

Open in Gerrit

Related details

Attention is currently required from:

Philip Rogers

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review
No-Unresolved-Comments
Review-Enforcement

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Philip Rogers (Gerrit)

unread,

May 25, 2026, 8:04:18 PM (7 hours ago) May 25

to Fredrik Söderquist, Chromium LUCI CQ, Menard, Alexis, chromium...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org

Philip Rogers voted and added 1 comment

Votes added by Philip Rogers

Auto-Submit	+1
Commit-Queue	+1

1 comment

File third_party/blink/renderer/core/css/resolver/filter_operation_resolver.cc

Line 192, Patchset 3: // To avoid unnecessary filter invalidations, preserve the filter pointer


      // from the old style if the URL and resource match.
      const AtomicString& url = url_value->ValueForSerialization();
      SVGResource* resource = state.GetSVGResource(property_id, *url_value);
      Filter* filter = nullptr;
      if (state.OldStyle() && state.OldStyle()->HasFilter()) {
        const auto& old_ops = state.OldStyle()->Filter().Operations();
        size_t index = operations.size();
        if (index < old_ops.size()) {
          if (auto* old_ref =
                  DynamicTo<ReferenceFilterOperation>(old_ops[index].Get())) {
            if (old_ref->Url() == url && old_ref->Resource() == resource) {
              filter = old_ref->GetFilter();
            }
          }
        }
      }
      auto* new_op =
          MakeGarbageCollected<ReferenceFilterOperation>(url, resource);
      new_op->SetFilter(filter);
      operations.Operations().push_back(new_op);

Fredrik Söderquist . resolved

Could you perhaps restructure this a bit given that we always want to create a new `ReferenceFilterOperation` (although technically I guess we don't need to...). Something like:
```
Filter* GetFilterFromOldStyle(const ComputedStyle* old_style, const ReferenceFilterOperation* new_ref, size_t index, ...) {
  ...
}
...
SVGResource* resource = state.GetSVGResource(property_id, *url_value);
auto* new_op =
    MakeGarbageCollected<ReferenceFilterOperation>(url_value->ValueForSerialization(), resource);
new_op->SetFilter(GetFilterFromOldStyle(state.OldStyle(), ...);
operations.Operations().push_back(new_op);
```
WDYT?

Philip Rogers

Good idea; done.

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

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

Gerrit

satisfied_requirement

open

diffy

Reply all

Reply to author

Forward

0 new messages