Bug 23860 – Resync some graphics/skia files with their chromium counterparts

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Evan Stade]

These files have gotten out of sync and I suspect they are currently quite buggy. 

The original chromium changelists that caused this were http://codereview.chromium.org/17633 and http://codereview.chromium.org/17454 .

[Evan Stade]

Created attachment 27511 [details]
patch

[Eric Seidel (no email)]

Comment on attachment 27511 [details]
patch

Looks fine

+    // Current path in global coordinates.

isn't actually right, is it?

If you change the current matrix, we don't update the path.  So won't currentPath() do the wrong thing?

Also WebKit style has {
+SkPath PlatformContextSkia::currentPath() const {
on its own line.

r- for the concerns about currentPath() doing the wrong thing if you have changed the current transform since setting the path.

[Evan Stade]

(In reply to comment #2)
> (From update of attachment 27511 [details] [review])
> Looks fine
> 
> +    // Current path in global coordinates.
> 
> isn't actually right, is it?
> 
> If you change the current matrix, we don't update the path.  So won't
> currentPath() do the wrong thing?
> 
> Also WebKit style has {
> +SkPath PlatformContextSkia::currentPath() const {
> on its own line.
> 
> r- for the concerns about currentPath() doing the wrong thing if you have
> changed the current transform since setting the path.
> 

That is actually what this patch is fixing. The path is only supposed to be transformed at the time when it is added. That is, we *do* want these two code paths to do different things:

* context->addPathPoints(a)
* context->appendTransform(t)
* context->addPathPoints(b)
========== vs. ===========
* context->appendTransform(t)
* context->addPathPoints(a)
* context->addPathPoints(b)

[Evan Stade]

Created attachment 27669 [details]
fix curly brace

[Eric Seidel (no email)]

Comment on attachment 27669 [details]
fix curly brace

I would name currentPath() to currentPathInLocalCoordinates() then you don't need the comment and all the callsites are clear.

I would add a comment next to m_path explaining how it's stored transformed to global coordinates *at time of setting* and that currentPathInLocalCoordinates applies the inverse globalTransform at time of getting so that getCTM().transform(currerntPathInLocalCoordinates()) works.

WebKit uses localPath instead of local_path and inverseMatrix instead of inverse_matrix.

r- for the style issues.

[Evan Stade]

Created attachment 27760 [details]
try3

changed currentPath() to currentPathInLocalCoordinates(), fixed local variable naming style issues

[Eric Seidel (no email)]

Comment on attachment 27760 [details]
try3

r=me

[Darin Fisher (:fishd, Google)]

Landed as http://trac.webkit.org/changeset/41064