Bug 23860 - Resync some graphics/skia files with their chromium counterparts
Summary: Resync some graphics/skia files with their chromium counterparts
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Other
: P2 Normal
Assignee: Evan Stade
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-02-09 17:57 PST by Evan Stade
Modified: 2009-02-18 14:05 PST (History)
0 users

See Also:


Attachments
patch (3.69 KB, patch)
2009-02-09 17:58 PST, Evan Stade
eric: review-
Details | Formatted Diff | Diff
fix curly brace (3.77 KB, patch)
2009-02-13 15:15 PST, Evan Stade
eric: review-
Details | Formatted Diff | Diff
try3 (4.08 KB, patch)
2009-02-18 12:18 PST, Evan Stade
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Evan Stade 2009-02-09 17:57:44 PST
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 .
Comment 1 Evan Stade 2009-02-09 17:58:29 PST
Created attachment 27511 [details]
patch
Comment 2 Eric Seidel (no email) 2009-02-13 14:24:51 PST
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.
Comment 3 Evan Stade 2009-02-13 14:33:06 PST
(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)
Comment 4 Evan Stade 2009-02-13 15:15:28 PST
Created attachment 27669 [details]
fix curly brace
Comment 5 Eric Seidel (no email) 2009-02-18 12:05:56 PST
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.
Comment 6 Evan Stade 2009-02-18 12:18:40 PST
Created attachment 27760 [details]
try3

changed currentPath() to currentPathInLocalCoordinates(), fixed local variable naming style issues
Comment 7 Eric Seidel (no email) 2009-02-18 12:20:05 PST
Comment on attachment 27760 [details]
try3

r=me
Comment 8 Darin Fisher (:fishd, Google) 2009-02-18 14:05:54 PST
Landed as http://trac.webkit.org/changeset/41064