Flesh out the Painting API a bit.

Flesh out the Painting API a bit.

This exposes most methods from Skia's C canvas API to Dart. For now, SkRect and
SkMatrix are represented simply as an array of floats, which requires a
conversion at the bindings layer. More complex types like SkPath are still TODO.

[Matt Perry, 2015-05-13 19:39:16]

mpcomplete@chromium.org changed reviewers:
+ eseidel@chromium.org, reed@chromium.org

[Matt Perry, 2015-05-13 19:39:16]

Here's a first step at the Painting API.

Note to Eric: feel free to land your Canvas patch first and I can rebase to
that.

[eseidel, 2015-05-13 20:18:56]

Landed.  This generally looks fine.  I suspect that we're going to want to
create a real Darty interface in front of this, with a Rect class, etc.

[robertphillips, 2015-05-13 20:22:05]

robertphillips@google.com changed reviewers:
+ robertphillips@google.com

[robertphillips, 2015-05-13 20:22:06]

https://codereview.chromium.org/1144483002/diff/1/sky/engine/core/painting/Pa...
File sky/engine/core/painting/PaintingContext.idl (right):

https://codereview.chromium.org/1144483002/diff/1/sky/engine/core/painting/Pa...
sky/engine/core/painting/PaintingContext.idl:15: void scale(float sx, float sy);
rotateDegrees ? to leave room for a rotateRadians ?

https://codereview.chromium.org/1144483002/diff/1/sky/engine/core/painting/Pa...
sky/engine/core/painting/PaintingContext.idl:24: void drawOval(float[] rect4,
Paint paint);
What about something like: drawRoundRectXY(float[] rect4, float xRad, float
yRad, Paint paint);
?

[Matt Perry, 2015-05-13 20:23:12]

Rebased. Also s/float/double on Canvas

[Matt Perry, 2015-05-13 20:23:32]

On 2015/05/13 20:23:12, Matt Perry wrote:
> Rebased. Also s/float/double on Canvas

Er s/double/float

[eseidel, 2015-05-13 20:24:36]

eseidel@chromium.org changed reviewers:
- robertphillips@google.com

[eseidel, 2015-05-13 20:24:38]

This looks great.  lgtm

https://codereview.chromium.org/1144483002/diff/10005/sky/engine/core/paintin...
File sky/engine/core/painting/Canvas.cpp (right):

https://codereview.chromium.org/1144483002/diff/10005/sky/engine/core/paintin...
sky/engine/core/painting/Canvas.cpp:49: if (bounds.size() != 0)
isEmpty()

https://codereview.chromium.org/1144483002/diff/10005/sky/engine/core/paintin...
sky/engine/core/painting/Canvas.cpp:51: m_canvas->saveLayer(bounds.size() != 0 ?
&sk_bounds : nullptr,
isEmpty() again.

https://codereview.chromium.org/1144483002/diff/10005/sky/engine/core/paintin...
File sky/engine/core/painting/Canvas.idl (right):

https://codereview.chromium.org/1144483002/diff/10005/sky/engine/core/paintin...
sky/engine/core/painting/Canvas.idl:27: // TODO(mpcomplete): float
Feel free to change it.  TBR=me.

[Matt Perry, 2015-05-13 20:32:44]

I'll work on a Dart-ier wrapper next.

https://codereview.chromium.org/1144483002/diff/1/sky/engine/core/painting/Pa...
File sky/engine/core/painting/PaintingContext.idl (right):

https://codereview.chromium.org/1144483002/diff/1/sky/engine/core/painting/Pa...
sky/engine/core/painting/PaintingContext.idl:15: void scale(float sx, float sy);
On 2015/05/13 20:22:06, robertphillips wrote:
> rotateDegrees ? to leave room for a rotateRadians ?

Done.

https://codereview.chromium.org/1144483002/diff/1/sky/engine/core/painting/Pa...
sky/engine/core/painting/PaintingContext.idl:24: void drawOval(float[] rect4,
Paint paint);
On 2015/05/13 20:22:06, robertphillips wrote:
> What about something like: drawRoundRectXY(float[] rect4, float xRad, float
> yRad, Paint paint);
> ?

Sure. I didn't add all the methods from skia right away, but will add more as I
go.

[Matt Perry, 2015-05-13 20:32:59]

The CQ bit was checked by mpcomplete@chromium.org

[commit-bot: I haz the power, 2015-05-13 20:34:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-13 20:34:04]

Commit queue rejected this change because it did not recognize the base URL.
Please commit your change manually.

[reed1, 2015-05-13 20:35:03]

reed@google.com changed reviewers:
+ reed@google.com

[reed1, 2015-05-13 20:35:03]

https://codereview.chromium.org/1144483002/diff/1/sky/engine/core/painting/Pa...
File sky/engine/core/painting/PaintingContext.idl (right):

https://codereview.chromium.org/1144483002/diff/1/sky/engine/core/painting/Pa...
sky/engine/core/painting/PaintingContext.idl:26: void drawCircle(double x,
double y, double radius, Paint paint);
FWIW : SkScalar is currently float, but we are building up the capability to
optionally switch it to double, but that will be a compile option, not a
requirement.

That said, I agree that this header should be consistent, using all floats or
all doubles.

[reed1, 2015-05-13 20:38:58]

comments for future CLs...

1. Matrix almost certainly should be a class, so we can hide/play-games with how
the rows/cols are stored. Also operations on matrices are tricky, and need not
be rediscovered by each user.

2. Rect would be nicer as a class too, partly just so its bloody clear what the
4 fields mean.

3. Possibly we should expose color components as 0...1 floats instead of 0..255
bytes, but we can discuss that later.

https://codereview.chromium.org/1144483002/diff/30001/sky/engine/core/paintin...
File sky/engine/core/painting/Canvas.h (right):

https://codereview.chromium.org/1144483002/diff/30001/sky/engine/core/paintin...
sky/engine/core/painting/Canvas.h:25: float width() const { return
m_size.width(); }
Do we really need to expose width/height to the clients?

Another way to allow "smart" clients to cull themselves is to pass a rect (which
they could ignore) to their paint method:

void Node::paint(Canvas*, const Rect* dirtyArea) { ... }

https://codereview.chromium.org/1144483002/diff/30001/sky/engine/core/paintin...
sky/engine/core/painting/Canvas.h:41: void drawRect(Vector<float> rect, Paint*
paint);
const Paint& ?

[Matt Perry, 2015-05-13 21:02:05]

Thanks for the feedback. I agree on the class wrappers for Matrix/Rect - I'll
work on those next. I have no opinion on the color values.

https://codereview.chromium.org/1144483002/diff/30001/sky/engine/core/paintin...
File sky/engine/core/painting/Canvas.h (right):

https://codereview.chromium.org/1144483002/diff/30001/sky/engine/core/paintin...
sky/engine/core/painting/Canvas.h:25: float width() const { return
m_size.width(); }
On 2015/05/13 20:38:58, reed1 wrote:
> Do we really need to expose width/height to the clients?
> 
> Another way to allow "smart" clients to cull themselves is to pass a rect
(which
> they could ignore) to their paint method:
> 
> void Node::paint(Canvas*, const Rect* dirtyArea) { ... }

I'm not sure - I think Ian is changing the way we're going to initiate a paint,
so I'll hold off on changing this for now.

https://codereview.chromium.org/1144483002/diff/30001/sky/engine/core/paintin...
sky/engine/core/painting/Canvas.h:41: void drawRect(Vector<float> rect, Paint*
paint);
On 2015/05/13 20:38:57, reed1 wrote:
> const Paint& ?

I'm at the mercy of the IDL compiler as to how this interface looks. We could
change the compiler, but for now I'll leave it as a pointer (though I added
const).

[reed1, 2015-05-13 21:17:29]

On 2015/05/13 21:02:05, Matt Perry wrote:
> Thanks for the feedback. I agree on the class wrappers for Matrix/Rect - I'll
> work on those next. I have no opinion on the color values.
> 
>
https://codereview.chromium.org/1144483002/diff/30001/sky/engine/core/paintin...
> File sky/engine/core/painting/Canvas.h (right):
> 
>
https://codereview.chromium.org/1144483002/diff/30001/sky/engine/core/paintin...
> sky/engine/core/painting/Canvas.h:25: float width() const { return
> m_size.width(); }
> On 2015/05/13 20:38:58, reed1 wrote:
> > Do we really need to expose width/height to the clients?
> > 
> > Another way to allow "smart" clients to cull themselves is to pass a rect
> (which
> > they could ignore) to their paint method:
> > 
> > void Node::paint(Canvas*, const Rect* dirtyArea) { ... }
> 
> I'm not sure - I think Ian is changing the way we're going to initiate a
paint,
> so I'll hold off on changing this for now.
> 
>
https://codereview.chromium.org/1144483002/diff/30001/sky/engine/core/paintin...
> sky/engine/core/painting/Canvas.h:41: void drawRect(Vector<float> rect, Paint*
> paint);
> On 2015/05/13 20:38:57, reed1 wrote:
> > const Paint& ?
> 
> I'm at the mercy of the IDL compiler as to how this interface looks. We could
> change the compiler, but for now I'll leave it as a pointer (though I added
> const).

well, const was the important part of my suggestion, so thanks.

[Matt Perry, 2015-05-13 21:17:30]

Committed patchset #4 (id:50001) manually as
b1b0224dc9c5189344fa96101cb0cff8ef328f6b (presubmit successful).

[reed1, 2015-05-13 21:20:11]

https://codereview.chromium.org/1144483002/diff/50001/sky/engine/core/paintin...
File sky/engine/core/painting/Canvas.h (right):

https://codereview.chromium.org/1144483002/diff/50001/sky/engine/core/paintin...
sky/engine/core/painting/Canvas.h:29: void saveLayer(const Vector<float>&
bounds, const Paint* paint);
The bounds to saveLayer is optional (passing NULL should be fine), so we may
need this signature to be const * instead of &

https://codereview.chromium.org/1144483002/diff/50001/sky/engine/core/paintin...
sky/engine/core/painting/Canvas.h:40: void drawPaint(Paint* paint);
const for drawPaint

https://codereview.chromium.org/1144483002/diff/50001/sky/engine/core/paintin...
sky/engine/core/painting/Canvas.h:43: void drawCircle(float x, float y, float
radius, Paint* paint);
const for drawCircle

[Matt Perry, 2015-05-13 21:22:00]

https://codereview.chromium.org/1144483002/diff/50001/sky/engine/core/paintin...
File sky/engine/core/painting/Canvas.h (right):

https://codereview.chromium.org/1144483002/diff/50001/sky/engine/core/paintin...
sky/engine/core/painting/Canvas.h:29: void saveLayer(const Vector<float>&
bounds, const Paint* paint);
On 2015/05/13 21:20:10, reed1 wrote:
> The bounds to saveLayer is optional (passing NULL should be fine), so we may
> need this signature to be const * instead of &

The IDL compiler dictates the interface. But passing null does work - it shows
up as an empty vector.

https://codereview.chromium.org/1144483002/diff/50001/sky/engine/core/paintin...
sky/engine/core/painting/Canvas.h:40: void drawPaint(Paint* paint);
On 2015/05/13 21:20:10, reed1 wrote:
> const for drawPaint

Oops, thanks. Will fix in a future CL.

[reed1, 2015-05-14 13:34:24]

On 2015/05/13 21:22:00, Matt Perry wrote:
>
https://codereview.chromium.org/1144483002/diff/50001/sky/engine/core/paintin...
> File sky/engine/core/painting/Canvas.h (right):
> 
>
https://codereview.chromium.org/1144483002/diff/50001/sky/engine/core/paintin...
> sky/engine/core/painting/Canvas.h:29: void saveLayer(const Vector<float>&
> bounds, const Paint* paint);
> On 2015/05/13 21:20:10, reed1 wrote:
> > The bounds to saveLayer is optional (passing NULL should be fine), so we may
> > need this signature to be const * instead of &
> 
> The IDL compiler dictates the interface. But passing null does work - it shows
> up as an empty vector.

Interesting. That seems a little limiting, but I concede its not your issue.

> 
>
https://codereview.chromium.org/1144483002/diff/50001/sky/engine/core/paintin...
> sky/engine/core/painting/Canvas.h:40: void drawPaint(Paint* paint);
> On 2015/05/13 21:20:10, reed1 wrote:
> > const for drawPaint
> 
> Oops, thanks. Will fix in a future CL.