Implement RenderImage and Image for Sky

Implement RenderImage and Image for Sky

This also fixes a bug that was preventing markNeedsLayout from working

R=abarth@chromium.org, eseidel@chromium.org, abarth, eseidel

Committed: https://chromium.googlesource.com/external/mojo/+/6902d78ca11b0400427a391d0774be368a546085

[eseidel, 2015-06-02 23:46:01]

lgtm

https://codereview.chromium.org/1160013004/diff/20001/sky/sdk/lib/framework/r...
File sky/sdk/lib/framework/rendering/render_image.dart (right):

https://codereview.chromium.org/1160013004/diff/20001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/render_image.dart:20: void configure(String src,
num width, num height) {
Should this be private?

https://codereview.chromium.org/1160013004/diff/20001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/render_image.dart:37: void updateSizeFromImage()
{
Should this be private?

[abarth-chromium, 2015-06-03 00:07:54]

not lgtm

There's some problem with updateSizeFromImage not integrating properly with the
layout algorithm.

https://codereview.chromium.org/1160013004/diff/20001/sky/sdk/lib/framework/f...
File sky/sdk/lib/framework/fn2.dart (right):

https://codereview.chromium.org/1160013004/diff/20001/sky/sdk/lib/framework/f...
sky/sdk/lib/framework/fn2.dart:554: RenderNodeWrapper get emptyNode =>
_emptyImage;
We don't need _emptyImage or emptyNode anymore.

https://codereview.chromium.org/1160013004/diff/20001/sky/sdk/lib/framework/r...
File sky/sdk/lib/framework/rendering/render_image.dart (right):

https://codereview.chromium.org/1160013004/diff/20001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/render_image.dart:12: RenderImage(String src,
num width, num height) {
s/num/double/

We're pretty committed to render tree sizes being doubles at this point.

https://codereview.chromium.org/1160013004/diff/20001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/render_image.dart:21: sky.Size newSize = new
sky.Size(width, height);
Probably the caller should pass in the sky.Size instead of passing in height and
width separately.

https://codereview.chromium.org/1160013004/diff/20001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/render_image.dart:25: markNeedsPaint();
No need to markNeedsPaint here.  markNeedsLayout will trigger a paint if this
changes size.

https://codereview.chromium.org/1160013004/diff/20001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/render_image.dart:31: markNeedsLayout();
Shouldn't updateSizeFromImage do the work of markNeedsLayout if the relevant
sizing information changed?  Otherwise we'll do a layout for no reason.

https://codereview.chromium.org/1160013004/diff/20001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/render_image.dart:40: size =
constraints.constrain(requestedSize);
How do you know |constraints| is populated at this point?  This code is called
from a network callback.  We might not have been through layout yet.

Also, you don't want to set |size| outside of a |performLayout| function.  The
position and size of other RenderNodes might depend on the size of this node. 
That's why layout is a coordinated walk of the tree.

https://codereview.chromium.org/1160013004/diff/20001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/render_image.dart:73: canvas.scale(widthScale,
heightScale);
We should skip the save/scale/restore if size.width == _image.width &&
size.height == _image.height.  That's the common case as save/scale/restore is
relatively expensive.

https://codereview.chromium.org/1160013004/diff/20001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/render_image.dart:74: canvas.drawImage(_image,
0.0, 0.0, _paint);
I don't think it's worth caching the paint on this object.  If we need to cache
empty paint objects, we can have a global static one that everyone can share
instead of storing a separate one on every RenderImage object in the tree.

[abarth-chromium, 2015-06-03 00:16:31]

https://codereview.chromium.org/1160013004/diff/40001/sky/examples/raw/intera...
File sky/examples/raw/interactive_flex.dart (right):

https://codereview.chromium.org/1160013004/diff/40001/sky/examples/raw/intera...
sky/examples/raw/interactive_flex.dart:38: }
Can we not copy/paste this class for the Nth time?

https://codereview.chromium.org/1160013004/diff/40001/sky/examples/raw/intera...
sky/examples/raw/interactive_flex.dart:41: RenderImageResizable(String src,
double width, double height)
The caller should pass in a sky.Size instead of width and height separately.

https://codereview.chromium.org/1160013004/diff/40001/sky/examples/raw/intera...
sky/examples/raw/interactive_flex.dart:58: growY = clamp(min: 0.0, value: growY
+ event.y - lastY);
Rather than using clamp, you can just use math.max

https://codereview.chromium.org/1160013004/diff/40001/sky/examples/raw/intera...
sky/examples/raw/interactive_flex.dart:61: configure(_src, newWidth, newHeight);
This leans pretty heavily on the image cache.  Should we instead just have a
setter for the desiredSize so that a client can set the size of the image
without needing to remember the URL of the image?

https://codereview.chromium.org/1160013004/diff/40001/sky/examples/raw/intera...
sky/examples/raw/interactive_flex.dart:62: markNeedsLayout();
This class shouldn't need to markNeedsLayout here.  The call to configure should
mark us for layout if the size change necessitated a layout.

https://codereview.chromium.org/1160013004/diff/40001/sky/examples/raw/intera...
sky/examples/raw/interactive_flex.dart:65: lastY = event.y;
This code will get really confused by multitouch.

It might be better to build an explicit state machine that looks for
pointerdown, remembers the pointer id and the startPoint, then looks at
pointermove and checks the pointer id and does math from there.

https://codereview.chromium.org/1160013004/diff/40001/sky/examples/raw/intera...
sky/examples/raw/interactive_flex.dart:95: RenderImage image1 = new
RenderImageResizable("https://www.dartlang.org/logos/dart-logo.png", 100.0,
100.0);
image1   <--- is there another image?

https://codereview.chromium.org/1160013004/diff/40001/sky/examples/raw/intera...
sky/examples/raw/interactive_flex.dart:96: row.add(new RenderPadding(const
EdgeDims(10.0, 10.0, 10.0, 10.0), image1));
const EdgeDims.all(10.0)

https://codereview.chromium.org/1160013004/diff/40001/sky/sdk/lib/framework/f...
File sky/sdk/lib/framework/fn2.dart (right):

https://codereview.chromium.org/1160013004/diff/40001/sky/sdk/lib/framework/f...
sky/sdk/lib/framework/fn2.dart:626: final double height;
These should be held as a sky.Size not separately as width and height.

https://codereview.chromium.org/1160013004/diff/40001/sky/sdk/lib/framework/r...
File sky/sdk/lib/framework/rendering/image.dart (right):

https://codereview.chromium.org/1160013004/diff/40001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/image.dart:10: class RenderImage extends
RenderBox {
We probably should just put the class in box.dart.

[jackson, 2015-06-03 17:41:43]

https://codereview.chromium.org/1160013004/diff/20001/sky/sdk/lib/framework/f...
File sky/sdk/lib/framework/fn2.dart (right):

https://codereview.chromium.org/1160013004/diff/20001/sky/sdk/lib/framework/f...
sky/sdk/lib/framework/fn2.dart:554: RenderNodeWrapper get emptyNode =>
_emptyImage;
On 2015/06/03 00:07:53, abarth wrote:
> We don't need _emptyImage or emptyNode anymore.

Acknowledged.

https://codereview.chromium.org/1160013004/diff/20001/sky/sdk/lib/framework/r...
File sky/sdk/lib/framework/rendering/render_image.dart (right):

https://codereview.chromium.org/1160013004/diff/20001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/render_image.dart:12: RenderImage(String src,
num width, num height) {
On 2015/06/03 00:07:54, abarth wrote:
> s/num/double/
> 
> We're pretty committed to render tree sizes being doubles at this point.

Acknowledged.

https://codereview.chromium.org/1160013004/diff/40001/sky/examples/raw/intera...
File sky/examples/raw/interactive_flex.dart (right):

https://codereview.chromium.org/1160013004/diff/40001/sky/examples/raw/intera...
sky/examples/raw/interactive_flex.dart:38: }
On 2015/06/03 00:16:31, abarth wrote:
> Can we not copy/paste this class for the Nth time?

Done.

https://codereview.chromium.org/1160013004/diff/40001/sky/examples/raw/intera...
sky/examples/raw/interactive_flex.dart:41: RenderImageResizable(String src,
double width, double height)
On 2015/06/03 00:16:31, abarth wrote:
> The caller should pass in a sky.Size instead of width and height separately.

Done.

https://codereview.chromium.org/1160013004/diff/40001/sky/examples/raw/intera...
sky/examples/raw/interactive_flex.dart:58: growY = clamp(min: 0.0, value: growY
+ event.y - lastY);
On 2015/06/03 00:16:30, abarth wrote:
> Rather than using clamp, you can just use math.max

Done.

https://codereview.chromium.org/1160013004/diff/40001/sky/examples/raw/intera...
sky/examples/raw/interactive_flex.dart:61: configure(_src, newWidth, newHeight);
On 2015/06/03 00:16:31, abarth wrote:
> This leans pretty heavily on the image cache.  Should we instead just have a
> setter for the desiredSize so that a client can set the size of the image
> without needing to remember the URL of the image?

Done.

https://codereview.chromium.org/1160013004/diff/40001/sky/examples/raw/intera...
sky/examples/raw/interactive_flex.dart:62: markNeedsLayout();
On 2015/06/03 00:16:31, abarth wrote:
> This class shouldn't need to markNeedsLayout here.  The call to configure
should
> mark us for layout if the size change necessitated a layout.

Done.

https://codereview.chromium.org/1160013004/diff/40001/sky/examples/raw/intera...
sky/examples/raw/interactive_flex.dart:65: lastY = event.y;
On 2015/06/03 00:16:31, abarth wrote:
> This code will get really confused by multitouch.
> 
> It might be better to build an explicit state machine that looks for
> pointerdown, remembers the pointer id and the startPoint, then looks at
> pointermove and checks the pointer id and does math from there.

Done.

https://codereview.chromium.org/1160013004/diff/40001/sky/examples/raw/intera...
sky/examples/raw/interactive_flex.dart:95: RenderImage image1 = new
RenderImageResizable("https://www.dartlang.org/logos/dart-logo.png", 100.0,
100.0);
On 2015/06/03 00:16:30, abarth wrote:
> image1   <--- is there another image?

Done.

https://codereview.chromium.org/1160013004/diff/40001/sky/examples/raw/intera...
sky/examples/raw/interactive_flex.dart:96: row.add(new RenderPadding(const
EdgeDims(10.0, 10.0, 10.0, 10.0), image1));
On 2015/06/03 00:16:31, abarth wrote:
> const EdgeDims.all(10.0)

Done.

https://codereview.chromium.org/1160013004/diff/40001/sky/sdk/lib/framework/f...
File sky/sdk/lib/framework/fn2.dart (right):

https://codereview.chromium.org/1160013004/diff/40001/sky/sdk/lib/framework/f...
sky/sdk/lib/framework/fn2.dart:626: final double height;
On 2015/06/03 00:16:31, abarth wrote:
> These should be held as a sky.Size not separately as width and height.

Done.

https://codereview.chromium.org/1160013004/diff/40001/sky/sdk/lib/framework/r...
File sky/sdk/lib/framework/rendering/image.dart (right):

https://codereview.chromium.org/1160013004/diff/40001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/image.dart:10: class RenderImage extends
RenderBox {
On 2015/06/03 00:16:31, abarth wrote:
> We probably should just put the class in box.dart.

Done.

[abarth-chromium, 2015-06-03 17:53:22]

https://codereview.chromium.org/1160013004/diff/60001/sky/sdk/lib/framework/f...
File sky/sdk/lib/framework/fn2.dart (right):

https://codereview.chromium.org/1160013004/diff/60001/sky/sdk/lib/framework/f...
sky/sdk/lib/framework/fn2.dart:619: RenderNodeWrapper get emptyNode =>
_emptyImage;
We don't need emptyNode anymore.

https://codereview.chromium.org/1160013004/diff/60001/sky/sdk/lib/framework/r...
File sky/sdk/lib/framework/rendering/box.dart (right):

https://codereview.chromium.org/1160013004/diff/60001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/box.dart:266: markNeedsPaint();
No need to call markNeedsPaint here.  markNeedsLayout will trigger a
markNeedsPaint automatically if something about the size ends up mattering.

https://codereview.chromium.org/1160013004/diff/60001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/box.dart:269: void _updateSizeFromImage() {
This function has one caller, performLayout.  We should just inline it into that
one caller.

https://codereview.chromium.org/1160013004/diff/60001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/box.dart:272: size =
constraints.constrain(requestedSize);
Should we return in this case?

https://codereview.chromium.org/1160013004/diff/60001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/box.dart:285: num width = requestedSize.height *
_image.width / _image.height;
s/num/double/  It's always a double

https://codereview.chromium.org/1160013004/diff/60001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/box.dart:305: canvas.scale(widthScale,
heightScale);
We should skip the save/scale/restore dance if size.width == _image.width &&
size.height == _image.height.  Calling save/scale/restore is expensive and
unnecessary in the common case that the size of the RenderImage matches the
intrinsic dimensions of the underlying image object.

https://codereview.chromium.org/1160013004/diff/60001/sky/sdk/lib/framework/r...
File sky/sdk/lib/framework/rendering/node.dart (right):

https://codereview.chromium.org/1160013004/diff/60001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/node.dart:239: }
Ideally we'd have a test case that covers this bug.

[jackson, 2015-06-03 21:51:05]

Patchset #5 (id:80001) has been deleted

[jackson, 2015-06-03 21:56:08]

Patchset #5 (id:100001) has been deleted

[jackson, 2015-06-03 22:06:26]

https://codereview.chromium.org/1160013004/diff/40001/sky/examples/raw/intera...
File sky/examples/raw/interactive_flex.dart (right):

https://codereview.chromium.org/1160013004/diff/40001/sky/examples/raw/intera...
sky/examples/raw/interactive_flex.dart:65: lastY = event.y;
On 2015/06/03 00:16:31, abarth wrote:
> This code will get really confused by multitouch.
> 
> It might be better to build an explicit state machine that looks for
> pointerdown, remembers the pointer id and the startPoint, then looks at
> pointermove and checks the pointer id and does math from there.

Done.

https://codereview.chromium.org/1160013004/diff/60001/sky/sdk/lib/framework/f...
File sky/sdk/lib/framework/fn2.dart (right):

https://codereview.chromium.org/1160013004/diff/60001/sky/sdk/lib/framework/f...
sky/sdk/lib/framework/fn2.dart:619: RenderNodeWrapper get emptyNode =>
_emptyImage;
On 2015/06/03 17:53:21, abarth wrote:
> We don't need emptyNode anymore.

Done.

https://codereview.chromium.org/1160013004/diff/60001/sky/sdk/lib/framework/r...
File sky/sdk/lib/framework/rendering/box.dart (right):

https://codereview.chromium.org/1160013004/diff/60001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/box.dart:266: markNeedsPaint();
On 2015/06/03 17:53:21, abarth wrote:
> No need to call markNeedsPaint here.  markNeedsLayout will trigger a
> markNeedsPaint automatically if something about the size ends up mattering.

Done.

https://codereview.chromium.org/1160013004/diff/60001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/box.dart:269: void _updateSizeFromImage() {
On 2015/06/03 17:53:21, abarth wrote:
> This function has one caller, performLayout.  We should just inline it into
that
> one caller.

Done.

https://codereview.chromium.org/1160013004/diff/60001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/box.dart:272: size =
constraints.constrain(requestedSize);
On 2015/06/03 17:53:21, abarth wrote:
> Should we return in this case?

Done.

https://codereview.chromium.org/1160013004/diff/60001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/box.dart:285: num width = requestedSize.height *
_image.width / _image.height;
On 2015/06/03 17:53:21, abarth wrote:
> s/num/double/  It's always a double 

Done.

https://codereview.chromium.org/1160013004/diff/60001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/box.dart:305: canvas.scale(widthScale,
heightScale);
On 2015/06/03 17:53:21, abarth wrote:
> We should skip the save/scale/restore dance if size.width == _image.width &&
> size.height == _image.height.  Calling save/scale/restore is expensive and
> unnecessary in the common case that the size of the RenderImage matches the
> intrinsic dimensions of the underlying image object.

Done.

https://codereview.chromium.org/1160013004/diff/60001/sky/sdk/lib/framework/r...
File sky/sdk/lib/framework/rendering/node.dart (right):

https://codereview.chromium.org/1160013004/diff/60001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/node.dart:239: }
On 2015/06/03 17:53:22, abarth wrote:
> Ideally we'd have a test case that covers this bug.

Agree, I will investigate after landing this.

[abarth-chromium, 2015-06-03 22:10:30]

lgtm

https://codereview.chromium.org/1160013004/diff/140001/sky/sdk/lib/framework/...
File sky/sdk/lib/framework/rendering/box.dart (right):

https://codereview.chromium.org/1160013004/diff/140001/sky/sdk/lib/framework/...
sky/sdk/lib/framework/rendering/box.dart:294: requestedSize.height == null ? 0.0
: requestedSize.height);
We need to constaint the size using the box constraints:

size = constraints.constrain(new sky.Size(....

[jackson, 2015-06-03 22:17:17]

Committed patchset #8 (id:180001) manually as
6902d78ca11b0400427a391d0774be368a546085 (presubmit successful).