Bug 232841 - [GPU Process] [Filters 19/23] Remove the dependency from FilterEffect to its inputs
Summary: [GPU Process] [Filters 19/23] Remove the dependency from FilterEffect to its ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks: 231253
  Show dependency treegraph
 
Reported: 2021-11-08 13:12 PST by Said Abou-Hallawa
Modified: 2022-01-11 12:31 PST (History)
16 users (show)

See Also:


Attachments
Patch (86.73 KB, patch)
2022-01-10 13:55 PST, Said Abou-Hallawa
darin: review+
Details | Formatted Diff | Diff
Patch (88.84 KB, patch)
2022-01-10 17:45 PST, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (88.82 KB, patch)
2022-01-11 10:57 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2021-11-08 13:12:25 PST
The Filter will pass the input FilterImages to the FilterEffect::apply() and will receive the result FilterImage. There is no actual need for aFilterEffect to access the input FilterEffects.
Comment 1 Radar WebKit Bug Importer 2021-11-15 13:13:25 PST
<rdar://problem/85425930>
Comment 2 Said Abou-Hallawa 2022-01-10 13:55:58 PST
Created attachment 448798 [details]
Patch
Comment 3 Darin Adler 2022-01-10 14:16:32 PST
Comment on attachment 448798 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=448798&action=review

Looks good, assuming tests are all passing.

> Source/WebCore/svg/graphics/filters/SVGFilterBuilder.h:62
> +    std::optional<FilterEffectVector> namedEffects(const Vector<AtomString>&) const;

I’d suggest Span<AtomString> instead as the argument type here.
Comment 4 Said Abou-Hallawa 2022-01-10 17:45:37 PST
Created attachment 448820 [details]
Patch
Comment 5 Said Abou-Hallawa 2022-01-10 17:48:49 PST
Comment on attachment 448798 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=448798&action=review

>> Source/WebCore/svg/graphics/filters/SVGFilterBuilder.h:62
>> +    std::optional<FilterEffectVector> namedEffects(const Vector<AtomString>&) const;
> 
> I’d suggest Span<AtomString> instead as the argument type here.

All FilterEffects, except FEMerge, have fixed number of inputs. FEMerge can have variable number of inputs. So I think Span can't be used in this case.
Comment 6 Darin Adler 2022-01-10 18:10:37 PST
Comment on attachment 448798 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=448798&action=review

>>> Source/WebCore/svg/graphics/filters/SVGFilterBuilder.h:62
>>> +    std::optional<FilterEffectVector> namedEffects(const Vector<AtomString>&) const;
>> 
>> I’d suggest Span<AtomString> instead as the argument type here.
> 
> All FilterEffects, except FEMerge, have fixed number of inputs. FEMerge can have variable number of inputs. So I think Span can't be used in this case.

Not sure why you say that. Span can have a fixed number, but I am suggesting a Span with a variable number.

Span *definitely* can be used. And what it does is allow a caller to call this without constructing a Vector, or call with a Vector.
Comment 7 Said Abou-Hallawa 2022-01-11 10:57:24 PST
Created attachment 448857 [details]
Patch
Comment 8 Said Abou-Hallawa 2022-01-11 11:59:07 PST
Comment on attachment 448798 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=448798&action=review

>>>> Source/WebCore/svg/graphics/filters/SVGFilterBuilder.h:62
>>>> +    std::optional<FilterEffectVector> namedEffects(const Vector<AtomString>&) const;
>>> 
>>> I’d suggest Span<AtomString> instead as the argument type here.
>> 
>> All FilterEffects, except FEMerge, have fixed number of inputs. FEMerge can have variable number of inputs. So I think Span can't be used in this case.
> 
> Not sure why you say that. Span can have a fixed number, but I am suggesting a Span with a variable number.
> 
> Span *definitely* can be used. And what it does is allow a caller to call this without constructing a Vector, or call with a Vector.

Sorry I misunderstood your suggestion. I thought you suggest changing the functions filterEffectInputsNames() also to return Span<AtomString>. Fixed in the latest patch.
Comment 9 EWS 2022-01-11 12:31:27 PST
Committed r287892 (245930@main): <https://commits.webkit.org/245930@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 448857 [details].