Summary: | [GPU Process] [Filters 19/23] Remove the dependency from FilterEffect to its inputs | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Said Abou-Hallawa <sabouhallawa> | ||||||||
Component: | Layout and Rendering | Assignee: | Said Abou-Hallawa <sabouhallawa> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bfulgham, changseok, darin, dino, esprehn+autocc, ews-watchlist, fmalita, glenn, gyuyoung.kim, kondapallykalyan, pdr, schenney, sergio, simon.fraser, webkit-bug-importer, zalan | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 231253 | ||||||||||
Attachments: |
|
Description
Said Abou-Hallawa
2021-11-08 13:12:25 PST
Created attachment 448798 [details]
Patch
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. Created attachment 448820 [details]
Patch
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 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. Created attachment 448857 [details]
Patch
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. 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]. |