Sky: Add a DrawLooper interface to the painting API to be used for shadows.

Sky: Add a DrawLooper interface to the painting API to be used for shadows.

R=abarth@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/3d8934bb8f51a29f997455dae582984306de40ab

[Matt Perry, 2015-05-26 21:20:14]

mpcomplete@chromium.org changed reviewers:
+ abarth@chromium.org

[Matt Perry, 2015-05-26 21:20:15]

Check out painting.sky first to see how the API is used.

[abarth-chromium, 2015-05-27 02:12:51]

https://codereview.chromium.org/1144193006/diff/40001/sky/engine/core/paintin...
File sky/engine/core/painting/DrawLooperLayerInfo.idl (right):

https://codereview.chromium.org/1144193006/diff/40001/sky/engine/core/paintin...
sky/engine/core/painting/DrawLooperLayerInfo.idl:10: // kEntirePaint_Bits means
use the layer's paint completely.
s/kEntirePaint_Bits/ENTIRE_PAINT_BITS/

https://codereview.chromium.org/1144193006/diff/40001/sky/engine/core/paintin...
File sky/engine/core/painting/Paint.h (right):

https://codereview.chromium.org/1144193006/diff/40001/sky/engine/core/paintin...
sky/engine/core/painting/Paint.h:28: }
How does this work?  It seems like the Paint object can live for an arbitrary
period of time but the SkPaint object passed in here will die at some point. 
After the SkPaint dies, won't the Paint object point to unallocated memory?

[Matt Perry, 2015-05-27 14:46:32]

https://codereview.chromium.org/1144193006/diff/40001/sky/engine/core/paintin...
File sky/engine/core/painting/DrawLooperLayerInfo.idl (right):

https://codereview.chromium.org/1144193006/diff/40001/sky/engine/core/paintin...
sky/engine/core/painting/DrawLooperLayerInfo.idl:10: // kEntirePaint_Bits means
use the layer's paint completely.
On 2015/05/27 02:12:51, abarth wrote:
> s/kEntirePaint_Bits/ENTIRE_PAINT_BITS/

Done.

https://codereview.chromium.org/1144193006/diff/40001/sky/engine/core/paintin...
File sky/engine/core/painting/Paint.h (right):

https://codereview.chromium.org/1144193006/diff/40001/sky/engine/core/paintin...
sky/engine/core/painting/Paint.h:28: }
On 2015/05/27 02:12:51, abarth wrote:
> How does this work?  It seems like the Paint object can live for an arbitrary
> period of time but the SkPaint object passed in here will die at some point. 
> After the SkPaint dies, won't the Paint object point to unallocated memory?

Yeah, I'm using it with SkLayerDrawLooper::Builder::addLayerOnTop. The
documentation for this method says it "returns a paint you can modify, but that
ptr is only valid until the next call made to addLayer()."

The way I'm handling this lifetime issue is by handing this Paint object to a
callback, and only guaranteeing that it's valid while the callback runs. (See
LayerDrawLooperBuilder::addLayerOnTop).

[abarth-chromium, 2015-05-27 15:46:39]

On 2015/05/27 at 14:46:32, mpcomplete wrote:
> Yeah, I'm using it with SkLayerDrawLooper::Builder::addLayerOnTop. The
documentation for this method says it "returns a paint you can modify, but that
ptr is only valid until the next call made to addLayer()."
> 
> The way I'm handling this lifetime issue is by handing this Paint object to a
callback, and only guaranteeing that it's valid while the callback runs. (See
LayerDrawLooperBuilder::addLayerOnTop).

We'll need some sort of mechanism for preventing Dart code from stomping over
unallocated memory.  Maybe we need to subclass Paint and add a mechanism for
nulling out the pointer when it stops being valid.

[Matt Perry, 2015-05-27 15:57:30]

On 2015/05/27 15:46:39, abarth wrote:
> On 2015/05/27 at 14:46:32, mpcomplete wrote:
> > Yeah, I'm using it with SkLayerDrawLooper::Builder::addLayerOnTop. The
> documentation for this method says it "returns a paint you can modify, but
that
> ptr is only valid until the next call made to addLayer()."
> > 
> > The way I'm handling this lifetime issue is by handing this Paint object to
a
> callback, and only guaranteeing that it's valid while the callback runs. (See
> LayerDrawLooperBuilder::addLayerOnTop).
> 
> We'll need some sort of mechanism for preventing Dart code from stomping over
> unallocated memory.  Maybe we need to subclass Paint and add a mechanism for
> nulling out the pointer when it stops being valid.

I could add a method for Paint to revert to pointing to its m_inline_paint
member, and call that when the outer pointer becomes invalid.

[Matt Perry, 2015-05-27 16:01:32]

On 2015/05/27 15:57:30, Matt Perry wrote:
> On 2015/05/27 15:46:39, abarth wrote:
> > On 2015/05/27 at 14:46:32, mpcomplete wrote:
> > > Yeah, I'm using it with SkLayerDrawLooper::Builder::addLayerOnTop. The
> > documentation for this method says it "returns a paint you can modify, but
> that
> > ptr is only valid until the next call made to addLayer()."
> > > 
> > > The way I'm handling this lifetime issue is by handing this Paint object
to
> a
> > callback, and only guaranteeing that it's valid while the callback runs.
(See
> > LayerDrawLooperBuilder::addLayerOnTop).
> > 
> > We'll need some sort of mechanism for preventing Dart code from stomping
over
> > unallocated memory.  Maybe we need to subclass Paint and add a mechanism for
> > nulling out the pointer when it stops being valid.
> 
> I could add a method for Paint to revert to pointing to its m_inline_paint
> member, and call that when the outer pointer becomes invalid.

See:
https://codereview.chromium.org/1144193006/diff2/40001:60001/sky/engine/core/...

[abarth-chromium, 2015-05-27 16:10:19]

Yeah, that's better.  Is there a way we can do this without making Paint more
complicated?  It's unfortunate to make Paint have an extra pointer and machinery
just for this obscure case...

[Matt Perry, 2015-05-27 18:09:02]

On 2015/05/27 16:10:19, abarth wrote:
> Yeah, that's better.  Is there a way we can do this without making Paint more
> complicated?  It's unfortunate to make Paint have an extra pointer and
machinery
> just for this obscure case...

One option is to just copy the SkPaint object before/after passing it to the
callback. e.g.

   SkPaint* skPaint = m_drawLooperBuilder.addLayerOnTop(layerInfo->layerInfo());
   RefPtr<Paint> paint = Paint::create();
   paint->set_paint(*skPaint);
   callback->handleEvent(paint.get());
   *skPaint = paint->paint();

My preference is to keep the weak ref method. WDYT?

[abarth-chromium, 2015-05-27 18:09:58]

IMHO, copying the paint is a bit better.  If this pattern comes up again, we
might want to make Paint more complex...

[Matt Perry, 2015-05-27 18:21:19]

On 2015/05/27 18:09:58, abarth wrote:
> IMHO, copying the paint is a bit better.  If this pattern comes up again, we
> might want to make Paint more complex...

OK, went with copy. I'm going to switch the C++ style over to chromium for these
files, since they're new.

[abarth-chromium, 2015-05-27 20:17:44]

lgtm

https://codereview.chromium.org/1144193006/diff/120001/sky/engine/core/painti...
File sky/engine/core/painting/Paint.h (right):

https://codereview.chromium.org/1144193006/diff/120001/sky/engine/core/painti...
sky/engine/core/painting/Paint.h:39: void set_paint(const SkPaint& paint) {
m_paint = paint; }
set_paint => setPaint ?

[Matt Perry, 2015-05-27 20:22:05]

Committed patchset #8 (id:140001) manually as
3d8934bb8f51a29f997455dae582984306de40ab (presubmit successful).