Factory methods for heap-allocated SkPathEffect and SkXfermode objects.

Factory methods for heap-allocated SkPathEffect and SkXfermode objects.

This is part of an effort to ensure that all SkPaint effects can only be
allocated on the heap.

This patch makes the constructors of SkPathEffect, SkXfermode and
their subclasses non-public and instead provides factory methods for
creating these objects on the heap. We temporarily keep the constructors
of the following classes public to not break Chrome/Blink:

SkXfermode
SkCornerPathEffect
SkDashPathEffect

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

[Dominik Grewe, 2014-02-14 11:05:40]

Following discussion on skia:1770 I've started to introduce factory methods for
SkPaint effects to make sure they're only ever allocated on the heap. I've kept
some constructors public so we don't break Chrome/Blink. After the next Skia
roll we can make them non-public too.
Is there anything else I need to be aware of?

[mtklein, 2014-02-14 22:42:55]

lgtm

A thought for the weekend / next CL:

There's nothing preventing a subclass right now from being allocated on the
stack except convention.  I'd feel even happier if we could think of a way to
make the each base class enforce this down its tree rather than do this by
convention.  Is there an existing idiom for this?

I'm wondering if we can somehow make the base class constructors private, then
create the subclass objects through a factory on the base class.  Something like
this?

template <typename BaseClass>
struct ForceHeapAllocation {
private:
   template <typename SubClass>
   static SubClass* Alloc() {
     SubClass* p = (SubClass*)sk_malloc_throw(sizeof(SubClass));
     SkNEW_PLACEMENT((BaseClass*)p, BaseClass);
     return p;
   }

public:
  template <typename SubClass>
  static SubClass* Create() {
    SubClass* p = Alloc<SubClass>();
    p->init();
    return p;
  }

  template <typename SubClass, typename A1>
  static SubClass* Create(A1 a1) {
    SubClass* p = Alloc<SubClass>();
    p->init(a1);
    return p;
  }

  template <typename SubClass, typename A1, typename A2>
  static SubClass* Create(A1 a1, A2 a2) {
    SubClass* p = Alloc<SubClass>();
    p->init(a1, a2);
    return p;
  }
 
  ... adding Create()s as needed
};

class BaseClass : public SkRefCnt, public ForceHeapAllocation<BaseClass> {
private:
  BaseClass() {}
};

class SomeSubClass : public BaseClass {
public:
  void init(int foo, int bar) {
    fFoo = foo;
    fBar = bar;
  }

private:
  int fFoo, fBar;
};

User calls SomeSubClass* p = BaseClass::Create<SomeSubClass>(3, 4);

[Dominik Grewe, 2014-02-14 22:54:52]

Yeah, if we could enforce that, it would be even better. We'd have to wait until
we've removed all direct calls to constructors in Chrome/Blink though. I'm not
aware of any existing idioms. I'll have a look at what you suggested on Monday.

@reed: Did you mean to leave a comment? Your message is just blank.

[mtklein, 2014-02-14 23:13:08]

On 2014/02/14 22:54:52, Dominik Grewe wrote:
> @reed: Did you mean to leave a comment? Your message is just blank.

That's what you get when just the reviewers were updated.  Pithy.  (Mike added
at least me there.)

[Dominik Grewe, 2014-02-18 11:11:26]

Had to update some uses in sampleapp and animator. PTAL.

[mtklein, 2014-02-18 13:25:40]

On 2014/02/18 11:11:26, Dominik Grewe wrote:
> Had to update some uses in sampleapp and animator. PTAL.

lgtm

[reed1, 2014-02-18 14:06:22]

Do we have any idea what the impact on Chrome will be when we try to land this?
How many files will need to be edited on the chrome side? Should we guard this
with a flag?

https://codereview.chromium.org/166583002/diff/260001/include/core/SkXfermode.h
File include/core/SkXfermode.h (right):

https://codereview.chromium.org/166583002/diff/260001/include/core/SkXfermode...
include/core/SkXfermode.h:252: SkProcXfermode(SkXfermodeProc proc) : fProc(proc)
{}
Should this (all) constructor be protected?

https://codereview.chromium.org/166583002/diff/260001/include/effects/SkCorne...
File include/effects/SkCornerPathEffect.h (right):

https://codereview.chromium.org/166583002/diff/260001/include/effects/SkCorne...
include/effects/SkCornerPathEffect.h:23: SkCornerPathEffect(SkScalar radius);
Should this guy be protected?

https://codereview.chromium.org/166583002/diff/260001/include/effects/SkDashP...
File include/effects/SkDashPathEffect.h (right):

https://codereview.chromium.org/166583002/diff/260001/include/effects/SkDashP...
include/effects/SkDashPathEffect.h:39: SkDashPathEffect(const SkScalar
intervals[], int count, SkScalar phase,
Protected?

[Dominik Grewe, 2014-02-18 14:13:10]

On 2014/02/18 14:06:22, reed1 wrote:
> Do we have any idea what the impact on Chrome will be when we try to land
this?
> How many files will need to be edited on the chrome side? Should we guard this
> with a flag?
> 
>
https://codereview.chromium.org/166583002/diff/260001/include/core/SkXfermode.h
> File include/core/SkXfermode.h (right):
> 
>
https://codereview.chromium.org/166583002/diff/260001/include/core/SkXfermode...
> include/core/SkXfermode.h:252: SkProcXfermode(SkXfermodeProc proc) :
fProc(proc)
> {}
> Should this (all) constructor be protected?
> 
>
https://codereview.chromium.org/166583002/diff/260001/include/effects/SkCorne...
> File include/effects/SkCornerPathEffect.h (right):
> 
>
https://codereview.chromium.org/166583002/diff/260001/include/effects/SkCorne...
> include/effects/SkCornerPathEffect.h:23: SkCornerPathEffect(SkScalar radius);
> Should this guy be protected?
> 
>
https://codereview.chromium.org/166583002/diff/260001/include/effects/SkDashP...
> File include/effects/SkDashPathEffect.h (right):
> 
>
https://codereview.chromium.org/166583002/diff/260001/include/effects/SkDashP...
> include/effects/SkDashPathEffect.h:39: SkDashPathEffect(const SkScalar
> intervals[], int count, SkScalar phase,
> Protected?

I left those constructors public, because they are the ones used by
Chrome/Blink. I'll add a TODO saying they will be removed.

Afaict only three files (two of which are in Blink) need to be changed:
src/skia/ext/platform_canvas.cc
src/third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp
src/third_party/WebKit/Source/platform/graphics/StrokeData.cpp

[reed1, 2014-02-18 14:18:38]

On 2014/02/18 14:13:10, Dominik Grewe wrote:
> On 2014/02/18 14:06:22, reed1 wrote:
> > Do we have any idea what the impact on Chrome will be when we try to land
> this?
> > How many files will need to be edited on the chrome side? Should we guard
this
> > with a flag?
> > 
> >
>
https://codereview.chromium.org/166583002/diff/260001/include/core/SkXfermode.h
> > File include/core/SkXfermode.h (right):
> > 
> >
>
https://codereview.chromium.org/166583002/diff/260001/include/core/SkXfermode...
> > include/core/SkXfermode.h:252: SkProcXfermode(SkXfermodeProc proc) :
> fProc(proc)
> > {}
> > Should this (all) constructor be protected?
> > 
> >
>
https://codereview.chromium.org/166583002/diff/260001/include/effects/SkCorne...
> > File include/effects/SkCornerPathEffect.h (right):
> > 
> >
>
https://codereview.chromium.org/166583002/diff/260001/include/effects/SkCorne...
> > include/effects/SkCornerPathEffect.h:23: SkCornerPathEffect(SkScalar
radius);
> > Should this guy be protected?
> > 
> >
>
https://codereview.chromium.org/166583002/diff/260001/include/effects/SkDashP...
> > File include/effects/SkDashPathEffect.h (right):
> > 
> >
>
https://codereview.chromium.org/166583002/diff/260001/include/effects/SkDashP...
> > include/effects/SkDashPathEffect.h:39: SkDashPathEffect(const SkScalar
> > intervals[], int count, SkScalar phase,
> > Protected?
> 
> I left those constructors public, because they are the ones used by
> Chrome/Blink. I'll add a TODO saying they will be removed.
> 
> Afaict only three files (two of which are in Blink) need to be changed:
> src/skia/ext/platform_canvas.cc
> src/third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp
> src/third_party/WebKit/Source/platform/graphics/StrokeData.cpp

Ah, glad you tested for that.

A common trick Skia uses is to guard the old behavior (public in this case) with
a #ifdef

#ifdef SK_SUPPORT_LEGACY_PUBLICEFFECTCONSTRUCTORS
...
#endif

We *won't* define this in Skia, so we can immediately test the "correct"
behavior. But we will define this in skia_common.gypi for chrome. Then after
we've fixed the call-sites in blink and chrome, we can just remove the define
from the .gypi to "activate" the correct behavior, and then later remove the
flag from skia's sources.

[Dominik Grewe, 2014-02-18 14:18:45]

Added TODOs to annotate public constructors that will be made protected.

[Dominik Grewe, 2014-02-18 14:29:44]

On 2014/02/18 14:18:38, reed1 wrote:
> On 2014/02/18 14:13:10, Dominik Grewe wrote:
> > On 2014/02/18 14:06:22, reed1 wrote:
> > > Do we have any idea what the impact on Chrome will be when we try to land
> > this?
> > > How many files will need to be edited on the chrome side? Should we guard
> this
> > > with a flag?
> > > 
> > >
> >
>
https://codereview.chromium.org/166583002/diff/260001/include/core/SkXfermode.h
> > > File include/core/SkXfermode.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/166583002/diff/260001/include/core/SkXfermode...
> > > include/core/SkXfermode.h:252: SkProcXfermode(SkXfermodeProc proc) :
> > fProc(proc)
> > > {}
> > > Should this (all) constructor be protected?
> > > 
> > >
> >
>
https://codereview.chromium.org/166583002/diff/260001/include/effects/SkCorne...
> > > File include/effects/SkCornerPathEffect.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/166583002/diff/260001/include/effects/SkCorne...
> > > include/effects/SkCornerPathEffect.h:23: SkCornerPathEffect(SkScalar
> radius);
> > > Should this guy be protected?
> > > 
> > >
> >
>
https://codereview.chromium.org/166583002/diff/260001/include/effects/SkDashP...
> > > File include/effects/SkDashPathEffect.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/166583002/diff/260001/include/effects/SkDashP...
> > > include/effects/SkDashPathEffect.h:39: SkDashPathEffect(const SkScalar
> > > intervals[], int count, SkScalar phase,
> > > Protected?
> > 
> > I left those constructors public, because they are the ones used by
> > Chrome/Blink. I'll add a TODO saying they will be removed.
> > 
> > Afaict only three files (two of which are in Blink) need to be changed:
> > src/skia/ext/platform_canvas.cc
> > src/third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp
> > src/third_party/WebKit/Source/platform/graphics/StrokeData.cpp
> 
> Ah, glad you tested for that.
> 
> A common trick Skia uses is to guard the old behavior (public in this case)
with
> a #ifdef
> 
> #ifdef SK_SUPPORT_LEGACY_PUBLICEFFECTCONSTRUCTORS
> ...
> #endif
> 
> We *won't* define this in Skia, so we can immediately test the "correct"
> behavior. But we will define this in skia_common.gypi for chrome. Then after
> we've fixed the call-sites in blink and chrome, we can just remove the define
> from the .gypi to "activate" the correct behavior, and then later remove the
> flag from skia's sources.

I see, cool. I'm wondering what the best way forward is. I'm planning to do
similar changes I've done here for the remaining effects classes (except for the
ones Leon and I working on any way). How about this:

1) add define to skia_common.gypi in Chrome
2) land changes in Skia for all effects (guarding constructors still in use by
Chrome)
3) once all changes have rolled into Skia, change uses in Chrome/Blink and
remove the define from skia_common.gypi
4) remove guards in Skia

Is there anything particular we need to do for changes in Blink? Do they ever
compile with Skia outside of Chrome?

[reed1, 2014-02-18 14:37:10]

On 2014/02/18 14:29:44, Dominik Grewe wrote:
> On 2014/02/18 14:18:38, reed1 wrote:
> > On 2014/02/18 14:13:10, Dominik Grewe wrote:
> > > On 2014/02/18 14:06:22, reed1 wrote:
> > > > Do we have any idea what the impact on Chrome will be when we try to
land
> > > this?
> > > > How many files will need to be edited on the chrome side? Should we
guard
> > this
> > > > with a flag?
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/166583002/diff/260001/include/core/SkXfermode.h
> > > > File include/core/SkXfermode.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/166583002/diff/260001/include/core/SkXfermode...
> > > > include/core/SkXfermode.h:252: SkProcXfermode(SkXfermodeProc proc) :
> > > fProc(proc)
> > > > {}
> > > > Should this (all) constructor be protected?
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/166583002/diff/260001/include/effects/SkCorne...
> > > > File include/effects/SkCornerPathEffect.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/166583002/diff/260001/include/effects/SkCorne...
> > > > include/effects/SkCornerPathEffect.h:23: SkCornerPathEffect(SkScalar
> > radius);
> > > > Should this guy be protected?
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/166583002/diff/260001/include/effects/SkDashP...
> > > > File include/effects/SkDashPathEffect.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/166583002/diff/260001/include/effects/SkDashP...
> > > > include/effects/SkDashPathEffect.h:39: SkDashPathEffect(const SkScalar
> > > > intervals[], int count, SkScalar phase,
> > > > Protected?
> > > 
> > > I left those constructors public, because they are the ones used by
> > > Chrome/Blink. I'll add a TODO saying they will be removed.
> > > 
> > > Afaict only three files (two of which are in Blink) need to be changed:
> > > src/skia/ext/platform_canvas.cc
> > > src/third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp
> > > src/third_party/WebKit/Source/platform/graphics/StrokeData.cpp
> > 
> > Ah, glad you tested for that.
> > 
> > A common trick Skia uses is to guard the old behavior (public in this case)
> with
> > a #ifdef
> > 
> > #ifdef SK_SUPPORT_LEGACY_PUBLICEFFECTCONSTRUCTORS
> > ...
> > #endif
> > 
> > We *won't* define this in Skia, so we can immediately test the "correct"
> > behavior. But we will define this in skia_common.gypi for chrome. Then after
> > we've fixed the call-sites in blink and chrome, we can just remove the
define
> > from the .gypi to "activate" the correct behavior, and then later remove the
> > flag from skia's sources.
> 
> I see, cool. I'm wondering what the best way forward is. I'm planning to do
> similar changes I've done here for the remaining effects classes (except for
the
> ones Leon and I working on any way). How about this:
> 
> 1) add define to skia_common.gypi in Chrome
> 2) land changes in Skia for all effects (guarding constructors still in use by
> Chrome)
> 3) once all changes have rolled into Skia, change uses in Chrome/Blink and
> remove the define from skia_common.gypi
> 4) remove guards in Skia
> 
> Is there anything particular we need to do for changes in Blink? Do they ever
> compile with Skia outside of Chrome?

Don't think so. skia/skia_common.gypi seems sufficient for blink and chrome.

I actually would vote to land these piece-meal in Skia, so we can begin testing
/ living-with them asap. No need (in my eyes) to wait to land them all at once.

[Dominik Grewe, 2014-02-18 14:41:54]

On 2014/02/18 14:37:10, reed1 wrote:
> On 2014/02/18 14:29:44, Dominik Grewe wrote:
> > On 2014/02/18 14:18:38, reed1 wrote:
> > > On 2014/02/18 14:13:10, Dominik Grewe wrote:
> > > > On 2014/02/18 14:06:22, reed1 wrote:
> > > > > Do we have any idea what the impact on Chrome will be when we try to
> land
> > > > this?
> > > > > How many files will need to be edited on the chrome side? Should we
> guard
> > > this
> > > > > with a flag?
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/166583002/diff/260001/include/core/SkXfermode.h
> > > > > File include/core/SkXfermode.h (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/166583002/diff/260001/include/core/SkXfermode...
> > > > > include/core/SkXfermode.h:252: SkProcXfermode(SkXfermodeProc proc) :
> > > > fProc(proc)
> > > > > {}
> > > > > Should this (all) constructor be protected?
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/166583002/diff/260001/include/effects/SkCorne...
> > > > > File include/effects/SkCornerPathEffect.h (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/166583002/diff/260001/include/effects/SkCorne...
> > > > > include/effects/SkCornerPathEffect.h:23: SkCornerPathEffect(SkScalar
> > > radius);
> > > > > Should this guy be protected?
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/166583002/diff/260001/include/effects/SkDashP...
> > > > > File include/effects/SkDashPathEffect.h (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/166583002/diff/260001/include/effects/SkDashP...
> > > > > include/effects/SkDashPathEffect.h:39: SkDashPathEffect(const SkScalar
> > > > > intervals[], int count, SkScalar phase,
> > > > > Protected?
> > > > 
> > > > I left those constructors public, because they are the ones used by
> > > > Chrome/Blink. I'll add a TODO saying they will be removed.
> > > > 
> > > > Afaict only three files (two of which are in Blink) need to be changed:
> > > > src/skia/ext/platform_canvas.cc
> > > > src/third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp
> > > > src/third_party/WebKit/Source/platform/graphics/StrokeData.cpp
> > > 
> > > Ah, glad you tested for that.
> > > 
> > > A common trick Skia uses is to guard the old behavior (public in this
case)
> > with
> > > a #ifdef
> > > 
> > > #ifdef SK_SUPPORT_LEGACY_PUBLICEFFECTCONSTRUCTORS
> > > ...
> > > #endif
> > > 
> > > We *won't* define this in Skia, so we can immediately test the "correct"
> > > behavior. But we will define this in skia_common.gypi for chrome. Then
after
> > > we've fixed the call-sites in blink and chrome, we can just remove the
> define
> > > from the .gypi to "activate" the correct behavior, and then later remove
the
> > > flag from skia's sources.
> > 
> > I see, cool. I'm wondering what the best way forward is. I'm planning to do
> > similar changes I've done here for the remaining effects classes (except for
> the
> > ones Leon and I working on any way). How about this:
> > 
> > 1) add define to skia_common.gypi in Chrome
> > 2) land changes in Skia for all effects (guarding constructors still in use
by
> > Chrome)
> > 3) once all changes have rolled into Skia, change uses in Chrome/Blink and
> > remove the define from skia_common.gypi
> > 4) remove guards in Skia
> > 
> > Is there anything particular we need to do for changes in Blink? Do they
ever
> > compile with Skia outside of Chrome?
> 
> Don't think so. skia/skia_common.gypi seems sufficient for blink and chrome.
> 
> I actually would vote to land these piece-meal in Skia, so we can begin
testing
> / living-with them asap. No need (in my eyes) to wait to land them all at
once.

Yeah, agreed. Step 2 should be done in multiple patches (like this one). But we
can land all changes to Skia before updating Chrome.

[reed1, 2014-02-18 15:20:07]

On 2014/02/18 14:41:54, Dominik Grewe wrote:
> On 2014/02/18 14:37:10, reed1 wrote:
> > On 2014/02/18 14:29:44, Dominik Grewe wrote:
> > > On 2014/02/18 14:18:38, reed1 wrote:
> > > > On 2014/02/18 14:13:10, Dominik Grewe wrote:
> > > > > On 2014/02/18 14:06:22, reed1 wrote:
> > > > > > Do we have any idea what the impact on Chrome will be when we try to
> > land
> > > > > this?
> > > > > > How many files will need to be edited on the chrome side? Should we
> > guard
> > > > this
> > > > > > with a flag?
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/166583002/diff/260001/include/core/SkXfermode.h
> > > > > > File include/core/SkXfermode.h (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/166583002/diff/260001/include/core/SkXfermode...
> > > > > > include/core/SkXfermode.h:252: SkProcXfermode(SkXfermodeProc proc) :
> > > > > fProc(proc)
> > > > > > {}
> > > > > > Should this (all) constructor be protected?
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/166583002/diff/260001/include/effects/SkCorne...
> > > > > > File include/effects/SkCornerPathEffect.h (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/166583002/diff/260001/include/effects/SkCorne...
> > > > > > include/effects/SkCornerPathEffect.h:23: SkCornerPathEffect(SkScalar
> > > > radius);
> > > > > > Should this guy be protected?
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/166583002/diff/260001/include/effects/SkDashP...
> > > > > > File include/effects/SkDashPathEffect.h (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/166583002/diff/260001/include/effects/SkDashP...
> > > > > > include/effects/SkDashPathEffect.h:39: SkDashPathEffect(const
SkScalar
> > > > > > intervals[], int count, SkScalar phase,
> > > > > > Protected?
> > > > > 
> > > > > I left those constructors public, because they are the ones used by
> > > > > Chrome/Blink. I'll add a TODO saying they will be removed.
> > > > > 
> > > > > Afaict only three files (two of which are in Blink) need to be
changed:
> > > > > src/skia/ext/platform_canvas.cc
> > > > > src/third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp
> > > > > src/third_party/WebKit/Source/platform/graphics/StrokeData.cpp
> > > > 
> > > > Ah, glad you tested for that.
> > > > 
> > > > A common trick Skia uses is to guard the old behavior (public in this
> case)
> > > with
> > > > a #ifdef
> > > > 
> > > > #ifdef SK_SUPPORT_LEGACY_PUBLICEFFECTCONSTRUCTORS
> > > > ...
> > > > #endif
> > > > 
> > > > We *won't* define this in Skia, so we can immediately test the "correct"
> > > > behavior. But we will define this in skia_common.gypi for chrome. Then
> after
> > > > we've fixed the call-sites in blink and chrome, we can just remove the
> > define
> > > > from the .gypi to "activate" the correct behavior, and then later remove
> the
> > > > flag from skia's sources.
> > > 
> > > I see, cool. I'm wondering what the best way forward is. I'm planning to
do
> > > similar changes I've done here for the remaining effects classes (except
for
> > the
> > > ones Leon and I working on any way). How about this:
> > > 
> > > 1) add define to skia_common.gypi in Chrome
> > > 2) land changes in Skia for all effects (guarding constructors still in
use
> by
> > > Chrome)
> > > 3) once all changes have rolled into Skia, change uses in Chrome/Blink and
> > > remove the define from skia_common.gypi
> > > 4) remove guards in Skia
> > > 
> > > Is there anything particular we need to do for changes in Blink? Do they
> ever
> > > compile with Skia outside of Chrome?
> > 
> > Don't think so. skia/skia_common.gypi seems sufficient for blink and chrome.
> > 
> > I actually would vote to land these piece-meal in Skia, so we can begin
> testing
> > / living-with them asap. No need (in my eyes) to wait to land them all at
> once.
> 
> Yeah, agreed. Step 2 should be done in multiple patches (like this one). But
we
> can land all changes to Skia before updating Chrome.

Yes, assuming we're always guarding the changed constructor visibility.

[Dominik Grewe, 2014-02-18 16:46:21]

I've added the guards for the constructors. It uncovered a few more places
within Skia that were still using them (I hadn't compiled all the targets when
testing), so it's already proven useful :)

Will upload a patch to Chrome now that adds the define to skia_common.gypi.

[scroggo, 2014-02-18 16:53:11]

https://codereview.chromium.org/166583002/diff/490001/include/core/SkPathEffe...
File include/core/SkPathEffect.h (right):

https://codereview.chromium.org/166583002/diff/490001/include/core/SkPathEffe...
include/core/SkPathEffect.h:110: SkPathEffect() {}
I guess the constructors that aren't public behind the flag aren't used by
Chrome?

https://codereview.chromium.org/166583002/diff/490001/include/effects/SkCorne...
File include/effects/SkCornerPathEffect.h (right):

https://codereview.chromium.org/166583002/diff/490001/include/effects/SkCorne...
include/effects/SkCornerPathEffect.h:37: /** radius must be > 0 to have an
effect. It specifies the distance from each corner
Can you put this comment by the new factory?

https://codereview.chromium.org/166583002/diff/490001/include/effects/SkDashP...
File include/effects/SkDashPathEffect.h (right):

https://codereview.chromium.org/166583002/diff/490001/include/effects/SkDashP...
include/effects/SkDashPathEffect.h:43: /** intervals: array containing an even
number of entries (>=2), with
Again, I think this comment belongs with the new factory.

[Dominik Grewe, 2014-02-18 17:05:29]

https://codereview.chromium.org/166583002/diff/490001/include/core/SkPathEffe...
File include/core/SkPathEffect.h (right):

https://codereview.chromium.org/166583002/diff/490001/include/core/SkPathEffe...
include/core/SkPathEffect.h:110: SkPathEffect() {}
On 2014/02/18 16:53:11, scroggo wrote:
> I guess the constructors that aren't public behind the flag aren't used by
> Chrome?

Yes. I've patched this into Chrome and it's working on Linux. Not sure about
other platforms though. Might be safer to add it everywhere...

https://codereview.chromium.org/166583002/diff/490001/include/effects/SkCorne...
File include/effects/SkCornerPathEffect.h (right):

https://codereview.chromium.org/166583002/diff/490001/include/effects/SkCorne...
include/effects/SkCornerPathEffect.h:37: /** radius must be > 0 to have an
effect. It specifies the distance from each corner
On 2014/02/18 16:53:11, scroggo wrote:
> Can you put this comment by the new factory?

Good point! Done.

https://codereview.chromium.org/166583002/diff/490001/include/effects/SkDashP...
File include/effects/SkDashPathEffect.h (right):

https://codereview.chromium.org/166583002/diff/490001/include/effects/SkDashP...
include/effects/SkDashPathEffect.h:43: /** intervals: array containing an even
number of entries (>=2), with
On 2014/02/18 16:53:11, scroggo wrote:
> Again, I think this comment belongs with the new factory.

Done.

[scroggo, 2014-02-19 00:37:12]

As I commented in line, some of the formerly public constructors are still used
by Android. Maybe that's okay, since we (and by "we" I really mean Derek -
cc'ing him so he can have a vote on whether to keep public constructors behind a
flag for Android merging) merge to Android manually instead of doing a DEPS
roll, so we can make the change all at once.

https://codereview.chromium.org/166583002/diff/600002/include/effects/Sk1DPat...
File include/effects/Sk1DPathEffect.h (right):

https://codereview.chromium.org/166583002/diff/600002/include/effects/Sk1DPat...
include/effects/Sk1DPathEffect.h:66: SkPath1DPathEffect(const SkPath& path,
SkScalar advance, SkScalar phase, Style);
This constructor is used by Android. It would be helpful for the merge if it was
behind the flag as well.

https://codereview.chromium.org/166583002/diff/600002/include/effects/SkAvoid...
File include/effects/SkAvoidXfermode.h (right):

https://codereview.chromium.org/166583002/diff/600002/include/effects/SkAvoid...
include/effects/SkAvoidXfermode.h:56: SkAvoidXfermode(SkColor opColor, U8CPU
tolerance, Mode mode);
Also used by Android.

[scroggo, 2014-02-19 00:37:26]

On 2014/02/19 00:37:12, scroggo wrote:
> As I commented in line, some of the formerly public constructors are still
used
> by Android. Maybe that's okay, since we (and by "we" I really mean Derek -
> cc'ing him so he can have a vote on whether to keep public constructors behind
a
> flag for Android merging) merge to Android manually instead of doing a DEPS
> roll, so we can make the change all at once.
> 
>
https://codereview.chromium.org/166583002/diff/600002/include/effects/Sk1DPat...
> File include/effects/Sk1DPathEffect.h (right):
> 
>
https://codereview.chromium.org/166583002/diff/600002/include/effects/Sk1DPat...
> include/effects/Sk1DPathEffect.h:66: SkPath1DPathEffect(const SkPath& path,
> SkScalar advance, SkScalar phase, Style);
> This constructor is used by Android. It would be helpful for the merge if it
was
> behind the flag as well.
> 
>
https://codereview.chromium.org/166583002/diff/600002/include/effects/SkAvoid...
> File include/effects/SkAvoidXfermode.h (right):
> 
>
https://codereview.chromium.org/166583002/diff/600002/include/effects/SkAvoid...
> include/effects/SkAvoidXfermode.h:56: SkAvoidXfermode(SkColor opColor, U8CPU
> tolerance, Mode mode);
> Also used by Android.

But in general, lgtm

[Dominik Grewe, 2014-02-19 11:31:24]

Just to avoid any surprises I've guarded all constructors in publicly visible
classes.

[reed1, 2014-02-19 13:15:08]

lgtm

[Dominik Grewe, 2014-02-19 16:20:53]

The CQ bit was checked by dominikg@chromium.org

[commit-bot: I haz the power, 2014-02-19 16:20:59]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/dominikg@chromium.org/166583002/750001

[Dominik Grewe, 2014-02-20 10:49:22]

The CQ bit was checked by dominikg@chromium.org

[commit-bot: I haz the power, 2014-02-20 10:49:30]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/dominikg@chromium.org/166583002/860001

[commit-bot: I haz the power, 2014-02-20 20:40:31]

Change committed as 13519