PathSegments cannot be used with anything else than PathImpl

[kkinnunen-apple]

d132cc1

PathSegments cannot be used with anything else than PathImpl
https://bugs.webkit.org/show_bug.cgi?id=265247
rdar://118717060

Reviewed by Said Abou-Hallawa.

PathImpl and PathSegment were coupled:
PathImpl::appendSegment would call into PathSegment::addToImpl
PathSegment::addToImpl would call into PathImpl

This would be more complex than needed and also limit the PathSegments
to be only used with PathImpl.

Instead:
  - PathSegment is the data that is being held -- no actions toward any
    class that does actual work on the data.
  - PathImpl knows how to use the path segment types: PathImpl::add()
    for each path segment type.
  - PathImpl knows how to use the PathSegment variant:
    PathImpl::addSegment() for the PathSegment, doing generic add() over
    the variants.
  - Make each member function for adding segments PathImpl::add(). This
    way generic algorithms over the variants are consistent to write.
  - The PathImpl::add() parameters are by-value to avoid introducing
    indirection, the arguments are always used.

Construct PathCG, PathCairo from PathStream without applySegments, as
the PathStream segment list can be just iterated.

This works towards being able to play back a PathSegment list to a
CGContext.

* Source/WebCore/platform/graphics/PathImpl.cpp:
(WebCore::PathImpl::appendSegment): Deleted.
* Source/WebCore/platform/graphics/PathImpl.h:
(WebCore::addPathSegment):
* Source/WebCore/platform/graphics/PathSegment.cpp:
(WebCore::PathSegment::addToImpl const): Deleted.
* Source/WebCore/platform/graphics/PathSegment.h:
(WebCore::PathSegment::addTo const):
* Source/WebCore/platform/graphics/PathSegmentData.cpp:
(WebCore::PathMoveTo::addToImpl const): Deleted.
(WebCore::PathLineTo::addToImpl const): Deleted.
(WebCore::PathQuadCurveTo::addToImpl const): Deleted.
(WebCore::PathBezierCurveTo::addToImpl const): Deleted.
(WebCore::PathArcTo::addToImpl const): Deleted.
(WebCore::PathArc::addToImpl const): Deleted.
(WebCore::PathEllipse::addToImpl const): Deleted.
(WebCore::PathEllipseInRect::addToImpl const): Deleted.
(WebCore::PathRect::addToImpl const): Deleted.
(WebCore::PathRoundedRect::addToImpl const): Deleted.
(WebCore::PathDataLine::addToImpl const): Deleted.
(WebCore::PathDataQuadCurve::addToImpl const): Deleted.
(WebCore::PathDataBezierCurve::addToImpl const): Deleted.
(WebCore::PathDataArc::addToImpl const): Deleted.
(WebCore::PathCloseSubpath::addToImpl const): Deleted.
* Source/WebCore/platform/graphics/PathSegmentData.h:
* Source/WebCore/platform/graphics/cairo/PathCairo.cpp:
(WebCore::PathCairo::create):
* Source/WebCore/platform/graphics/cg/PathCG.cpp:
(WebCore::PathCG::create):

Canonical link: https://commits.webkit.org/271199@main

8b87147

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv-sim
✅ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

[webkit-early-warning-system]

EWS run on previous version of this PR (hash 4aa2d57)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	❌ 🧪 api-gtk
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

[webkit-early-warning-system]

EWS run on previous version of this PR (hash 80181a8)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	🛠 wpe	🛠 wincairo
🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	🧪 wpe-wk2
✅ 🧪 webkitperl	🧪 ios-wk2	🧪 api-mac	🧪 api-wpe
	🧪 ios-wk2-wpt	🧪 mac-wk1	🛠 gtk
⏳ 🛠 🧪 jsc	🧪 api-ios	🧪 mac-wk2	🧪 gtk-wk2
	✅ 🛠 tv	🧪 mac-AS-debug-wk2	🧪 api-gtk
	🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

[webkit-early-warning-system]

EWS run on previous version of this PR (hash 25b11c9)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	❌ 🛠 wpe	❌ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2
✅ 🧪 webkitperl	🧪 ios-wk2	🧪 api-mac	❌ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	🧪 mac-wk1	❌ 🛠 gtk
⏳ 🛠 🧪 jsc	✅ 🧪 api-ios	🧪 mac-wk2	❌ 🧪 gtk-wk2
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	❌ 🧪 api-gtk
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

[webkit-early-warning-system]

EWS run on previous version of this PR (hash 3e3ce5e)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	❌ 🛠 wpe	❌ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	❌ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	❌ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	❌ 🧪 gtk-wk2
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	❌ 🧪 api-gtk
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

[webkit-early-warning-system]

EWS run on previous version of this PR (hash ae45f40)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	❌ 🛠 wpe	❌ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	🧪 api-mac	❌ 🧪 api-wpe
	🧪 ios-wk2-wpt	🧪 mac-wk1	❌ 🛠 gtk
⏳ 🛠 🧪 jsc	✅ 🧪 api-ios	🧪 mac-wk2	❌ 🧪 gtk-wk2
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	❌ 🧪 api-gtk
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

[webkit-early-warning-system]

EWS run on current version of this PR (hash 8b87147)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv-sim
✅ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

[shallawa]

Is it okay to pass the PathSegment and the PathSegment data structures by value? PathRoundedRect has 12 floats and an enum.

[kkinnunen-apple]

I would think so -- the PathSegment is only passed when it's a syntactic sugar over the individual calls. So no callframe should be generated, rather the switch should operate on the caller's value.
The individual PathSegment::Data types, especially small ones, should mostly be neutral when passed as a value:
For example, add(PathMoveTo) is two floats, where as moveTo(const FloatPoint&) passes the two simple values with a pointer indirection. Here the PathMoveTo variant should be a progression.

However, very big ones like PathRoundedRect, the ones that might have unread, redundant values, is a bit of a question mark. I would imagine that the effect is neutral, but I have not benchmarked it extensively.

Mostly I think the pass-by-value is progression if:

• There is not many call layers that just forward the parameters (there is not)
• There is not many variables that are passed redundantly and never read by the payload function (all except rounded rect variant)

At least if the problem is the latter, redundant values being passed, this would mean that the command is overloaded: two different methods in one. This is the case with "add rounded rect with platform strategy", "add rounded rect with webkit strategy". So a better, more consistent approach to fix that particular problem would be tahat these two could be split into two separate methods, if they mean different things.

[shallawa]

If the Impl is PathCG, this adds the overhead of copying the point to structure here. Then getting the address of the point in the structure in PathCG::add(PathMoveTo).

So I do not see the benefit of this change so far. Maybe in #10594, I will see the benefit of it.

[kkinnunen-apple]

If the Impl is PathCG, this adds the overhead of copying the point to structure here.

No, I don't think there's an additional overhead.
Before, the pointer read to registers happened in PathCG::moveTo.
After, the pointer read to registers happens in Path::addLineTo.
It's the same amount of reads.

Then getting the address of the point in the structure in PathCG::add(PathMoveTo).

No, there's no pointers anymore in PathCG::add. It's pass by value.
I think the values are already in the registers.

[kkinnunen-apple]

So I do not see the benefit of this change so far. Maybe in #10594, I will see the benefit of it.

The intended benefit of having uniform name and passing the PathSegment::Data values as values:
It is done to be able to write polymorphic functions (==templates) over PathSegment.

For PathImpl::add over moveTo,bezierCurveTo,arcTo, it's evident: we are able to deconstruct PathSegment and apply the deconstructed variants in uniform fashion, and we don't need to further deconstruct the various PathMoveTo, ..., structs into moveTo(a,b,c,) calls redundantly.

[webkit-commit-queue]

Committed 271199@main (d132cc1): https://commits.webkit.org/271199@main

Reviewed commits have been landed. Closing PR #20819 and removing active labels.