Decouple Canvas from DisplayList and map Picture and PictureRecorder more directly to their Skia co…

Decouple Canvas from DisplayList and map Picture and PictureRecorder more directly to their Skia counterparts.

Also changes the framework dart code to use the
refactored APIs and fixes the various examples and
tests.

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

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

[iansf, 2015-06-17 23:37:08]

This is still technically in-progress while I fix the expected output from the
tests.  Also, I haven't rebased yet...

[abarth-chromium, 2015-06-18 00:03:53]

This looks great.  I'll give it another look once you've rebased.

https://codereview.chromium.org/1190123003/diff/1/sky/engine/core/painting/Ca...
File sky/engine/core/painting/Canvas.cpp (right):

https://codereview.chromium.org/1190123003/diff/1/sky/engine/core/painting/Ca...
sky/engine/core/painting/Canvas.cpp:90: ASSERT(isRecording());
Does this function just check m_canvas for null?  If so, we should probably just
remove the call because it is redundant with the previous line.

https://codereview.chromium.org/1190123003/diff/1/sky/engine/core/painting/Pi...
File sky/engine/core/painting/PictureRecorder.cpp (right):

https://codereview.chromium.org/1190123003/diff/1/sky/engine/core/painting/Pi...
sky/engine/core/painting/PictureRecorder.cpp:24:
m_pictureRecorder->beginRecording(width, height));
indent

https://codereview.chromium.org/1190123003/diff/1/sky/engine/core/painting/Pi...
File sky/engine/core/painting/PictureRecorder.h (right):

https://codereview.chromium.org/1190123003/diff/1/sky/engine/core/painting/Pi...
sky/engine/core/painting/PictureRecorder.h:16: public DartWrappable {
This should be indented so that the |public| lines up.

https://codereview.chromium.org/1190123003/diff/1/sky/engine/core/painting/Pi...
File sky/engine/core/painting/PictureRecorder.idl (right):

https://codereview.chromium.org/1190123003/diff/1/sky/engine/core/painting/Pi...
sky/engine/core/painting/PictureRecorder.idl:9: boolean isRecording();
readonly attribute boolean isRecording;

That way it shows up in Dart as an attribute getter instead of a function call. 
(The C++ side isn't changed.)

https://codereview.chromium.org/1190123003/diff/1/sky/examples/game/lib/sprit...
File sky/examples/game/lib/sprite_box.dart (right):

https://codereview.chromium.org/1190123003/diff/1/sky/examples/game/lib/sprit...
sky/examples/game/lib/sprite_box.dart:1: import 'dart:sky' as sky;
This file is a "part of" the main game library, which means the imports go in
the main library file.  I believe the main game library already imports
dart:sky, so this isn't needed.  However, I believe it imports it without the
"sky" prefix, which means the paint call below should just be:

void paint(Canvas canvas) {

https://codereview.chromium.org/1190123003/diff/1/sky/tests/resources/display...
File sky/tests/resources/display_list.dart (right):

https://codereview.chromium.org/1190123003/diff/1/sky/tests/resources/display...
sky/tests/resources/display_list.dart:38: sky.Canvas canvas;
I don't think you need to pass |canvas| to this object.  You don't seem to use
it for anything.

[Hixie, 2015-06-18 00:07:08]

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

[Hixie, 2015-06-18 00:07:10]

overall looks good to me, but this isn't my area of expertise

https://codereview.chromium.org/1190123003/diff/1/sky/sdk/lib/framework/rende...
File sky/sdk/lib/framework/rendering/object.dart (right):

https://codereview.chromium.org/1190123003/diff/1/sky/sdk/lib/framework/rende...
sky/sdk/lib/framework/rendering/object.dart:49: 
I really don't like this change. It is going to encourage people to just call
child.paint(canvas) instead of paintChild(canvas).

The reason behind canvas.paintChild(child) is that it will subsequently let us
change whether or not a node painting a child introduces a new
canvas/displaylist/recorder/whatever without having to change any of the call
sites.

https://codereview.chromium.org/1190123003/diff/1/sky/sdk/lib/framework/rende...
File sky/sdk/lib/framework/rendering/stack.dart (right):

https://codereview.chromium.org/1190123003/diff/1/sky/sdk/lib/framework/rende...
sky/sdk/lib/framework/rendering/stack.dart:6: import 'dart:sky' as sky;
If we don't subclass Canvas to have a paintChild() method (as I think we
should), then we should at least have object.dart reexport it so that we don't
have to import dart:sky everywhere.

[iansf, 2015-06-18 18:17:12]

Changes for rebasing, in case you're curious.  Doesn't address any of the prior
comments yet.

[iansf, 2015-06-18 18:17:15]

Changes for rebasing, in case you're curious.  Doesn't address any of the prior
comments yet.

[iansf, 2015-06-20 00:02:40]

New version is here.  The test part still isn't done, though.

https://codereview.chromium.org/1190123003/diff/1/sky/engine/core/painting/Ca...
File sky/engine/core/painting/Canvas.cpp (right):

https://codereview.chromium.org/1190123003/diff/1/sky/engine/core/painting/Ca...
sky/engine/core/painting/Canvas.cpp:90: ASSERT(isRecording());
On 2015/06/18 00:03:52, abarth wrote:
> Does this function just check m_canvas for null?  If so, we should probably
just
> remove the call because it is redundant with the previous line.

Done.

https://codereview.chromium.org/1190123003/diff/1/sky/engine/core/painting/Pi...
File sky/engine/core/painting/PictureRecorder.cpp (right):

https://codereview.chromium.org/1190123003/diff/1/sky/engine/core/painting/Pi...
sky/engine/core/painting/PictureRecorder.cpp:24:
m_pictureRecorder->beginRecording(width, height));
On 2015/06/18 00:03:52, abarth wrote:
> indent

Done.

https://codereview.chromium.org/1190123003/diff/1/sky/engine/core/painting/Pi...
File sky/engine/core/painting/PictureRecorder.h (right):

https://codereview.chromium.org/1190123003/diff/1/sky/engine/core/painting/Pi...
sky/engine/core/painting/PictureRecorder.h:16: public DartWrappable {
On 2015/06/18 00:03:52, abarth wrote:
> This should be indented so that the |public| lines up.

Done.

https://codereview.chromium.org/1190123003/diff/1/sky/engine/core/painting/Pi...
File sky/engine/core/painting/PictureRecorder.idl (right):

https://codereview.chromium.org/1190123003/diff/1/sky/engine/core/painting/Pi...
sky/engine/core/painting/PictureRecorder.idl:9: boolean isRecording();
On 2015/06/18 00:03:52, abarth wrote:
> readonly attribute boolean isRecording;
> 
> That way it shows up in Dart as an attribute getter instead of a function
call. 
> (The C++ side isn't changed.)

Done.

https://codereview.chromium.org/1190123003/diff/1/sky/examples/game/lib/sprit...
File sky/examples/game/lib/sprite_box.dart (right):

https://codereview.chromium.org/1190123003/diff/1/sky/examples/game/lib/sprit...
sky/examples/game/lib/sprite_box.dart:1: import 'dart:sky' as sky;
On 2015/06/18 00:03:52, abarth wrote:
> This file is a "part of" the main game library, which means the imports go in
> the main library file.  I believe the main game library already imports
> dart:sky, so this isn't needed.  However, I believe it imports it without the
> "sky" prefix, which means the paint call below should just be:
> 
> void paint(Canvas canvas) {

Done.

https://codereview.chromium.org/1190123003/diff/1/sky/sdk/lib/framework/rende...
File sky/sdk/lib/framework/rendering/object.dart (right):

https://codereview.chromium.org/1190123003/diff/1/sky/sdk/lib/framework/rende...
sky/sdk/lib/framework/rendering/object.dart:49: 
On 2015/06/18 00:07:09, Hixie wrote:
> I really don't like this change. It is going to encourage people to just call
> child.paint(canvas) instead of paintChild(canvas).
> 
> The reason behind canvas.paintChild(child) is that it will subsequently let us
> change whether or not a node painting a child introduces a new
> canvas/displaylist/recorder/whatever without having to change any of the call
> sites.

Let me know what you think of the new version.

https://codereview.chromium.org/1190123003/diff/1/sky/sdk/lib/framework/rende...
File sky/sdk/lib/framework/rendering/stack.dart (right):

https://codereview.chromium.org/1190123003/diff/1/sky/sdk/lib/framework/rende...
sky/sdk/lib/framework/rendering/stack.dart:6: import 'dart:sky' as sky;
On 2015/06/18 00:07:10, Hixie wrote:
> If we don't subclass Canvas to have a paintChild() method (as I think we
> should), then we should at least have object.dart reexport it so that we don't
> have to import dart:sky everywhere.

Acknowledged.

https://codereview.chromium.org/1190123003/diff/1/sky/tests/resources/display...
File sky/tests/resources/display_list.dart (right):

https://codereview.chromium.org/1190123003/diff/1/sky/tests/resources/display...
sky/tests/resources/display_list.dart:38: sky.Canvas canvas;
On 2015/06/18 00:03:52, abarth wrote:
> I don't think you need to pass |canvas| to this object.  You don't seem to use
> it for anything.

Done.  (This file is still in flux, though, so don't worry about it too much.)

[Hixie, 2015-06-20 00:44:53]

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

https://codereview.chromium.org/1190123003/diff/40001/sky/engine/core/paintin...
sky/engine/core/painting/PictureRecorder.idl:10: void setCanvas(Canvas canvas);
Exposing setCanvas() to dart seems really dodgy to me. What if people call it
with random Canvas objects? Or even objects that implement Canvas but aren't
actually Canvases?

https://codereview.chromium.org/1190123003/diff/40001/sky/sdk/lib/rendering/o...
File sky/sdk/lib/rendering/object.dart (right):

https://codereview.chromium.org/1190123003/diff/40001/sky/sdk/lib/rendering/o...
sky/sdk/lib/rendering/object.dart:39: RenderCanvas(sky.Canvas canvas) :
super(canvas);
We probably want to avoid making two instances of the Canvas every time we
create a canvas.

If you can create a canvas by passing it a canvas, why can't you create a canvas
by passing it a PictureRecorder?

[abarth-chromium, 2015-06-20 00:50:29]

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

https://codereview.chromium.org/1190123003/diff/40001/sky/engine/core/paintin...
sky/engine/core/painting/Canvas.h:66: void clearSkCanvas() { m_canvas = nullptr;
}
How does this mechanism work with the create(PassRefPtr<Canvas>) constructor? 
It seems like the picture recorder will call clearSkCanvas() only on the
original Canvas object, which means other Canvas objects that make a copy of the
same SkCanvas* will now have pointers off into unallocated memory.

https://codereview.chromium.org/1190123003/diff/40001/sky/engine/core/paintin...
sky/engine/core/painting/Canvas.h:67: bool isRecording() const { return m_canvas
!= nullptr; }
Does anyone call this function anymore?  If not, we should probably remove it.

https://codereview.chromium.org/1190123003/diff/40001/sky/engine/core/paintin...
sky/engine/core/painting/Canvas.h:70: Canvas(SkCanvas* skCanvas);
Please mark one-argument constructors explicit.

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

https://codereview.chromium.org/1190123003/diff/40001/sky/engine/core/paintin...
sky/engine/core/painting/PictureRecorder.h:28: bool isRecording() { return
m_canvas != nullptr && m_canvas->isRecording(); }
It is every possible for m_canvas != nullptr and !m_canvas->isRecording() ?

https://codereview.chromium.org/1190123003/diff/40001/sky/engine/core/paintin...
sky/engine/core/painting/PictureRecorder.h:30: void setCanvas(PassRefPtr<Canvas>
canvas) { m_canvas = canvas; }
This function doesn't make much sense.  What does it mean to set the canvas on a
PictureRecorder?  PictureRecorder is a thing that produces a canvas.

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

https://codereview.chromium.org/1190123003/diff/40001/sky/engine/core/paintin...
sky/engine/core/painting/PictureRecorder.idl:10: void setCanvas(Canvas canvas);
This API doesn't make sense.  We should find another way to solve whatever
problem this is solving.

https://codereview.chromium.org/1190123003/diff/40001/sky/sdk/lib/rendering/o...
File sky/sdk/lib/rendering/object.dart (right):

https://codereview.chromium.org/1190123003/diff/40001/sky/sdk/lib/rendering/o...
sky/sdk/lib/rendering/object.dart:33: setCanvas(realCanvas);
I see, you're using this to solve the dangling SkCanvas* problem. 
Unfortunately, we can't use this approach to solve this problem because the API
we expose from C++ to Dart needs to be memory safe.  The way you've set things
up, we need to rely on Dart code using the C++ API with a particular idiom in
order to maintain C++ memory safety.

https://codereview.chromium.org/1190123003/diff/40001/sky/sdk/lib/rendering/o...
sky/sdk/lib/rendering/object.dart:39: RenderCanvas(sky.Canvas canvas) :
super(canvas);
I don't understand why we can't just pass a sky.PictureRecorder to super and
have the C++ Canvas class call |beginRecording| recording on the C++
PictureRecorder to get the SkCanvas*.  Specifically:

RenderCanvas(sky.PictureRecorder recorder, double width, double height) :
super(recorder, width, height);

And on line 31:

return new RenderCanvas(this, width, height);

https://codereview.chromium.org/1190123003/diff/40001/sky/tests/resources/dis...
File sky/tests/resources/display_list.dart (right):

https://codereview.chromium.org/1190123003/diff/40001/sky/tests/resources/dis...
sky/tests/resources/display_list.dart:29: return new TestCanvas(width, height,
this.logger, indent: this.indent);
This function then becomes:

return new TestCanvas(this, width, height, this.logger, indent: this.indent);

https://codereview.chromium.org/1190123003/diff/40001/sky/tests/resources/dis...
sky/tests/resources/display_list.dart:34: TestCanvas(this.width, this.height,
this.logger, { this.indent: '' }) {
I'd call |super| here.

[iansf, 2015-06-23 00:26:59]

iansf@google.com changed reviewers:
+ eseidel@chromium.org, mpcomplete@chromium.org

[iansf, 2015-06-23 00:27:00]

Ok -- hopefully this patch set addresses the remaining issues.  Thanks for the
reviews!

[eseidel, 2015-06-23 00:41:32]

This isn't actually too huge, but I'll need to take another pass through,
probably in the morning.

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

https://codereview.chromium.org/1190123003/diff/80001/sky/engine/core/paintin...
sky/engine/core/painting/Canvas.h:38: recorder->set_canvas(canvas.get());
Who keeps the PictureRecorder alive? , Typically accepting a PassRefPtr<> means
that you're taking ownership of the object, or at least you're comfortable being
the last ref of said object. :)  It looks like this code may depend on someone
else holding onto the PictureRecorder?

https://codereview.chromium.org/1190123003/diff/80001/sky/engine/core/paintin...
sky/engine/core/painting/Canvas.h:73: bool isRecording() const { return m_canvas
!= nullptr; }
!!m_canvas works too.  This is fine though. :)

https://codereview.chromium.org/1190123003/diff/80001/sky/engine/core/paintin...
File sky/engine/core/painting/PictureRecorder.cpp (right):

https://codereview.chromium.org/1190123003/diff/80001/sky/engine/core/paintin...
sky/engine/core/painting/PictureRecorder.cpp:23: return m_canvas != nullptr &&
m_canvas->isRecording();
Hmmm. you might be right, it's possible chromium style requires comparison to
nullptr/0.  Back in WebKit/Blink we would ahve just written return m_canvas &&
m_canvas->isRecording(), but maybe this is the correct chromium (and now sky)
style?

https://codereview.chromium.org/1190123003/diff/80001/sky/engine/core/paintin...
sky/engine/core/painting/PictureRecorder.cpp:35: auto picture = Picture::create(
As much as I like Auto, I don't think this is what you want.  PassRefPtrs
generally are bad news on the stack.  You want to get out of a PassRefPtr into a
RefPtr as soon as possible to avoid the auto-zeroing behavior.

RefPtr<Picture> picture = Picture::create();

return picture.release();

Would be better and would have fewer accidental clear gotchas.

https://codereview.chromium.org/1190123003/diff/80001/sky/engine/core/paintin...
File sky/engine/core/painting/PictureRecorder.h (right):

https://codereview.chromium.org/1190123003/diff/80001/sky/engine/core/paintin...
sky/engine/core/painting/PictureRecorder.h:36: int foo = 0;
I suspect you don't want this?

[abarth-chromium, 2015-06-23 02:28:45]

LGTM

Please address Eric's feedback as well as mine before landing.

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

https://codereview.chromium.org/1190123003/diff/80001/sky/engine/core/paintin...
sky/engine/core/painting/Canvas.h:32: static PassRefPtr<Canvas>
create(PassRefPtr<PictureRecorder> recorder,
I think what Eric is saying is that ideally |PassRefPtr<PictureRecorder>| would
be |PictureRecorder*| because we're not going to keep a reference to the
|PictureRecorder|.  It's possible that doesn't compile because of the IDL code
generator, in which case what you've got here is fine.

https://codereview.chromium.org/1190123003/diff/80001/sky/engine/core/paintin...
sky/engine/core/painting/Canvas.h:38: recorder->set_canvas(canvas.get());
On 2015/06/23 at 00:41:32, eseidel wrote:
> Who keeps the PictureRecorder alive? , Typically accepting a PassRefPtr<>
means that you're taking ownership of the object, or at least you're comfortable
being the last ref of said object. :)  It looks like this code may depend on
someone else holding onto the PictureRecorder?

Canvas effectively has a weak reference to the picture recorder.  We don't
actually use a base::WeakPtr because that would add overhead to every draw call.
 Instead, the PictureRecorder notifies the Canvas when it dies, which makes the
Canvas object ignore all subsequent draw calls.

https://codereview.chromium.org/1190123003/diff/80001/sky/engine/core/paintin...
sky/engine/core/painting/Canvas.h:76: Canvas(SkCanvas* skCanvas);
Please mark one-argument constructors |explicit|.

https://codereview.chromium.org/1190123003/diff/80001/sky/engine/core/paintin...
sky/engine/core/painting/Canvas.h:81: // pointer and manually set the SkCanvas
to null in clearSkCanvas.
@eseidel: Here's the comment that's intended to answer your question.

https://codereview.chromium.org/1190123003/diff/80001/sky/engine/core/paintin...
File sky/engine/core/painting/PictureRecorder.cpp (right):

https://codereview.chromium.org/1190123003/diff/80001/sky/engine/core/paintin...
sky/engine/core/painting/PictureRecorder.cpp:23: return m_canvas != nullptr &&
m_canvas->isRecording();
On 2015/06/23 at 00:41:32, eseidel wrote:
> Hmmm. you might be right, it's possible chromium style requires comparison to
nullptr/0.  Back in WebKit/Blink we would ahve just written return m_canvas &&
m_canvas->isRecording(), but maybe this is the correct chromium (and now sky)
style?

Chromium style doesn't have that rule.  I suspect Ian might just be noticing
that we use "!= null" a lot in Dart code because the Dart language requires it.

https://codereview.chromium.org/1190123003/diff/80001/sky/examples/game/lib/g...
File sky/examples/game/lib/game_demo.dart (right):

https://codereview.chromium.org/1190123003/diff/80001/sky/examples/game/lib/g...
sky/examples/game/lib/game_demo.dart:8: import
'package:sky/rendering/object.dart';
Is this change still needed?

https://codereview.chromium.org/1190123003/diff/80001/sky/examples/raw/hello_...
File sky/examples/raw/hello_world.dart (right):

https://codereview.chromium.org/1190123003/diff/80001/sky/examples/raw/hello_...
sky/examples/raw/hello_world.dart:8: import 'package:sky/rendering/object.dart';
This demo shouldn't depend on package:sky.  It's meant to be a minimal example
of programming the raw dart:sky engine.

https://codereview.chromium.org/1190123003/diff/80001/sky/examples/raw/hello_...
sky/examples/raw/hello_world.dart:15: RenderCanvas canvas = new
RenderCanvas(recorder, width, height);
You can just use regular Canvas here.

https://codereview.chromium.org/1190123003/diff/80001/sky/examples/raw/launch...
File sky/examples/raw/launcher.dart (right):

https://codereview.chromium.org/1190123003/diff/80001/sky/examples/raw/launch...
sky/examples/raw/launcher.dart:17: RenderCanvas canvas = new
RenderCanvas(recorder, width, height);
Again, regular Canvas is fine here.  There's no need to add a dependency on
package:sky/rendering.

https://codereview.chromium.org/1190123003/diff/80001/sky/examples/raw/painti...
File sky/examples/raw/painting.dart (right):

https://codereview.chromium.org/1190123003/diff/80001/sky/examples/raw/painti...
sky/examples/raw/painting.dart:9: import 'package:sky/rendering/object.dart';
Again, please don't add dependencies on package:sky/rendering to these low-level
examples.  They're meant to show how to use the lower level primitives.

https://codereview.chromium.org/1190123003/diff/80001/sky/examples/raw/shadow...
File sky/examples/raw/shadow.dart (right):

https://codereview.chromium.org/1190123003/diff/80001/sky/examples/raw/shadow...
sky/examples/raw/shadow.dart:7: import 'package:sky/rendering/object.dart';
ditto

https://codereview.chromium.org/1190123003/diff/80001/sky/examples/raw/spinni...
File sky/examples/raw/spinning_arabic.dart (right):

https://codereview.chromium.org/1190123003/diff/80001/sky/examples/raw/spinni...
sky/examples/raw/spinning_arabic.dart:8: import
'package:sky/rendering/object.dart';
ditto

https://codereview.chromium.org/1190123003/diff/80001/sky/examples/raw/spinni...
File sky/examples/raw/spinning_image.dart (right):

https://codereview.chromium.org/1190123003/diff/80001/sky/examples/raw/spinni...
sky/examples/raw/spinning_image.dart:9: import
'package:sky/rendering/object.dart';
ditto

https://codereview.chromium.org/1190123003/diff/80001/sky/examples/raw/spinni...
File sky/examples/raw/spinning_square.dart (right):

https://codereview.chromium.org/1190123003/diff/80001/sky/examples/raw/spinni...
sky/examples/raw/spinning_square.dart:8: import
'package:sky/rendering/object.dart';
ditto

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

https://codereview.chromium.org/1190123003/diff/80001/sky/sdk/lib/rendering/b...
sky/sdk/lib/rendering/box.dart:1081: RenderObject.debugDoingPaint = false;
This line of code isn't needed.  We assign this value already on line 1083. 
Maybe a bad merge?

[Hixie, 2015-06-23 16:36:49]

https://codereview.chromium.org/1190123003/diff/80001/sky/examples/raw/painti...
File sky/examples/raw/painting.dart (right):

https://codereview.chromium.org/1190123003/diff/80001/sky/examples/raw/painti...
sky/examples/raw/painting.dart:9: import 'package:sky/rendering/object.dart';
On 2015/06/23 at 02:28:45, abarth wrote:
> Again, please don't add dependencies on package:sky/rendering to these
low-level examples.  They're meant to show how to use the lower level
primitives.

Specifically, nothing in examples/raw/ should have any dependencies on anything
in the SDK directory.

https://codereview.chromium.org/1190123003/diff/80001/sky/tests/resources/dis...
File sky/tests/resources/display_list.dart (right):

https://codereview.chromium.org/1190123003/diff/80001/sky/tests/resources/dis...
sky/tests/resources/display_list.dart:23: final sky.PictureRecorder recorder;
what's this used for?

https://codereview.chromium.org/1190123003/diff/80001/sky/tests/resources/dis...
sky/tests/resources/display_list.dart:140: var recorder = new
sky.PictureRecorder();
why is the recorder needed here?

[iansf, 2015-06-23 22:46:10]

One more CL incoming.

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

https://codereview.chromium.org/1190123003/diff/80001/sky/engine/core/paintin...
sky/engine/core/painting/Canvas.h:32: static PassRefPtr<Canvas>
create(PassRefPtr<PictureRecorder> recorder,
On 2015/06/23 02:28:44, abarth wrote:
> I think what Eric is saying is that ideally |PassRefPtr<PictureRecorder>|
would
> be |PictureRecorder*| because we're not going to keep a reference to the
> |PictureRecorder|.  It's possible that doesn't compile because of the IDL code
> generator, in which case what you've got here is fine.

Confirmed that |PictureRecorder*| works, and updated accordingly.

https://codereview.chromium.org/1190123003/diff/80001/sky/engine/core/paintin...
sky/engine/core/painting/Canvas.h:73: bool isRecording() const { return m_canvas
!= nullptr; }
On 2015/06/23 00:41:32, eseidel wrote:
> !!m_canvas works too.  This is fine though. :)

Acknowledged.

https://codereview.chromium.org/1190123003/diff/80001/sky/engine/core/paintin...
sky/engine/core/painting/Canvas.h:76: Canvas(SkCanvas* skCanvas);
On 2015/06/23 02:28:44, abarth wrote:
> Please mark one-argument constructors |explicit|.

Done.

https://codereview.chromium.org/1190123003/diff/80001/sky/engine/core/paintin...
File sky/engine/core/painting/PictureRecorder.cpp (right):

https://codereview.chromium.org/1190123003/diff/80001/sky/engine/core/paintin...
sky/engine/core/painting/PictureRecorder.cpp:23: return m_canvas != nullptr &&
m_canvas->isRecording();
On 2015/06/23 02:28:44, abarth wrote:
> On 2015/06/23 at 00:41:32, eseidel wrote:
> > Hmmm. you might be right, it's possible chromium style requires comparison
to
> nullptr/0.  Back in WebKit/Blink we would ahve just written return m_canvas &&
> m_canvas->isRecording(), but maybe this is the correct chromium (and now sky)
> style?
> 
> Chromium style doesn't have that rule.  I suspect Ian might just be noticing
> that we use "!= null" a lot in Dart code because the Dart language requires
it.

Oh -- originally this was just a check of m_canvas, and I felt like the code was
made more clear with the explicit nullptr comparison after the return, but if
that's not the style, I'm happy to remove the explicit check here and in the
other spot.

https://codereview.chromium.org/1190123003/diff/80001/sky/engine/core/paintin...
sky/engine/core/painting/PictureRecorder.cpp:35: auto picture = Picture::create(
On 2015/06/23 00:41:32, eseidel wrote:
> As much as I like Auto, I don't think this is what you want.  PassRefPtrs
> generally are bad news on the stack.  You want to get out of a PassRefPtr into
a
> RefPtr as soon as possible to avoid the auto-zeroing behavior.
> 
> RefPtr<Picture> picture = Picture::create();
> 
> return picture.release();
> 
> Would be better and would have fewer accidental clear gotchas.

Thanks for the explanation!  Done.

https://codereview.chromium.org/1190123003/diff/80001/sky/engine/core/paintin...
File sky/engine/core/painting/PictureRecorder.h (right):

https://codereview.chromium.org/1190123003/diff/80001/sky/engine/core/paintin...
sky/engine/core/painting/PictureRecorder.h:36: int foo = 0;
On 2015/06/23 00:41:32, eseidel wrote:
> I suspect you don't want this?

You may be right... ;-)

https://codereview.chromium.org/1190123003/diff/80001/sky/examples/game/lib/g...
File sky/examples/game/lib/game_demo.dart (right):

https://codereview.chromium.org/1190123003/diff/80001/sky/examples/game/lib/g...
sky/examples/game/lib/game_demo.dart:8: import
'package:sky/rendering/object.dart';
On 2015/06/23 02:28:45, abarth wrote:
> Is this change still needed?

Yes -- otherwise the game doesn't know about RenderCanvas, which is used in the
paint calls.  Is that problematic?

https://codereview.chromium.org/1190123003/diff/80001/sky/examples/raw/hello_...
File sky/examples/raw/hello_world.dart (right):

https://codereview.chromium.org/1190123003/diff/80001/sky/examples/raw/hello_...
sky/examples/raw/hello_world.dart:8: import 'package:sky/rendering/object.dart';
On 2015/06/23 02:28:45, abarth wrote:
> This demo shouldn't depend on package:sky.  It's meant to be a minimal example
> of programming the raw dart:sky engine.

Done.

https://codereview.chromium.org/1190123003/diff/80001/sky/examples/raw/hello_...
sky/examples/raw/hello_world.dart:15: RenderCanvas canvas = new
RenderCanvas(recorder, width, height);
On 2015/06/23 02:28:45, abarth wrote:
> You can just use regular Canvas here.

Done.

https://codereview.chromium.org/1190123003/diff/80001/sky/examples/raw/launch...
File sky/examples/raw/launcher.dart (right):

https://codereview.chromium.org/1190123003/diff/80001/sky/examples/raw/launch...
sky/examples/raw/launcher.dart:17: RenderCanvas canvas = new
RenderCanvas(recorder, width, height);
On 2015/06/23 02:28:45, abarth wrote:
> Again, regular Canvas is fine here.  There's no need to add a dependency on
> package:sky/rendering.

Done.

https://codereview.chromium.org/1190123003/diff/80001/sky/examples/raw/painti...
File sky/examples/raw/painting.dart (right):

https://codereview.chromium.org/1190123003/diff/80001/sky/examples/raw/painti...
sky/examples/raw/painting.dart:9: import 'package:sky/rendering/object.dart';
On 2015/06/23 02:28:45, abarth wrote:
> Again, please don't add dependencies on package:sky/rendering to these
low-level
> examples.  They're meant to show how to use the lower level primitives.

Done.

https://codereview.chromium.org/1190123003/diff/80001/sky/examples/raw/painti...
sky/examples/raw/painting.dart:9: import 'package:sky/rendering/object.dart';
On 2015/06/23 16:36:48, Hixie wrote:
> On 2015/06/23 at 02:28:45, abarth wrote:
> > Again, please don't add dependencies on package:sky/rendering to these
> low-level examples.  They're meant to show how to use the lower level
> primitives.
> 
> Specifically, nothing in examples/raw/ should have any dependencies on
anything
> in the SDK directory.

Done.

https://codereview.chromium.org/1190123003/diff/80001/sky/examples/raw/shadow...
File sky/examples/raw/shadow.dart (right):

https://codereview.chromium.org/1190123003/diff/80001/sky/examples/raw/shadow...
sky/examples/raw/shadow.dart:7: import 'package:sky/rendering/object.dart';
On 2015/06/23 02:28:45, abarth wrote:
> ditto

Done.

https://codereview.chromium.org/1190123003/diff/80001/sky/examples/raw/spinni...
File sky/examples/raw/spinning_arabic.dart (right):

https://codereview.chromium.org/1190123003/diff/80001/sky/examples/raw/spinni...
sky/examples/raw/spinning_arabic.dart:8: import
'package:sky/rendering/object.dart';
On 2015/06/23 02:28:45, abarth wrote:
> ditto

Done.

https://codereview.chromium.org/1190123003/diff/80001/sky/examples/raw/spinni...
File sky/examples/raw/spinning_image.dart (right):

https://codereview.chromium.org/1190123003/diff/80001/sky/examples/raw/spinni...
sky/examples/raw/spinning_image.dart:9: import
'package:sky/rendering/object.dart';
On 2015/06/23 02:28:45, abarth wrote:
> ditto 

Done.

https://codereview.chromium.org/1190123003/diff/80001/sky/examples/raw/spinni...
File sky/examples/raw/spinning_square.dart (right):

https://codereview.chromium.org/1190123003/diff/80001/sky/examples/raw/spinni...
sky/examples/raw/spinning_square.dart:8: import
'package:sky/rendering/object.dart';
On 2015/06/23 02:28:45, abarth wrote:
> ditto 

Done.

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

https://codereview.chromium.org/1190123003/diff/80001/sky/sdk/lib/rendering/b...
sky/sdk/lib/rendering/box.dart:1081: RenderObject.debugDoingPaint = false;
On 2015/06/23 02:28:45, abarth wrote:
> This line of code isn't needed.  We assign this value already on line 1083. 
> Maybe a bad merge?

Correct.  Good catch.

https://codereview.chromium.org/1190123003/diff/80001/sky/tests/resources/dis...
File sky/tests/resources/display_list.dart (right):

https://codereview.chromium.org/1190123003/diff/80001/sky/tests/resources/dis...
sky/tests/resources/display_list.dart:23: final sky.PictureRecorder recorder;
On 2015/06/23 16:36:48, Hixie wrote:
> what's this used for?

Acknowledged.

https://codereview.chromium.org/1190123003/diff/80001/sky/tests/resources/dis...
sky/tests/resources/display_list.dart:140: var recorder = new
sky.PictureRecorder();
On 2015/06/23 16:36:49, Hixie wrote:
> why is the recorder needed here?

As discussed, changing this to create a new |sky.PictureRecorder| each time.

[iansf, 2015-06-23 22:56:03]

Hopefully final set of changes for this patch.

[abarth-chromium, 2015-06-24 01:29:34]

LGTM

Ship it!  :)

https://codereview.chromium.org/1190123003/diff/120001/sky/engine/core/painti...
File sky/engine/core/painting/PictureRecorder.cpp (right):

https://codereview.chromium.org/1190123003/diff/120001/sky/engine/core/painti...
sky/engine/core/painting/PictureRecorder.cpp:42: void
PictureRecorder::set_canvas(PassRefPtr<Canvas> canvas) { m_canvas = canvas; }
Usually we put trivial setters like this in the header so the compiler can
inline them.

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

https://codereview.chromium.org/1190123003/diff/120001/sky/engine/core/painti...
sky/engine/core/painting/PictureRecorder.h:29: // PassRefPtr<Canvas>
beginRecording(double width, double height);
Please remove commented out code.

https://codereview.chromium.org/1190123003/diff/120001/sky/engine/core/painti...
sky/engine/core/painting/PictureRecorder.h:40: RefPtr<Canvas> m_canvas;
Please make the indent consistent in this file.

[iansf, 2015-06-24 17:21:54]

Committed patchset #7 (id:120001) manually as
ef31fdcc6fd53f71abda8fb960fff447114ca9aa (presubmit successful).