GraphicsContext: Remove "current path" and add a path argument to strokePath, fillPath and clipPath

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Andreas Kling]

GraphicsContext currently has a "current path", set using GraphicsContext::beginPath() and GraphicsContext::addPath()

This path is then implicitly used by strokePath(), fillPath() and clipPath(). To make it more confusing, due to a quirk of CGContextStrokePath, strokePath() is expected to clear the current path. This is poorly handled ATM, for example there is some mysterious Skia-specific code handling the case in RenderSVGResourceSolidColor::postApplyResource().

I propose we refactor GraphicsContext to take a Path object as an argument to these functions and remove the concept of a "current path."

[Andreas Kling]

Created attachment 72179 [details]
Proposed patch

First stab at this, tested on Qt, CG and Cairo.

(In reply to comment #0)
> This is poorly handled ATM, for example there is some mysterious Skia-specific code handling the case in RenderSVGResourceSolidColor::postApplyResource().

In my opinion, a bug in the implementation is not good justification for changing the API.

[Dirk Schulze]

I think it would be better, if fillPath and strokePath don't take Path arguments. Instead they shouldn't delete the current path after filling or stroking a shape. We could handle it manually. IIRC the most platforms support this (not sure about CG).
This way we would save the operation of re-adding the same path onto the context for a fill+stroke operation of shapes in SVG.
What do you think?

[Andreas Kling]

(In reply to comment #2)
> In my opinion, a bug in the implementation is not good justification for changing the API.

You're right, the primary reason I wanted to do this was to get a cleaner, more concise API.

(In reply to comment #3)
> This way we would save the operation of re-adding the same path onto the context for a fill+stroke operation of shapes in SVG.

You're right, I didn't properly consider the fill+stroke redundancy since that doesn't happen on Qt.

New suggestion: how about replacing beginPath()+addPath() with setPath(const Path&) (since that's how it's used anyway) and not expecting strokePath() or fillPath() to touch the current path?

[Dirk Schulze]

(In reply to comment #4)
> New suggestion: how about replacing beginPath()+addPath() with setPath(const Path&) (since that's how it's used anyway) and not expecting strokePath() or fillPath() to touch the current path?

Like you said, setPath would make use of beginPath()+addPath() anyway. So where is the improvement, if fillPath() and strokePath don't use the currentPath?
Btw, why doesn't fillPath or strokePath on Qt close the currentPath? IIRC it was the way I implemented it on Qt. Did it change? When yes, why?

Nevertheless, it's ok to use setPath, but I'd still let fillPath and strokePath use the currentPath, just make sure that we don't close/clear the path and do it manually instead.

[Andreas Kling]

(In reply to comment #5)
> (In reply to comment #4)
> > New suggestion: how about replacing beginPath()+addPath() with setPath(const Path&) (since that's how it's used anyway) and not expecting strokePath() or fillPath() to touch the current path?
> 
> Like you said, setPath would make use of beginPath()+addPath() anyway. So where is the improvement, if fillPath() and strokePath don't use the currentPath?

IMO it would make the API less ambiguous - why have the concept of "addPath" when we're never actually adding multiple paths?

For Qt, there's the additional advantage that we don't have a "platform beginPath()" so setPath() means a little less work for us.

> Btw, why doesn't fillPath or strokePath on Qt close the currentPath?

It does both, actually.

> Nevertheless, it's ok to use setPath, but I'd still let fillPath and strokePath use the currentPath, just make sure that we don't close/clear the path and do it manually instead.

I like this idea, though I'm not sure what the best way to do that on CG is.

[Dirk Schulze]

(In reply to comment #6)
> I like this idea, though I'm not sure what the best way to do that on CG is.
So CGContextStrokePath/CGContextFillPath clear the currentPath after the drawing operation and there is no way to prevent CG to do it?

[Andreas Kling]

(In reply to comment #7)
> (In reply to comment #6)
> > I like this idea, though I'm not sure what the best way to do that on CG is.
> So CGContextStrokePath/CGContextFillPath clear the currentPath after the drawing operation and there is no way to prevent CG to do it?

For CGContextStrokePath: "As a side effect when you call this function, Quartz clears the current path."

Nothing is said about it for CGContextFillPath, so I assume it doesn't happen.

Source:
http://edr.euro.apple.com/library/mac/documentation/GraphicsImaging/Reference/CGContext/Reference/reference.html

[Dirk Schulze]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > I like this idea, though I'm not sure what the best way to do that on CG is.
> > So CGContextStrokePath/CGContextFillPath clear the currentPath after the drawing operation and there is no way to prevent CG to do it?
> 
> For CGContextStrokePath: "As a side effect when you call this function, Quartz clears the current path."
> 
> Nothing is said about it for CGContextFillPath, so I assume it doesn't happen.

Hm, not sure. A CG guy should comment on this. Nevertheless, SVG 2.0 will make it possible to change the order of drawing fill and stroke, so it would still be helpful if CGContextStrokePath wouldn't clear the path :-(

[Nikolas Zimmermann]

I'd like to share my opinion. Let's examine the users of the code, all do:

Path myPath;
path.doSomething();

context->beginPath();
context->addPath(myPath);
context->fillPath();

The problem we want to solve is to avoid multiples copies of the Path object. There are multiple options, like making it RefCounted, which I dislike though.

The point is that the path _has to be created_ and stored in the "client code" that wants to use GraphicsContext. So why storing it again the GraphicsContext?

We should really think about changing this concept, that means fillPath(const Path&), strokePath(const Path&) is a good option IMHO.

The reason why the current concept is as-is, is CoreGraphics cocept of a current path, added to the CGContext. We still satisfy CG by doing this from the fill/strokePath methods.

What do you think?

[Nikolas Zimmermann]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (In reply to comment #6)
> > > > I like this idea, though I'm not sure what the best way to do that on CG is.
> > > So CGContextStrokePath/CGContextFillPath clear the currentPath after the drawing operation and there is no way to prevent CG to do it?
> > 
> > For CGContextStrokePath: "As a side effect when you call this function, Quartz clears the current path."
> > 
> > Nothing is said about it for CGContextFillPath, so I assume it doesn't happen.
> 
> Hm, not sure. A CG guy should comment on this. Nevertheless, SVG 2.0 will make it possible to change the order of drawing fill and stroke, so it would still be helpful if CGContextStrokePath wouldn't clear the path :-(
It's no problem that the stroke path is cleared, if strokePath would take a const Path&, we could easily readd it after clearing. But I don't think there's any need to do that....

[Andreas Kling]

Jumped the gun on renaming the bug, let's have some more discussion.

[Dirk Schulze]

(In reply to comment #11)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > (In reply to comment #7)
> > > > (In reply to comment #6)
> > > > > I like this idea, though I'm not sure what the best way to do that on CG is.
> > > > So CGContextStrokePath/CGContextFillPath clear the currentPath after the drawing operation and there is no way to prevent CG to do it?
> > > 
> > > For CGContextStrokePath: "As a side effect when you call this function, Quartz clears the current path."
> > > 
> > > Nothing is said about it for CGContextFillPath, so I assume it doesn't happen.
> > 
> > Hm, not sure. A CG guy should comment on this. Nevertheless, SVG 2.0 will make it possible to change the order of drawing fill and stroke, so it would still be helpful if CGContextStrokePath wouldn't clear the path :-(
> It's no problem that the stroke path is cleared, if strokePath would take a const Path&, we could easily readd it after clearing. But I don't think there's any need to do that....

If you call addPath, you do a copy anyway. Maybe just internally by the graphic library, but you have to add the path to the context again. This issue can be avoided on Qt and Cairo. And that was we were talking about.

[Dirk Schulze]

Hm, looked at Cairo. The implementation really changed to a stored currentPath. That's a pity. This should really be removed. Under this premise it makes really a lot of sense to give a Path&. Maybe not much for CG, since it has a different concept for current path, but definitely for other platforms.

[Nikolas Zimmermann]

(In reply to comment #13)
> (In reply to comment #11)
> > (In reply to comment #9)
> > > (In reply to comment #8)
> > > > (In reply to comment #7)
> > > > > (In reply to comment #6)
> > > > > > I like this idea, though I'm not sure what the best way to do that on CG is.
> > > > > So CGContextStrokePath/CGContextFillPath clear the currentPath after the drawing operation and there is no way to prevent CG to do it?
> > > > 
> > > > For CGContextStrokePath: "As a side effect when you call this function, Quartz clears the current path."
> > > > 
> > > > Nothing is said about it for CGContextFillPath, so I assume it doesn't happen.
> > > 
> > > Hm, not sure. A CG guy should comment on this. Nevertheless, SVG 2.0 will make it possible to change the order of drawing fill and stroke, so it would still be helpful if CGContextStrokePath wouldn't clear the path :-(
> > It's no problem that the stroke path is cleared, if strokePath would take a const Path&, we could easily readd it after clearing. But I don't think there's any need to do that....
> 
> If you call addPath, you do a copy anyway. Maybe just internally by the graphic library, but you have to add the path to the context again. This issue can be avoided on Qt and Cairo. And that was we were talking about.

I'm not talking about GraphicsContext::addPath. In cg Path wraps a CGPath object, and CGContextAddPath just takes a CGPathRef (handle to the CGPath). That is _not_ a copy.

[Dirk Schulze]

(In reply to comment #15)
> I'm not talking about GraphicsContext::addPath. In cg Path wraps a CGPath object, and CGContextAddPath just takes a CGPathRef (handle to the CGPath). That is _not_ a copy.

And I was more thinking about Cairo. And we do a lot of cairo_copy_path there.

[Nikolas Zimmermann]

(In reply to comment #16)
> (In reply to comment #15)
> > I'm not talking about GraphicsContext::addPath. In cg Path wraps a CGPath object, and CGContextAddPath just takes a CGPathRef (handle to the CGPath). That is _not_ a copy.
> 
> And I was more thinking about Cairo. And we do a lot of cairo_copy_path there.

I wanted to move the discussion towards _our_ API, and how we can avoid copying Path, and making the GraphicsContext API nicer. I'd like to reach consensus about that first, then look at the individual graphic libraries.

Path foobar;
foobar.addRect(...);

context->fillPath(foobar);
context->strokePath(foobar);
seems like a nice API, where fill/strokePath take a const Path&, to avoid a copy.

Do we all agree on that? If so, let's look at how we can implement that kind of API efficiently in CG/Qt/GTK/Skia. If we'll then reach the point, where we need changes in the design, we can continue discussing that, but I'd like to see a nice API proposal first, and here's mine, which is I think exactly like Andreas (we discussed it before, btw.)

[Andreas Kling]

(In reply to comment #17)
> Do we all agree on that? If so, let's look at how we can implement that kind of API efficiently in CG/Qt/GTK/Skia. If we'll then reach the point, where we need changes in the design, we can continue discussing that, but I'd like to see a nice API proposal first, and here's mine, which is I think exactly like Andreas (we discussed it before, btw.)

Yes, this is what my patch implements, and I still think it's the most elegant API. Let's hear from some more people. :)

[Dirk Schulze]

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > I'm not talking about GraphicsContext::addPath. In cg Path wraps a CGPath object, and CGContextAddPath just takes a CGPathRef (handle to the CGPath). That is _not_ a copy.
> > 
> > And I was more thinking about Cairo. And we do a lot of cairo_copy_path there.
> 
> I wanted to move the discussion towards _our_ API, and how we can avoid copying Path, and making the GraphicsContext API nicer. I'd like to reach consensus about that first, then look at the individual graphic libraries.

haha. Before we can work on a "nicer" API for GraphicsContext we _have to_ discuss how we can implement it efficiently on platforms, not after we implemented it.
But I'm really fine with the new headline of this bug. We really don't want current path on Cairo. And I guess Martin and Alex agree to me. I'd like to have at least one comment of the Skia guys.

The work on WebKit does not really rely on this patch. So we should take the time now, instead of redesigning a few times.

[Nikolas Zimmermann]

(In reply to comment #19)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > (In reply to comment #15)
> > > > I'm not talking about GraphicsContext::addPath. In cg Path wraps a CGPath object, and CGContextAddPath just takes a CGPathRef (handle to the CGPath). That is _not_ a copy.
> > > 
> > > And I was more thinking about Cairo. And we do a lot of cairo_copy_path there.
> > 
> > I wanted to move the discussion towards _our_ API, and how we can avoid copying Path, and making the GraphicsContext API nicer. I'd like to reach consensus about that first, then look at the individual graphic libraries.
> 
> haha. Before we can work on a "nicer" API for GraphicsContext we _have to_ discuss how we can implement it efficiently on platforms, not after we implemented it.
Would you please read more carefully, to not spread more confusion than needed?
I said we should _discuss_ how we like to have the API, I never said "implement it".

We should reach consensus on how the API _should_ look like. Then we can look how _and if_ the new API can be implemented efficiently, then reevaluate the desired design if needed.

> But I'm really fine with the new headline of this bug. We really don't want current path on Cairo. And I guess Martin and Alex agree to me. I'd like to have at least one comment of the Skia guys.
> 
> The work on WebKit does not really rely on this patch. So we should take the time now, instead of redesigning a few times.

*sigh* Dirk, please read more carefully, before replying. To restate: I never said, I want to implement the new design, but rather _think_ of a new design. If everybody agress the new design would be best, then let's see whether it can be implemented efficiently. If not, refine the design.

[Jakob Petsovits]

I find the proposed API a good idea. The current OpenVG branch has those methods in PainterOpenVG already, and automatic deletion of paths is unintuitive and confusing, in one place in our port I specifically used the platform context directly just to avoid the clumsy GraphicsContext API and its path copies. +1 from me, Andreas ftw!

[Dirk Schulze]

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #17)
> > > (In reply to comment #16)
> > > > (In reply to comment #15)
> > > > > I'm not talking about GraphicsContext::addPath. In cg Path wraps a CGPath object, and CGContextAddPath just takes a CGPathRef (handle to the CGPath). That is _not_ a copy.
> > > > 
> > > > And I was more thinking about Cairo. And we do a lot of cairo_copy_path there.
> > > 
> > > I wanted to move the discussion towards _our_ API, and how we can avoid copying Path, and making the GraphicsContext API nicer. I'd like to reach consensus about that first, then look at the individual graphic libraries.
> > 
> > haha. Before we can work on a "nicer" API for GraphicsContext we _have to_ discuss how we can implement it efficiently on platforms, not after we implemented it.
> Would you please read more carefully, to not spread more confusion than needed?
> I said we should _discuss_ how we like to have the API, I never said "implement it".
> 
> We should reach consensus on how the API _should_ look like. Then we can look how _and if_ the new API can be implemented efficiently, then reevaluate the desired design if needed.
> 
> > But I'm really fine with the new headline of this bug. We really don't want current path on Cairo. And I guess Martin and Alex agree to me. I'd like to have at least one comment of the Skia guys.
> > 
> > The work on WebKit does not really rely on this patch. So we should take the time now, instead of redesigning a few times.
> 
> *sigh* Dirk, please read more carefully, before replying. To restate: I never said, I want to implement the new design, but rather _think_ of a new design. If everybody agress the new design would be best, then let's see whether it can be implemented efficiently. If not, refine the design.

Great! So we agree to each other, fine. And I just wrote that from the Cairo point of few it is better to get rid of the current path concept, and therefor use to fillPath(Path&)/strokePath(Path&) is good. No need to be aggressive or irritated :-)

[Nikolas Zimmermann]

(In reply to comment #22).
> 
> Great! So we agree to each other, fine. And I just wrote that from the Cairo point of few it is better to get rid of the current path concept, and therefor use to fillPath(Path&)/strokePath(Path&) is good. No need to be aggressive or irritated :-)

It shouldn't sounds aggressive, sorry if you felt so :-)
From a CG and Qt point of view, that design would also be possible.
I've looked again through Andreas patch, and I think the CG side is fine, Qt as well.

The Skia code should probably be made smarter, before landing the patch as-is.
Needs some input from the Chromium team (James Robinsion?)
For WinCE we should talk to Patrick Gansterer, for Wx to Kevin Ollivier.

They're all CC'ed we should wait for their comments...

[Patrick R. Gansterer]

(In reply to comment #23)
> For WinCE we should talk to Patrick Gansterer, for Wx to Kevin Ollivier.
IMHO it's a good idea too, so +1. It shouldn't be a big difference for WinCE, because it only saves the paths as a Vector<Path> member in GraphicsContext and iterates of them in fillPath.

[Martin Robinson]

(In reply to comment #14)
> Hm, looked at Cairo. The implementation really changed to a stored currentPath. That's a pity. This should really be removed. Under this premise it makes really a lot of sense to give a Path&. Maybe not much for CG, since it has a different concept for current path, but definitely for other platforms.

Unsure yet if this affects the API discussion, but I made this change, so I'll explain it. GraphicsContext assumes that things like ::fillRect and ::strokeRect are not affected by or clear the current path. The solution was to wait until the last possible moment to actually apply the path to the Cairo context.

[Stephen White]

Comment on attachment 72179 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=72179&action=review

This looks great.

I think the beginPath/addPath() functions could be removed from skia too.  There's nothing in skia itself that requires these semantics, AFAIK (the current path is stored in PlatformContextSkia::m_path, and reset in beginPath() -- which was wrong; it should probably have been reset in fillPath()/strokePath() to emulate the CG behaviour).  Anyway, you can either do this yourself, or I can do it after this patch lands.

> WebCore/inspector/InspectorController.cpp:-1819
> -        context.addPath(quadPath);

Out of curiosity, was this a bug?  It looks like the path was being added twice (before and after the clipOut).  Or did the semantics of clipOut() change in this patch too?

[Andreas Kling]

(In reply to comment #26)
> > WebCore/inspector/InspectorController.cpp:-1819
> > -        context.addPath(quadPath);
> 
> Out of curiosity, was this a bug?  It looks like the path was being added twice (before and after the clipOut).  Or did the semantics of clipOut() change in this patch too?

Not a bug, merely redundant, since clipOut() takes a Path parameter rather than using the current context path.

[Stephen White]

(In reply to comment #27)
> (In reply to comment #26)
> > > WebCore/inspector/InspectorController.cpp:-1819
> > > -        context.addPath(quadPath);
> > 
> > Out of curiosity, was this a bug?  It looks like the path was being added twice (before and after the clipOut).  Or did the semantics of clipOut() change in this patch too?
> 
> Not a bug, merely redundant, since clipOut() takes a Path parameter rather than using the current context path.

Right, but it looks like the path would have been added twice to the current path for the subsequent strokePath().  Anyway, no big deal, if it was a bug, your patch fixes it.

[Alejandro G. Castro]

(In reply to comment #19)
>
> But I'm really fine with the new headline of this bug. We really don't want current path on Cairo. And I guess Martin and Alex agree to me. I'd like to have at least one comment of the Skia guys.

Yeah, I agree, I think we will save at least one cairo copy here.

[Dirk Schulze]

Comment on attachment 72179 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=72179&action=review

The design looks good. A CG guy should look at the code GraphicsContextCG changes. Kling will also run pixeltests with this patch, to make sure that we don't get regressions.

> WebCore/platform/graphics/GraphicsContext.h:306
>  

Add a fixme, that these functions are deprecated.

> WebCore/platform/graphics/cg/GraphicsContextCG.cpp:307
> -    CGContextRef context = platformContext();
> -    CGContextBeginPath(context);
> -    float r = (float)rect.width() / 2;
> -    CGContextAddArc(context, rect.x() + r, rect.y() + r, r, 0.0f, 2.0f * piFloat, 0);
> -    CGContextClosePath(context);
> -
> -    drawPath();
> +    Path path;
> +    path.addEllipse(rect);
> +    drawPath(path);

Not sure, if the code in PathCG looks identical, if not, this will change some pixel tests (maybe just visible with a tolerance of 0)

> WebCore/platform/graphics/cg/GraphicsContextCG.cpp:399
> +static Path createConvexPolygonPath(size_t numPoints, const FloatPoint* points)

Doesn't this give back a copy of the created Path?

[Dirk Schulze]

(In reply to comment #30)
> Add a fixme, that these functions are deprecated.
Hm, was talking about beginPath and addPath in GraphicsContext.h, not sure why the marked lines are not on the comment.

[Andreas Kling]

Created attachment 72503 [details]
Proposed patch v2

Rebased and updated patch.

beginPath() -> deprecatedBeginPath()
addPath() -> deprecatedAddPath()

Layout test results on Snow Leopard (I don't think any of the failures are legit):
http://chaos.troll.no/~kling/48516/layout-test-results/results.html

[Oliver Hunt]

(In reply to comment #32)
> Created an attachment (id=72503) [details]
> Proposed patch v2
> 
> Rebased and updated patch.
> 
> beginPath() -> deprecatedBeginPath()
> addPath() -> deprecatedAddPath()
> 
> Layout test results on Snow Leopard (I don't think any of the failures are legit):
> http://chaos.troll.no/~kling/48516/layout-test-results/results.html

Has this patch been performance tested?

I am slightly concerned by this patch as it seems to not gain us anything other than move from being more like one API to being more like another.  I certainly think we need a very good reason before making such a dramatic change.

[Dirk Schulze]

(In reply to comment #33)
> (In reply to comment #32)
> > Created an attachment (id=72503) [details] [details]
> > Proposed patch v2
> > 
> > Rebased and updated patch.
> > 
> > beginPath() -> deprecatedBeginPath()
> > addPath() -> deprecatedAddPath()
> > 
> > Layout test results on Snow Leopard (I don't think any of the failures are legit):
> > http://chaos.troll.no/~kling/48516/layout-test-results/results.html
> 
> Has this patch been performance tested?
> 
> I am slightly concerned by this patch as it seems to not gain us anything other than move from being more like one API to being more like another.  I certainly think we need a very good reason before making such a dramatic change.

Many posters above already talked about the benefits. The main benefit in my eyes is that most platforms don't support the currenPath concept of CG. Storing the currentPath is a performance and memory issue on those platforms. This is a very good reason to move to a new concept for me.
On the other hand, I don't see performance issues for CG, beside the one I blamed on my first review. If there are performance problems, they need to be fixed before this patch gets landed of course. Please help us to find them.
Is there a performance test on webkit for graphics?

[Nikolas Zimmermann]

(In reply to comment #34)
> (In reply to comment #33)
> > (In reply to comment #32)
> > > Created an attachment (id=72503) [details] [details] [details]
> > > Proposed patch v2
> > > 
> > > Rebased and updated patch.
> > > 
> > > beginPath() -> deprecatedBeginPath()
> > > addPath() -> deprecatedAddPath()
> > > 
> > > Layout test results on Snow Leopard (I don't think any of the failures are legit):
> > > http://chaos.troll.no/~kling/48516/layout-test-results/results.html
> > 
> > Has this patch been performance tested?
> > 
> > I am slightly concerned by this patch as it seems to not gain us anything other than move from being more like one API to being more like another.  I certainly think we need a very good reason before making such a dramatic change.

We should get some numbers, agreed. But certainly calling one method instead of three is better.

context->beginPath();
context->addPath(path);
context->fillPath()

vs. context->fillPath(path).

Dirk already said that we also save memory, as the current path concept is not giving us any benefits.
And I don't think this patch is anything but dramatic, it's rather simple.

[Andreas Kling]

Created attachment 74492 [details]
Proposed patch v3

Rebased on trunk. No code changes since last time.

> Has this patch been performance tested?

Yes, on Qt. It improves performance for us (nothing to write home about, but a measurable progression), since we avoid a bunch of work maintaining transforms etc. The same should eventually be true for everyone but CG, since we're all emulating the "current path" concept.

People with port-specific knowledge may need to come in after this lands and tweak whatever's appropriate for their GraphicsContext implementation though. That will mostly consist of removing code.

[Dirk Schulze]

Comment on attachment 74492 [details]
Proposed patch v3

View in context: https://bugs.webkit.org/attachment.cgi?id=74492&action=review

> WebCore/platform/graphics/cg/GraphicsContextCG.cpp:399
> +static Path createConvexPolygonPath(size_t numPoints, const FloatPoint* points)

Path& not Path. Don't copy the created path.

> WebCore/platform/graphics/qt/GraphicsContextQt.cpp:491
> +void GraphicsContext::fillPath(const Path& pathToFill)

Is fillPath better? Not sure.

> WebCore/platform/graphics/qt/GraphicsContextQt.cpp:529
> +void GraphicsContext::strokePath(const Path& pathToStroke)

Ditto. strokePath?

> WebCore/platform/graphics/qt/GraphicsContextQt.cpp:755
> +void GraphicsContext::clipPath(const Path& path, WindRule clipRule)

That is inconsistent, you name it pathToFill and pathToStroke above and name the new path just 'path', but here you call it 'path' and the new path 'newPath'. Could you use one schema? E.g. clipPath here?

> WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:423
> +void GraphicsContext::clipPath(const Path& pathToClip, WindRule clipRule)

See above.

> WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:730
> +void GraphicsContext::fillPath(const Path& pathToFill)

Ditto.

> WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:1188
> +void GraphicsContext::strokePath(const Path& pathToStroke)

Ditto.

> WebCore/platform/graphics/wince/GraphicsContextWinCE.cpp:1332
> +    // FIXME: Be smarter about this.
> +    beginPath();
> +    addPath(path);

You use this pattern multiple times, how would you make it smarter? What is wrong with it right now? Just that you use the GC functions instead of platform code? Can you add more details please?

> WebCore/platform/graphics/wince/GraphicsContextWinCE.cpp:1386
> +    // FIXME: Be smarter about this.
> +    beginPath();
> +    addPath(path);
> +

Ditto. And on other places as well.

> WebCore/platform/graphics/wx/GraphicsContextWx.cpp:541
> +    // FIXME: Be smarter about this.
> +    beginPath();
> +    addPath(path);

Ditto. Won't continue this on the bug.

> WebCore/rendering/RenderObject.cpp:958
> -        graphicsContext->addPath(borderPath);
> -        graphicsContext->strokePath();
> +        graphicsContext->strokePath(borderPath);

hm, Is this correct? We added a Path to the existing one before, with your patch you delete the existing one. Is it possible that an old Path still exist? If not, where is the beginPath()?

> WebCore/rendering/svg/RenderSVGPath.cpp:186
> -        context->addPath(m_path);
> +        path = m_path;

this slows down the common case. Please don't copy the path here!

> WebCore/rendering/svg/RenderSVGPath.cpp:189
> +        strokePaintingResource->postApplyResource(this, context, ApplyToStrokeMode, path);

better to add something like nonScalingStroke ? path : m_path

> WebCore/rendering/svg/RenderSVGPath.cpp:194
> +            fallbackResource->postApplyResource(this, context, ApplyToStrokeMode, path);

ditto.

[Simon Fraser (smfr)]

(In reply to comment #37)
> (From update of attachment 74492 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74492&action=review
> 
> > WebCore/platform/graphics/cg/GraphicsContextCG.cpp:399
> > +static Path createConvexPolygonPath(size_t numPoints, const FloatPoint* points)
> 
> Path& not Path. Don't copy the created path.

You can't return a reference here; the Path is being created.

[Dirk Schulze]

(In reply to comment #38)
> (In reply to comment #37)
> > (From update of attachment 74492 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=74492&action=review
> > 
> > > WebCore/platform/graphics/cg/GraphicsContextCG.cpp:399
> > > +static Path createConvexPolygonPath(size_t numPoints, const FloatPoint* points)
> > 
> > Path& not Path. Don't copy the created path.
> 
> You can't return a reference here; the Path is being created.

Yes, but maybe if can be a void function and it gets a Path&. Of course it will need the caller functions to create a Path first, but I think it is still better.

[Andreas Kling]

(In reply to comment #37)
> > WebCore/platform/graphics/wince/GraphicsContextWinCE.cpp:1386
> > +    // FIXME: Be smarter about this.
> > +    beginPath();
> > +    addPath(path);
> > +
> You use this pattern multiple times, how would you make it smarter? What is wrong with it right now? Just that you use the GC functions instead of platform code? Can you add more details please?

I don't know what the best thing to do on each platform is. It looks like this could be done in nicer ways, but since I can't even build this code, I'd rather not make any larger changes.

> > WebCore/rendering/RenderObject.cpp:958
> > -        graphicsContext->addPath(borderPath);
> > -        graphicsContext->strokePath();
> > +        graphicsContext->strokePath(borderPath);
> 
> hm, Is this correct? We added a Path to the existing one before, with your patch you delete the existing one. Is it possible that an old Path still exist? If not, where is the beginPath()?

I believe that's simply a bug in the current code. Though keep in mind that some GraphicsContext calls clear the path on completion, so a beginPath() may not be necessary.

[Andreas Kling]

Created attachment 74626 [details]
Proposed patch v4

[Eric Seidel (no email)]

Attachment 74626 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6330010

[Andreas Kling]

Created attachment 74629 [details]
Proposed patch v5

D'oh! Add missing semicolon in GraphicsContextCG.cpp..

[Eric Seidel (no email)]

Attachment 74626 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6419005

[Nikolas Zimmermann]

Comment on attachment 74629 [details]
Proposed patch v5

View in context: https://bugs.webkit.org/attachment.cgi?id=74629&action=review

Looks great to me, still a nitpick though:

> WebCore/platform/graphics/GraphicsContext.h:304
> +#if !PLATFORM(QT) && !PLATFORM(CAIRO) && !PLATFORM(CG) && !PLATFORM(HAIKU)

Could you only list the platforms that actually need begin/addPath here, instead of blacklisting those who don't?

> WebCore/rendering/svg/SVGInlineTextBox.cpp:347
> +    releasePaintingResource(context, Path());

Hm, this seems wrong to me. The text code path doesn't need any Path usage at all for painting text.
This implies that postApplyResource() should take a Path* pointer, so that we can avoid creating a temporary Path() object here. Agreed?

> WebCore/rendering/svg/SVGInlineTextBox.cpp:505
> +        releasePaintingResource(context, path);

You'd just have to use &path here then.

[Andreas Kling]

Created attachment 75150 [details]
Proposed patch v6

Addressed Niko's comments- postApplyResource() now takes a Path* instead of a Path&

[Dirk Schulze]

Comment on attachment 75150 [details]
Proposed patch v6

View in context: https://bugs.webkit.org/attachment.cgi?id=75150&action=review

Looks great! Don't think that it needs another round. But would be great if you can fix the following snippets. r=me.

> WebCore/ChangeLog:5
> +        GraphicsContext: Remove "current path" and have strokePath, fillPath and clipPath take a Path argument.

I sometime make mistakes with english phrasing. Not sure, but have ... take sounds strange :-P

> WebCore/platform/graphics/cg/GraphicsContextCG.cpp:@
>  void GraphicsContext::drawConvexPolygon(size_t npoints, const FloatPoint* points

Don't like the name npoints. Can you rename it to numPoints like in addConvexPolygonToPath? Or rename both to numberOfPoints?

[Darin Adler]

Retitled this based on Dirk’s comment.

[Andreas Kling]

Committed r72926: <http://trac.webkit.org/changeset/72926>