Bug 232841 – [GPU Process] [Filters 19/23] Remove the dependency from FilterEffect to its inputs

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

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.

[Radar WebKit Bug Importer]

<rdar://problem/85425930>

[Said Abou-Hallawa]

Created attachment 448798 [details]
Patch

[Darin Adler]

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.

[Said Abou-Hallawa]

Created attachment 448820 [details]
Patch

[Said Abou-Hallawa]

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.

[Darin Adler]

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.

[Said Abou-Hallawa]

Created attachment 448857 [details]
Patch

[Said Abou-Hallawa]

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.

[EWS]

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].