Implement ClipRRect

Implement ClipRRect

R=abarth@chromium.org, abarth

Committed: https://chromium.googlesource.com/external/mojo/+/cb7478e737ebf03e39c8f65752171f35cf4a366a

[abarth-chromium, 2015-06-25 17:11:21]

https://codereview.chromium.org/1206373002/diff/1/sky/sdk/lib/rendering/box.dart
File sky/sdk/lib/rendering/box.dart (right):

https://codereview.chromium.org/1206373002/diff/1/sky/sdk/lib/rendering/box.d...
sky/sdk/lib/rendering/box.dart:561: final double yRad;
xRadius and yRadius

Please use complete words in variable names.

Also, these shouldn't be final.  They should have setters that compare the new
value to the existing value and call markNeedsPaint if the value changes.

https://codereview.chromium.org/1206373002/diff/1/sky/sdk/lib/widgets/basic.dart
File sky/sdk/lib/widgets/basic.dart (right):

https://codereview.chromium.org/1206373002/diff/1/sky/sdk/lib/widgets/basic.d...
sky/sdk/lib/widgets/basic.dart:111: final double yRad;
xRadius and yRadius

https://codereview.chromium.org/1206373002/diff/1/sky/sdk/lib/widgets/basic.d...
sky/sdk/lib/widgets/basic.dart:116: RenderClipRRect createNode() => new
RenderClipRRect(xRad: xRad, yRad: yRad);
You're missing a syncRenderObject function.  Looks like ClipRect and ClipOval
are missing them too.  Padding is a good class to use as a template here.

[Hixie, 2015-06-25 17:16:58]

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

[Hixie, 2015-06-25 17:16:58]

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

[Hixie, 2015-06-25 17:16:59]

https://codereview.chromium.org/1206373002/diff/1/sky/sdk/lib/rendering/box.dart
File sky/sdk/lib/rendering/box.dart (right):

https://codereview.chromium.org/1206373002/diff/1/sky/sdk/lib/rendering/box.d...
sky/sdk/lib/rendering/box.dart:561: final double yRad;
also, constructors first. fields second or with the code that uses them.

https://codereview.chromium.org/1206373002/diff/1/sky/sdk/lib/rendering/box.d...
sky/sdk/lib/rendering/box.dart:571: sky.RRect rrect = new
sky.RRect()..setRectXY(rect, xRad, yRad);
Doesn't RRect() have a suitable constructor?

Indeed, why isn't RRect immutable?

https://codereview.chromium.org/1206373002/diff/1/sky/sdk/lib/rendering/box.d...
sky/sdk/lib/rendering/box.dart:572: canvas.clipRRect(rrect);
What's the anti-aliasing story here?

(ok, trick question, we talked about this. But we should contact Skia people
about it.)

[Hixie, 2015-06-25 17:16:59]

https://codereview.chromium.org/1206373002/diff/1/sky/sdk/lib/rendering/box.dart
File sky/sdk/lib/rendering/box.dart (right):

https://codereview.chromium.org/1206373002/diff/1/sky/sdk/lib/rendering/box.d...
sky/sdk/lib/rendering/box.dart:571: sky.RRect rrect = new
sky.RRect()..setRectXY(rect, xRad, yRad);
Doesn't RRect() have a suitable constructor?

Indeed, why isn't RRect immutable?

https://codereview.chromium.org/1206373002/diff/1/sky/sdk/lib/rendering/box.d...
sky/sdk/lib/rendering/box.dart:572: canvas.clipRRect(rrect);
What's the anti-aliasing story here?

(ok, trick question, we talked about this. But we should contact Skia people
about it.)

[jackson, 2015-06-25 17:21:35]

https://codereview.chromium.org/1206373002/diff/1/sky/sdk/lib/widgets/basic.dart
File sky/sdk/lib/widgets/basic.dart (right):

https://codereview.chromium.org/1206373002/diff/1/sky/sdk/lib/widgets/basic.d...
sky/sdk/lib/widgets/basic.dart:116: RenderClipRRect createNode() => new
RenderClipRRect(xRad: xRad, yRad: yRad);
On 2015/06/25 17:11:21, abarth wrote:
> You're missing a syncRenderObject function.  Looks like ClipRect and ClipOval
> are missing them too.  Padding is a good class to use as a template here.

ClipRect and ClipOval are missing them, but RenderClipRect and RenderClipOval
have no members to sync. Should I have a sync method anyway for consistency?

[abarth-chromium, 2015-06-25 17:30:13]

On 2015/06/25 at 17:21:35, jackson wrote:
> ClipRect and ClipOval are missing them, but RenderClipRect and RenderClipOval
have no members to sync. Should I have a sync method anyway for consistency?

Ah, true.  If there aren't any members to sync, then we don't need a sync
method.

[jackson, 2015-06-25 17:56:07]

Here is some text that I can add to the readme in lib/ or lib/widgets/ to make
it clear that RRect and Paint are mutable. Edits are welcome.

[abarth-chromium, 2015-06-25 18:04:13]

LGTM

https://codereview.chromium.org/1206373002/diff/20001/sky/sdk/lib/README.md
File sky/sdk/lib/README.md (right):

https://codereview.chromium.org/1206373002/diff/20001/sky/sdk/lib/README.md#n...
sky/sdk/lib/README.md:78: the widget.
This text is good, but I'm not sure this is the right README file for it.  This
doc is pretty high-level and this text is pretty specific...  Maybe create a
README.md in sky/sdk/lib/painting with this text?  That way the
painting/README.md can grow to be all about the painting aspects of the system.

https://codereview.chromium.org/1206373002/diff/20001/sky/sdk/lib/rendering/b...
File sky/sdk/lib/rendering/box.dart (right):

https://codereview.chromium.org/1206373002/diff/20001/sky/sdk/lib/rendering/b...
sky/sdk/lib/rendering/box.dart:561: : _xRadius = xRadius,_yRadius = yRadius,
super(child) {
s/xRadius,_yRadius/xRadius, _yRadius/

https://codereview.chromium.org/1206373002/diff/20001/sky/sdk/lib/widgets/bas...
File sky/sdk/lib/widgets/basic.dart (right):

https://codereview.chromium.org/1206373002/diff/20001/sky/sdk/lib/widgets/bas...
sky/sdk/lib/widgets/basic.dart:465: 
Looks like this part of the diff is from your next CL.  :)

[Hixie, 2015-06-25 18:22:36]

https://codereview.chromium.org/1206373002/diff/20001/sky/sdk/lib/README.md
File sky/sdk/lib/README.md (right):

https://codereview.chromium.org/1206373002/diff/20001/sky/sdk/lib/README.md#n...
sky/sdk/lib/README.md:78: the widget.
On 2015/06/25 at 18:04:12, abarth wrote:
> This text is good, but I'm not sure this is the right README file for it. 
This doc is pretty high-level and this text is pretty specific...  Maybe create
a README.md in sky/sdk/lib/painting with this text?  That way the
painting/README.md can grow to be all about the painting aspects of the system.

Yeah we weren't sure where to put it. We considered painting/ first, actually,
but it's not really the place to decument the dart:sky API (which is what this
is).

I suggested to Collin that we put it in this README for now and that once this
section grew big enough we'd split it into a separate SKYENGINE.md file or some
such.

[Hixie, 2015-06-25 18:24:47]

https://codereview.chromium.org/1206373002/diff/20001/sky/sdk/lib/widgets/bas...
File sky/sdk/lib/widgets/basic.dart (right):

https://codereview.chromium.org/1206373002/diff/20001/sky/sdk/lib/widgets/bas...
sky/sdk/lib/widgets/basic.dart:109: // ClipRect has nothing to sync
see comment below

https://codereview.chromium.org/1206373002/diff/20001/sky/sdk/lib/widgets/bas...
sky/sdk/lib/widgets/basic.dart:137: void syncRenderObject(ClipOval old) {
Don't have functions that do nothing but call super, that's just wasted cycles.

Maybe have a comment to the effect of
// ClipOval has nothing to sync, so doesn't implement syncRenderObject()
...or something?

[abarth-chromium, 2015-06-25 18:34:01]

https://codereview.chromium.org/1206373002/diff/20001/sky/sdk/lib/README.md
File sky/sdk/lib/README.md (right):

https://codereview.chromium.org/1206373002/diff/20001/sky/sdk/lib/README.md#n...
sky/sdk/lib/README.md:78: the widget.
On 2015/06/25 at 18:22:36, Hixie wrote:
> I suggested to Collin that we put it in this README for now and that once this
section grew big enough we'd split it into a separate SKYENGINE.md file or some
such.

Yeah, we're clearly going to need docs for all the dart:sky APIs, and you're
right that this belongs with that more than lib/painting.

I'm fine with this text being here until we've got a better home for it.

[jackson, 2015-06-25 20:32:30]

Committed patchset #3 (id:40001) manually as
cb7478e737ebf03e39c8f65752171f35cf4a366a (presubmit successful).