Provide GraphicsContext::fillRect(const FloatRect&, Gradient&, const AffineTransform& gradientSpaceTransform) variant#22352
Merged
Conversation
nikolaszimmermann
requested review from
cdumez,
magomez and
zdobersek
as code owners
Collaborator
|
EWS run on previous version of this PR (hash 88a79e2) Details
|
shallawa
reviewed
Jan 3, 2024
Contributor
There was a problem hiding this comment.
I think you can just pass m_gradient and the reference operator will implicitly do the conversion.
Contributor
There was a problem hiding this comment.
Maybe you can have only this constructor and you make the RecorderImpl pass
append(FillRectWithGradientAndSpaceTransform(rect, Ref { gradient }, gradientSpaceTransform));
Contributor
Author
There was a problem hiding this comment.
Makes sense to check. I followed the existing pattern where most classes offer two constructors, e.g.
DrawDisplayListItems::DrawDisplayListItems(const Vector<Item>& items, const FloatPoint& destination)
: m_items(items)
, m_destination(destination)
{
}
DrawDisplayListItems::DrawDisplayListItems(Vector<Item>&& items, const FloatPoint& destination)
: m_items(WTFMove(items))
, m_destination(destination)
{
}
However, we can try to unify them, and get rid of the FIXME in the DisplayListItems header as well for both FillRectWithGradient and the new FillRectWithGradientAndSpaceTransform.
Contributor
Author
There was a problem hiding this comment.
One possibility is to keep only the constructor taking the rvalue references, e.g.:
diff --git a/Source/WebCore/platform/graphics/displaylists/DisplayListItems.h b/Source/WebCore/platform/graphics/displaylists/DisplayListItems.h
index 0e4218629b6c..5befb335467b 100644
--- a/Source/WebCore/platform/graphics/displaylists/DisplayListItems.h
+++ b/Source/WebCore/platform/graphics/displaylists/DisplayListItems.h
@@ -936,8 +936,7 @@ class FillRectWithGradient {
public:
static constexpr char name[] = "fill-rect-with-gradient";
- WEBCORE_EXPORT FillRectWithGradient(const FloatRect&, Gradient&);
- WEBCORE_EXPORT FillRectWithGradient(FloatRect&&, Ref<Gradient>&&);
+ WEBCORE_EXPORT FillRectWithGradient(const FloatRect&&, Ref<Gradient>&&);
const FloatRect& rect() const { return m_rect; }
const Ref<Gradient>& gradient() const { return m_gradient; }
Note the rather unfamiiar "const Foo&&" usage, that is required to pass on the "const FloatRect&" from the recorder as-is:
diff --git a/Source/WebCore/platform/graphics/displaylists/DisplayListRecorderImpl.cpp b/Source/WebCore/platform/graphics/displaylists/DisplayListRecorderImpl.cpp
index 69f9b7705378..061981b04ffd 100644
--- a/Source/WebCore/platform/graphics/displaylists/DisplayListRecorderImpl.cpp
+++ b/Source/WebCore/platform/graphics/displaylists/DisplayListRecorderImpl.cpp
@@ -274,12 +274,12 @@ void RecorderImpl::recordFillRectWithColor(const FloatRect& rect, const Color& c
void RecorderImpl::recordFillRectWithGradient(const FloatRect& rect, Gradient& gradient)
{
- append(FillRectWithGradient(rect, gradient));
+ append(FillRectWithGradient(WTFMove(rect), Ref { gradient }));
}
I will try this further and see if I hit any issue.
Contributor
Author
There was a problem hiding this comment.
DisplayListTests (API tests) require a change, that makes the code slightly harder to read:
diff --git a/Tools/TestWebKitAPI/Tests/WebCore/DisplayListTests.cpp b/Tools/TestWebKitAPI/Tests/WebCore/DisplayListTests.cpp
index d45c209e3951..200639129af4 100644
--- a/Tools/TestWebKitAPI/Tests/WebCore/DisplayListTests.cpp
+++ b/Tools/TestWebKitAPI/Tests/WebCore/DisplayListTests.cpp
@@ -74,7 +74,10 @@ TEST(DisplayListTests, AppendItems)
for (int i = 0; i < 50; ++i) {
list.append(SetInlineStroke(1.5));
list.append(FillPath(path));
- list.append(FillRectWithGradient(FloatRect { 1., 1., 10., 10. }, gradient));
+ {
+ auto rect = FloatRect { 1., 1., 10., 10. };
+ list.append(FillRectWithGradient(WTFMove(rect), Ref { gradient }));
+ }
list.append(SetInlineFillColor(Color::red));
#if ENABLE(INLINE_PATH_DATA)
list.append(StrokeLine(PathDataLine { { 0., 0. }, { 10., 15. } }));
Anything else benefits so, I think we should still go with your suggestion, and get away with a single constructor.
Contributor
There was a problem hiding this comment.
Yes I think we should make the apply methods non const functions.
Contributor
Author
There was a problem hiding this comment.
I will have a look at it.
Contributor
Author
There was a problem hiding this comment.
The mutable is unnecessary, there is no const method that attempts to modify 'm_gradient'.
Contributor
There was a problem hiding this comment.
Why does the change cover the fillRect() only? Do not we need to add a similar variant to strokeRect()?
Contributor
Author
There was a problem hiding this comment.
No, stroking is not needed, at least not for the purposes of LBSE gradients. I need to be able to fill a rectangle using the stroke, gradient in a clean way.
Contributor
There was a problem hiding this comment.
Can't just the caller do this:
CGContextStateSaver stateSaver(context);
context.setFillGradient(gradient, gradientSpaceTransform);
context.fillRect(rect);
Contributor
Author
There was a problem hiding this comment.
That is fully correct, however, I need it in a cross-platform fashion.
To achieve that before this patch, I simply adjusted the fill gradient to point to the stroke gradient, fill the rectangle, and swap back. To avoid that, I added the new fillRect variant.
nikolaszimmermann
force-pushed
the
eng/Provide-GraphicsContextfillRectconst-FloatRect-Gradient-const-AffineTransform-gradientSpaceTransform-variant
branch
from
88a79e2 to
c974a5d
Compare
Contributor
Author
|
@shallawa pushed a new version addressing all comments. |
Collaborator
|
EWS run on previous version of this PR (hash c974a5d) Details |
webkit-ews-buildbot
added
the
merging-blocked
Contributor
Author
|
That won't work, e.g. Gtk complains: I will stay with the two constructors, as the other DisplayListItems handle it. |
nikolaszimmermann
removed
the
merging-blocked
nikolaszimmermann
force-pushed
the
eng/Provide-GraphicsContextfillRectconst-FloatRect-Gradient-const-AffineTransform-gradientSpaceTransform-variant
branch
from
c974a5d to
5f3b8af
Compare
Collaborator
|
EWS run on current version of this PR (hash 5f3b8af) Details |
shallawa
approved these changes
Jan 4, 2024
nikolaszimmermann
added
the
merge-queue
…AffineTransform& gradientSpaceTransform) variant https://bugs.webkit.org/show_bug.cgi?id=267040 Reviewed by Said Abou-Hallawa. The existing fillRect(const FloatRect&, Gradient&) accessor, only calls gradient.paint(), but does not handle gradient space transformations, nor drop shadows, nor does it clip to the rect, etc. in contrary to the generic fillRect(const FloatRect&) variant, which supports all these features, but only when using the fillGradient() and the fillGradientSpaceTransform(). For LBSE gradient support it would be useful to be able to fill a rectangle using the _stroke_ gradient, therefore extend the GraphicsContext::fillRect() variants to provide one that takes a specific Gradient + AffineTransform pair. Implement the new fillRect() variant in various places: NullGraphicsContext, BifurcatedGraphicsCOntext, and all other variants inheriting from GraphicsContext. No change in existing functionality, thus no new tests. * Source/WebCore/platform/graphics/BifurcatedGraphicsContext.cpp: (WebCore::BifurcatedGraphicsContext::fillRect): * Source/WebCore/platform/graphics/BifurcatedGraphicsContext.h: * Source/WebCore/platform/graphics/GraphicsContext.h: * Source/WebCore/platform/graphics/NullGraphicsContext.h: * Source/WebCore/platform/graphics/cairo/CairoOperations.cpp: (WebCore::Cairo::FillSource::FillSource): * Source/WebCore/platform/graphics/cairo/CairoOperations.h: * Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp: (WebCore::GraphicsContextCairo::fillRect): * Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.h: * Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp: (WebCore::GraphicsContextCG::fillRect): * Source/WebCore/platform/graphics/cg/GraphicsContextCG.h: * Source/WebCore/platform/graphics/displaylists/DisplayListItem.h: * Source/WebCore/platform/graphics/displaylists/DisplayListItems.cpp: (WebCore::DisplayList::FillRectWithGradient::apply const): (WebCore::DisplayList::FillRectWithGradientAndSpaceTransform::FillRectWithGradientAndSpaceTransform): (WebCore::DisplayList::FillRectWithGradientAndSpaceTransform::apply const): (WebCore::DisplayList::FillRectWithGradientAndSpaceTransform::dump const): * Source/WebCore/platform/graphics/displaylists/DisplayListItems.h: (WebCore::DisplayList::FillRectWithGradientAndSpaceTransform::rect const): (WebCore::DisplayList::FillRectWithGradientAndSpaceTransform::gradient const): (WebCore::DisplayList::FillRectWithGradientAndSpaceTransform::gradientSpaceTransform const): * Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp: (WebCore::DisplayList::Recorder::fillRect): * Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.h: * Source/WebCore/platform/graphics/displaylists/DisplayListRecorderImpl.cpp: (WebCore::DisplayList::RecorderImpl::recordFillRectWithGradientAndSpaceTransform): * Source/WebCore/platform/graphics/displaylists/DisplayListRecorderImpl.h: * Source/WebCore/platform/graphics/nicosia/cairo/NicosiaCairoOperationRecorder.cpp: (Nicosia::CairoOperationRecorder::fillRect): * Source/WebCore/platform/graphics/nicosia/cairo/NicosiaCairoOperationRecorder.h: * Source/WebKit/GPUProcess/graphics/RemoteDisplayListRecorder.cpp: (WebKit::RemoteDisplayListRecorder::fillRectWithGradientAndSpaceTransform): * Source/WebKit/GPUProcess/graphics/RemoteDisplayListRecorder.h: * Source/WebKit/GPUProcess/graphics/RemoteDisplayListRecorder.messages.in: * Source/WebKit/Shared/DisplayListArgumentCoders.serialization.in: * Source/WebKit/WebProcess/GPU/graphics/RemoteDisplayListRecorderProxy.cpp: (WebKit::RemoteDisplayListRecorderProxy::recordFillRectWithGradientAndSpaceTransform): * Source/WebKit/WebProcess/GPU/graphics/RemoteDisplayListRecorderProxy.h: Canonical link: https://commits.webkit.org/272648@main
webkit-commit-queue
force-pushed
the
eng/Provide-GraphicsContextfillRectconst-FloatRect-Gradient-const-AffineTransform-gradientSpaceTransform-variant
branch
from
5f3b8af to
df59d72
Compare
Collaborator
|
Committed 272648@main (df59d72): https://commits.webkit.org/272648@main Reviewed commits have been landed. Closing PR #22352 and removing active labels. |
webkit-commit-queue
removed
the
merge-queue
nikolaszimmermann
deleted the
eng/Provide-GraphicsContextfillRectconst-FloatRect-Gradient-const-AffineTransform-gradientSpaceTransform-variant
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
df59d72
5f3b8af