Created attachment 445118 [details] Patch

Comment on attachment 445118 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=445118&action=review > Source/WebCore/ChangeLog:10 > + his change, FilterImage needs to hold two new members: primitiveSubregion "this change" > Source/WebCore/ChangeLog:13 > + FilterEffect::setIsAlphaImage() is not needed and isAlphaImage() will be "will be a" > Source/WebCore/platform/graphics/filters/FEBlend.cpp:56 > + FilterImageVector inputs { { *inputEffect(0)->filterImage() }, { *inputEffect(1)->filterImage() } }; Are the inner braces needed? The Ref constructor that takes a T& is not explicit, so I think they shouldn't be needed. (Throughout this patch.) Also: maybe this helps subsequent patches in your queue, but wouldn't it be easier to have a function that returns the FilterImageVector by iterating over m_inputEffects and calling `filterImage()` on them all (called `inputEffectImages()` or `inputImages()`)? Is there an advantage to grabbing out input effect #0 and input effect #1 here explicitly? > Source/WebCore/platform/graphics/filters/FEColorMatrix.cpp:96 > +bool FEColorMatrix::isAlphaImage() const Since we're separating the effects from the images, maybe this should be called "producesAlphaImage", something that sounds less like the effect object is an image itself. > Source/WebCore/platform/graphics/filters/FEMerge.cpp:46 > + FilterImageVector inputs; > + > + for (auto& inputEffect : inputEffects()) > + inputs.append({ *inputEffect->filterImage() }); (Something like this, as a member function on effect objects, is what I was thinking.)

Created attachment 445128 [details] Patch

Comment on attachment 445118 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=445118&action=review >> Source/WebCore/platform/graphics/filters/FEBlend.cpp:56 >> + FilterImageVector inputs { { *inputEffect(0)->filterImage() }, { *inputEffect(1)->filterImage() } }; > > Are the inner braces needed? The Ref constructor that takes a T& is not explicit, so I think they shouldn't be needed. (Throughout this patch.) > > Also: maybe this helps subsequent patches in your queue, but wouldn't it be easier to have a function that returns the FilterImageVector by iterating over m_inputEffects and calling `filterImage()` on them all (called `inputEffectImages()` or `inputImages()`)? Is there an advantage to grabbing out input effect #0 and input effect #1 here explicitly? Done. >> Source/WebCore/platform/graphics/filters/FEColorMatrix.cpp:96 >> +bool FEColorMatrix::isAlphaImage() const > > Since we're separating the effects from the images, maybe this should be called "producesAlphaImage", something that sounds less like the effect object is an image itself. Done. But I named resultIsAlphaImage() to be consistent with resultColorSpace().

Committed r286164 (244547@main): <https://commits.webkit.org/244547@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 445128 [details].

<rdar://problem/85748077>