Move fLastMoveToIndex from SkPath to SkPathRef

The old omnibus CL (https://codereview.chromium.org/25787002/) has gotten too far out of date. This calves off just the "fLastMoveToIndex" portion.

[bsalomon, 2014-01-24 18:04:58]

This one doesn't seem as obvious to me as the previous changes. I guess I'm not
sure what the benefit is to moving this into pathref.

[robertphillips, 2014-01-30 21:00:53]

Here is a version that allows for code reuse in the relative calls but adds an
explicit injectMoveToIfNeeded to the SkPathRef editor API.

I agree that this change isn't as motivating as the others. The requirement to
inject extra moveTos does depend on the geometric data (so it makes some sense
to put it with the data in that regard). At least in the current version, having
SkPath communicate the information that it is about to add an op possibly
requiring a moveTo (via injectMoveToIfNeeded) is a bit micro-managing (thus
arguing to keep this in SkPath). From a pragmatic perspective moving the
fLastMoveToIndex into SkPathRef does speed up the SkPath copying:

Original vs. Stateful path renderer
    - stateful path is slower (esp. on maskshader and maskopaque)

maskshader GPU c     876589.00  893711.25  -17122.25    -2.0%
maskopaque GPU c     640912.50  648814.00   -7901.50    -1.2%
maskblack GPU c     647188.00  651615.50   -4427.50    -0.7%
maskcolor GPU c     640432.50  642722.50   -2290.00    -0.4%

Stateful path renderer vs. Stateful path renderer with reduced path copy
    - having reduced path copy is faster (esp. on maskshader)

maskopaque GPU c     648814.00  644496.50   +4317.50    +0.7%
maskcolor GPU c     642722.50  637353.75   +5368.75    +0.8%
maskblack GPU c     651615.50  645441.50   +6174.00    +0.9%
maskshader GPU c     893711.25  873311.50  +20399.75    +2.3%

Original vs. Stateful path renderer with reduced path copy
    - stateful path renderer with reduced path copy is ~same speed as original

maskopaque GPU c     640912.50  644496.50   -3584.00    -0.6%
maskblack GPU c     647188.00  645441.50   +1746.50    +0.3%
maskshader GPU c     876589.00  873311.50   +3277.50    +0.4%
maskcolor GPU c     640432.50  637353.75   +3078.75    +0.5%

[reed1, 2014-01-30 21:28:25]

On 2014/01/30 21:00:53, robertphillips wrote:
> Here is a version that allows for code reuse in the relative calls but adds an
> explicit injectMoveToIfNeeded to the SkPathRef editor API.
> 
> I agree that this change isn't as motivating as the others. The requirement to
> inject extra moveTos does depend on the geometric data (so it makes some sense
> to put it with the data in that regard). At least in the current version,
having
> SkPath communicate the information that it is about to add an op possibly
> requiring a moveTo (via injectMoveToIfNeeded) is a bit micro-managing (thus
> arguing to keep this in SkPath). From a pragmatic perspective moving the
> fLastMoveToIndex into SkPathRef does speed up the SkPath copying:
> 
> Original vs. Stateful path renderer
>     - stateful path is slower (esp. on maskshader and maskopaque)
> 
> maskshader GPU c     876589.00  893711.25  -17122.25    -2.0%
> maskopaque GPU c     640912.50  648814.00   -7901.50    -1.2%
> maskblack GPU c     647188.00  651615.50   -4427.50    -0.7%
> maskcolor GPU c     640432.50  642722.50   -2290.00    -0.4%
> 
> Stateful path renderer vs. Stateful path renderer with reduced path copy
>     - having reduced path copy is faster (esp. on maskshader)
> 
> maskopaque GPU c     648814.00  644496.50   +4317.50    +0.7%
> maskcolor GPU c     642722.50  637353.75   +5368.75    +0.8%
> maskblack GPU c     651615.50  645441.50   +6174.00    +0.9%
> maskshader GPU c     893711.25  873311.50  +20399.75    +2.3%
> 
> Original vs. Stateful path renderer with reduced path copy
>     - stateful path renderer with reduced path copy is ~same speed as original
> 
> maskopaque GPU c     640912.50  644496.50   -3584.00    -0.6%
> maskblack GPU c     647188.00  645441.50   +1746.50    +0.3%
> maskshader GPU c     876589.00  873311.50   +3277.50    +0.4%
> maskcolor GPU c     640432.50  637353.75   +3078.75    +0.5%

Should we remove the relative calls, and force the clients to call the
getLastPt... or also remove getLast as well?

[robertphillips, 2014-01-31 15:35:09]

I think the real problem is the insertion of moveTos if the user neglects to add
them. If we were to remove the relative calls the caller would have a painful
time replicating the auto-insertion functionality (they would have to track the
last moveTo externally just as we do internally) - provided they wanted to mimic
the current functionality. 

As a sidebar, Android documents this behavior as:

    public void lineTo (float x, float y)

    Added in API level 1
    Add a line from the last point to the specified point (x,y). If no moveTo()
call has been 
    made for this contour, the first point is automatically set to (0,0).

So I don't know what they're doing with our insertion of the last moveTo
location after a close.

[robertphillips, 2014-02-07 15:25:53]

Here is the CL that was the genesis for the current moveTo insertion logic:
https://code.google.com/p/skia/source/detail?r=3027

[Stephen Chennney, 2014-02-07 15:56:37]

On 2014/01/31 15:35:09, robertphillips wrote:
> I think the real problem is the insertion of moveTos if the user neglects to
add
> them. If we were to remove the relative calls the caller would have a painful
> time replicating the auto-insertion functionality (they would have to track
the
> last moveTo externally just as we do internally) - provided they wanted to
mimic
> the current functionality. 
> 
> As a sidebar, Android documents this behavior as:
> 
>     public void lineTo (float x, float y)
> 
>     Added in API level 1
>     Add a line from the last point to the specified point (x,y). If no
moveTo()
> call has been 
>     made for this contour, the first point is automatically set to (0,0).
> 
> So I don't know what they're doing with our insertion of the last moveTo
> location after a close.

I certainly wouldn't want to track this in Blink's path code. For performance
reasons we don't want to have to maintain any extra information before feeding
it off to Skia. In particular, we can't modify the path data we hold in Blink
because we need to be able to report it back to Javascript.

[robertphillips, 2014-02-07 16:01:43]

The current debate is what do users expect to happen in the case of:
moveTo(10,10)|lineTo1|close|lineTo2. Android seems to expect a moveTo(0, 0) to
be inserted before lineTo2 while we currently insert a moveTo(10,10).

[reed1, 2014-02-07 18:23:09]

lgtm

[robertphillips, 2014-02-10 13:51:51]

committed as r13379

[robertphillips, 2014-02-12 19:16:11]

This was reverted in r13421 (Revert of r13379 (Move fLastMoveToIndex from SkPath
to SkPathRef)  - https://codereview.chromium.org/137863006/) due to an image
quality regression in Chromium (crbug.com/343123 Regression - UI issue observed
for any "Notification infobars" in Chrome browser)