Fix underlying issue causing crbug.com/348821.

Fix underlying issue causing crbug.com/348821.

SkDashPathEffect refuses to flatten itself if it thinks it's a no-op.  This
messes with the assumption that flattening a non-NULL effect and then
unflattening from those bytes produces a non-NULL effect.  I'd rather make that
assumption always be true than save a few bytes for rare nonsense path effects.

BUG=348821
Committed: http://code.google.com/p/skia/source/detail?r=13701

[reed1, 2014-03-07 13:49:17]

lgtm

lets write a unittest (now or later) to exercise this (i.e. create a wacky dash
and try to flatten/unflatten it)

[mtklein, 2014-03-07 13:55:47]

On 2014/03/07 13:49:17, reed1 wrote:
> lgtm
> 
> lets write a unittest (now or later) to exercise this (i.e. create a wacky
dash
> and try to flatten/unflatten it)

Working on that right now.  Will tack the test onto this before submitting.

It's a weird one though, in that it's really unlikely to ever be a helpful test.
 It's a specific type of regression and I haven't yet thought of a good way to
test this constraint holds in general.

[reed1, 2014-03-07 14:00:50]

On 2014/03/07 13:55:47, mtklein wrote:
> On 2014/03/07 13:49:17, reed1 wrote:
> > lgtm
> > 
> > lets write a unittest (now or later) to exercise this (i.e. create a wacky
> dash
> > and try to flatten/unflatten it)
> 
> Working on that right now.  Will tack the test onto this before submitting.
> 
> It's a weird one though, in that it's really unlikely to ever be a helpful
test.
>  It's a specific type of regression and I haven't yet thought of a good way to
> test this constraint holds in general.

I'd be ok if we throw away specific regression tests over time (like this one).

Seems like one "invariant" we've stumbled over is the desire for getFactory to
*always* return its proc...? Perhaps we could (not today) change flattenable to
have getFactory be non-virtual, which access the virtual onGetFactory. Then
getFactory could assert that it never got null... Maybe overkill, but would have
no runtime cost, as getFactory could be inlined.

[mtklein, 2014-03-07 14:19:43]

On 2014/03/07 14:00:50, reed1 wrote:
> On 2014/03/07 13:55:47, mtklein wrote:
> > On 2014/03/07 13:49:17, reed1 wrote:
> > > lgtm
> > > 
> > > lets write a unittest (now or later) to exercise this (i.e. create a wacky
> > dash
> > > and try to flatten/unflatten it)
> > 
> > Working on that right now.  Will tack the test onto this before submitting.
> > 
> > It's a weird one though, in that it's really unlikely to ever be a helpful
> test.
> >  It's a specific type of regression and I haven't yet thought of a good way
to
> > test this constraint holds in general.
> 
> I'd be ok if we throw away specific regression tests over time (like this
one).
> 
> Seems like one "invariant" we've stumbled over is the desire for getFactory to
> *always* return its proc...? Perhaps we could (not today) change flattenable
to
> have getFactory be non-virtual, which access the virtual onGetFactory. Then
> getFactory could assert that it never got null... Maybe overkill, but would
have
> no runtime cost, as getFactory could be inlined.

Right, I think that's the invariant I want.

How does https://codereview.chromium.org/190843002/ look to you?

[mtklein, 2014-03-07 14:22:15]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-03-07 14:24:57]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/mtklein@chromium.org/189643002/1

[commit-bot: I haz the power, 2014-03-07 14:43:33]

Change committed as 13701