Make rendering use PaintingNodes for increased efficiency.

Make rendering use PaintingNodes for increased efficiency.
WIP -- need to update some tests at the least.

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

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

[Hixie, 2015-07-02 00:19:54]

https://codereview.chromium.org/1216833003/diff/1/sky/sdk/lib/rendering/objec...
File sky/sdk/lib/rendering/object.dart (right):

https://codereview.chromium.org/1216833003/diff/1/sky/sdk/lib/rendering/objec...
sky/sdk/lib/rendering/object.dart:32: List<RenderObject> _children; // used by
_doPaint() to find out which RenderObjects to ask to paint
rename this to _descendantsWithPaintNodes or some such

https://codereview.chromium.org/1216833003/diff/1/sky/sdk/lib/rendering/objec...
sky/sdk/lib/rendering/object.dart:39: 
remove this blank line

https://codereview.chromium.org/1216833003/diff/1/sky/sdk/lib/rendering/objec...
sky/sdk/lib/rendering/object.dart:270: 
remove this random blank line :-)

https://codereview.chromium.org/1216833003/diff/1/sky/sdk/lib/rendering/objec...
sky/sdk/lib/rendering/object.dart:291: void _doPaint() {
call this _paintOnPaintNode() or _updatePaintNode() or some such.

https://codereview.chromium.org/1216833003/diff/1/sky/sdk/lib/rendering/objec...
sky/sdk/lib/rendering/object.dart:298: performPaint(canvas,
Point.origin.toOffset());
use Offset.zero instead

https://codereview.chromium.org/1216833003/diff/1/sky/sdk/lib/rendering/objec...
sky/sdk/lib/rendering/object.dart:300: print('Exception raised during paint of
${this.runtimeType}: ${e}');
make this match the style in layout()

https://codereview.chromium.org/1216833003/diff/1/sky/sdk/lib/rendering/objec...
sky/sdk/lib/rendering/object.dart:318: Rect get paintBounds;
move this below performPaint()

https://codereview.chromium.org/1216833003/diff/1/sky/sdk/lib/rendering/objec...
sky/sdk/lib/rendering/object.dart:320: void performPaint(PaintingCanvas canvas,
Offset offset) {
call this _paintOnCanvas() or some such.

[iansf, 2015-07-06 18:22:01]

New version.  Test updates next.

https://codereview.chromium.org/1216833003/diff/1/sky/sdk/lib/rendering/objec...
File sky/sdk/lib/rendering/object.dart (right):

https://codereview.chromium.org/1216833003/diff/1/sky/sdk/lib/rendering/objec...
sky/sdk/lib/rendering/object.dart:32: List<RenderObject> _children; // used by
_doPaint() to find out which RenderObjects to ask to paint
On 2015/07/02 00:19:54, Hixie wrote:
> rename this to _descendantsWithPaintNodes or some such

Done.

https://codereview.chromium.org/1216833003/diff/1/sky/sdk/lib/rendering/objec...
sky/sdk/lib/rendering/object.dart:39: 
On 2015/07/02 00:19:54, Hixie wrote:
> remove this blank line

Done.

https://codereview.chromium.org/1216833003/diff/1/sky/sdk/lib/rendering/objec...
sky/sdk/lib/rendering/object.dart:270: 
On 2015/07/02 00:19:54, Hixie wrote:
> remove this random blank line :-)

Done.

https://codereview.chromium.org/1216833003/diff/1/sky/sdk/lib/rendering/objec...
sky/sdk/lib/rendering/object.dart:291: void _doPaint() {
On 2015/07/02 00:19:54, Hixie wrote:
> call this _paintOnPaintNode() or _updatePaintNode() or some such.

Done.

https://codereview.chromium.org/1216833003/diff/1/sky/sdk/lib/rendering/objec...
sky/sdk/lib/rendering/object.dart:298: performPaint(canvas,
Point.origin.toOffset());
On 2015/07/02 00:19:54, Hixie wrote:
> use Offset.zero instead

Done.

https://codereview.chromium.org/1216833003/diff/1/sky/sdk/lib/rendering/objec...
sky/sdk/lib/rendering/object.dart:300: print('Exception raised during paint of
${this.runtimeType}: ${e}');
On 2015/07/02 00:19:54, Hixie wrote:
> make this match the style in layout()

Done.

https://codereview.chromium.org/1216833003/diff/1/sky/sdk/lib/rendering/objec...
sky/sdk/lib/rendering/object.dart:318: Rect get paintBounds;
On 2015/07/02 00:19:54, Hixie wrote:
> move this below performPaint()

Done.

https://codereview.chromium.org/1216833003/diff/1/sky/sdk/lib/rendering/objec...
sky/sdk/lib/rendering/object.dart:320: void performPaint(PaintingCanvas canvas,
Offset offset) {
On 2015/07/02 00:19:54, Hixie wrote:
> call this _paintOnCanvas() or some such.

Done.

[abarth-chromium, 2015-07-06 20:50:56]

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

[abarth-chromium, 2015-07-06 20:50:58]

https://codereview.chromium.org/1216833003/diff/20001/sky/engine/core/paintin...
File sky/engine/core/painting/PaintingNode.cpp (right):

https://codereview.chromium.org/1216833003/diff/20001/sky/engine/core/paintin...
sky/engine/core/painting/PaintingNode.cpp:29: }
Rather than having two constructors, we could just have one that takes a
PassRefPtr<SkDrawable>.  We can then delete PaintingNodeDrawable::create() and
just have the other |create| function default to nullptr:

PaintingNodeDrawable::create(PassRefPtr<SkDrawable> skDrawable = nullptr)

https://codereview.chromium.org/1216833003/diff/20001/sky/engine/core/paintin...
sky/engine/core/painting/PaintingNode.cpp:67: assert(m_paintingNodeDrawable);
s/assert/ASSERT/

ASSERT is a macro we define that has better logging than assert.

https://codereview.chromium.org/1216833003/diff/20001/sky/engine/core/paintin...
File sky/engine/core/painting/PaintingNode.h (right):

https://codereview.chromium.org/1216833003/diff/20001/sky/engine/core/paintin...
sky/engine/core/painting/PaintingNode.h:21: ~PaintingNodeDrawable() override {}
nit: we usually put virtual destructors out-of-line.

https://codereview.chromium.org/1216833003/diff/20001/sky/engine/core/paintin...
sky/engine/core/painting/PaintingNode.h:23: SkRect onGetBounds();
override?

https://codereview.chromium.org/1216833003/diff/20001/sky/engine/core/paintin...
sky/engine/core/painting/PaintingNode.h:25: SkPicture* onNewPictureSnapshot();
override?

https://codereview.chromium.org/1216833003/diff/20001/sky/engine/core/paintin...
sky/engine/core/painting/PaintingNode.h:37: class PaintingNode : public
RefCounted<PaintingNode>, public DartWrappable {
We prefer one class per file.  Would you be willing to put these classes in
separate files?

https://codereview.chromium.org/1216833003/diff/20001/sky/engine/core/paintin...
sky/engine/core/painting/PaintingNode.h:50: PassRefPtr<Picture>
newPictureSnapshot();
Does anyone call this function?  If not, we should remove it.

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

https://codereview.chromium.org/1216833003/diff/20001/sky/sdk/lib/rendering/o...
sky/sdk/lib/rendering/object.dart:36: _descendentsWithPaintingCanvases = new
List<RenderObject>();
You can simplify this pattern by writing:

final List<RenderObject> _descendentsWithPaintingCanvases = new
List<RenderObject>();

The list will be created lazily when you first access
_descendentsWithPaintingCanvases.

https://codereview.chromium.org/1216833003/diff/20001/sky/sdk/lib/rendering/o...
sky/sdk/lib/rendering/object.dart:41: translate(-point.x, -point.y);
It looks like drawPaintingNode should take a Point so we don't need to do this
matrix operations from Dart.  The Skia team is adding offsets to drawDrawable so
we can refactor the C++ to not use translate once we have that patch.

https://codereview.chromium.org/1216833003/diff/20001/sky/sdk/lib/rendering/o...
sky/sdk/lib/rendering/object.dart:257: sky.PaintingNode _paintingNode = new
sky.PaintingNode(); // only set if createNewDisplayList is true
Without |final|, this call to new will happen all the time.  Maybe leave it with
a default value of |null| ?

https://codereview.chromium.org/1216833003/diff/20001/sky/sdk/lib/rendering/o...
sky/sdk/lib/rendering/object.dart:261: bool get createNewDisplayList => true;
I'm not sure we want the default behavior to be to create a new display list. 
Most RenderObjects don't paint anything at all, so that seems very wasteful.

https://codereview.chromium.org/1216833003/diff/20001/sky/sdk/lib/rendering/o...
sky/sdk/lib/rendering/object.dart:286: _debugDoingPaint = false;
We should wrap the body of this function in a try .. finally block to make sure
we always set _debugDoingPaint = false on the way out of this function if we hit
an exception.  If we don't set _debugDoingPaint = false, we'll be in a very
corrupt state.

https://codereview.chromium.org/1216833003/diff/20001/sky/sdk/lib/rendering/o...
sky/sdk/lib/rendering/object.dart:297: } catch (e, stack) {
Capturing the stack here is going to be too expensive.  Can we just capture |e|
alone?

https://codereview.chromium.org/1216833003/diff/20001/sky/sdk/lib/rendering/o...
sky/sdk/lib/rendering/object.dart:307:
canvas._descendentsWithPaintingCanvases.forEach((node) {
We should use a for-each loop rather than calling |forEach|.  Calling the
forEach function is more expensive because we need to allocate a stack from for
each entry in the list:

for (RenderObject node in canvas._descendentsWithPaintingCanvases) {
  ...
}

https://codereview.chromium.org/1216833003/diff/20001/sky/sdk/lib/rendering/o...
sky/sdk/lib/rendering/object.dart:317: _needsPaint = false;
Why do we set _needsPaint = false both here and on line 294?  It seems like we
can remove the set on line 294.

[Hixie, 2015-07-06 21:13:42]

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

https://codereview.chromium.org/1216833003/diff/20001/sky/sdk/lib/rendering/o...
sky/sdk/lib/rendering/object.dart:297: } catch (e, stack) {
On 2015/07/06 at 20:50:58, abarth-chromium wrote:
> Capturing the stack here is going to be too expensive.  Can we just capture
|e| alone?

If you change this make sure the equivalent code in layout is kept in sync.

[iansf, 2015-07-07 20:36:47]

Still have to fix tests, but this should address the other concerns and the
rendering issues.

https://codereview.chromium.org/1216833003/diff/20001/sky/engine/core/paintin...
File sky/engine/core/painting/PaintingNode.cpp (right):

https://codereview.chromium.org/1216833003/diff/20001/sky/engine/core/paintin...
sky/engine/core/painting/PaintingNode.cpp:67: assert(m_paintingNodeDrawable);
On 2015/07/06 20:50:57, abarth-chromium wrote:
> s/assert/ASSERT/
> 
> ASSERT is a macro we define that has better logging than assert.

Done.

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

https://codereview.chromium.org/1216833003/diff/20001/sky/sdk/lib/rendering/o...
sky/sdk/lib/rendering/object.dart:36: _descendentsWithPaintingCanvases = new
List<RenderObject>();
On 2015/07/06 20:50:58, abarth-chromium wrote:
> You can simplify this pattern by writing:
> 
> final List<RenderObject> _descendentsWithPaintingCanvases = new
> List<RenderObject>();
> 
> The list will be created lazily when you first access
> _descendentsWithPaintingCanvases.

Done.

https://codereview.chromium.org/1216833003/diff/20001/sky/sdk/lib/rendering/o...
sky/sdk/lib/rendering/object.dart:41: translate(-point.x, -point.y);
On 2015/07/06 20:50:58, abarth-chromium wrote:
> It looks like drawPaintingNode should take a Point so we don't need to do this
> matrix operations from Dart.  The Skia team is adding offsets to drawDrawable
so
> we can refactor the C++ to not use translate once we have that patch.

Done.

https://codereview.chromium.org/1216833003/diff/20001/sky/sdk/lib/rendering/o...
sky/sdk/lib/rendering/object.dart:286: _debugDoingPaint = false;
On 2015/07/06 20:50:58, abarth-chromium wrote:
> We should wrap the body of this function in a try .. finally block to make
sure
> we always set _debugDoingPaint = false on the way out of this function if we
hit
> an exception.  If we don't set _debugDoingPaint = false, we'll be in a very
> corrupt state.

Done.

https://codereview.chromium.org/1216833003/diff/20001/sky/sdk/lib/rendering/o...
sky/sdk/lib/rendering/object.dart:297: } catch (e, stack) {
On 2015/07/06 20:50:58, abarth-chromium wrote:
> Capturing the stack here is going to be too expensive.  Can we just capture
|e|
> alone?

Done.

https://codereview.chromium.org/1216833003/diff/20001/sky/sdk/lib/rendering/o...
sky/sdk/lib/rendering/object.dart:297: } catch (e, stack) {
On 2015/07/06 21:13:42, Hixie wrote:
> On 2015/07/06 at 20:50:58, abarth-chromium wrote:
> > Capturing the stack here is going to be too expensive.  Can we just capture
> |e| alone?
> 
> If you change this make sure the equivalent code in layout is kept in sync.

Done.

https://codereview.chromium.org/1216833003/diff/20001/sky/sdk/lib/rendering/o...
sky/sdk/lib/rendering/object.dart:307:
canvas._descendentsWithPaintingCanvases.forEach((node) {
On 2015/07/06 20:50:57, abarth-chromium wrote:
> We should use a for-each loop rather than calling |forEach|.  Calling the
> forEach function is more expensive because we need to allocate a stack from
for
> each entry in the list:
> 
> for (RenderObject node in canvas._descendentsWithPaintingCanvases) {
>   ...
> }

Done.

https://codereview.chromium.org/1216833003/diff/20001/sky/sdk/lib/rendering/o...
sky/sdk/lib/rendering/object.dart:317: _needsPaint = false;
On 2015/07/06 20:50:58, abarth-chromium wrote:
> Why do we set _needsPaint = false both here and on line 294?  It seems like we
> can remove the set on line 294.

Done.

[abarth-chromium, 2015-07-07 20:49:39]

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

https://codereview.chromium.org/1216833003/diff/40001/sky/engine/core/paintin...
sky/engine/core/painting/PaintingNode.h:16: class PaintingNodeDrawable : public
SkDrawable {
One class per file, please.

https://codereview.chromium.org/1216833003/diff/40001/sky/engine/core/paintin...
sky/engine/core/painting/PaintingNode.h:20: ~PaintingNodeDrawable() override {}
Please move virtual (and override) implementations out of line.

[iansf, 2015-07-07 21:11:23]

Fixes for the comments I missed.

https://codereview.chromium.org/1216833003/diff/20001/sky/engine/core/paintin...
File sky/engine/core/painting/PaintingNode.cpp (right):

https://codereview.chromium.org/1216833003/diff/20001/sky/engine/core/paintin...
sky/engine/core/painting/PaintingNode.cpp:29: }
On 2015/07/06 20:50:57, abarth-chromium wrote:
> Rather than having two constructors, we could just have one that takes a
> PassRefPtr<SkDrawable>.  We can then delete PaintingNodeDrawable::create() and
> just have the other |create| function default to nullptr:
> 
> PaintingNodeDrawable::create(PassRefPtr<SkDrawable> skDrawable = nullptr)

Done.

https://codereview.chromium.org/1216833003/diff/20001/sky/engine/core/paintin...
File sky/engine/core/painting/PaintingNode.h (right):

https://codereview.chromium.org/1216833003/diff/20001/sky/engine/core/paintin...
sky/engine/core/painting/PaintingNode.h:21: ~PaintingNodeDrawable() override {}
On 2015/07/06 20:50:57, abarth-chromium wrote:
> nit: we usually put virtual destructors out-of-line.

Done.

https://codereview.chromium.org/1216833003/diff/20001/sky/engine/core/paintin...
sky/engine/core/painting/PaintingNode.h:23: SkRect onGetBounds();
On 2015/07/06 20:50:57, abarth-chromium wrote:
> override?

Done.

https://codereview.chromium.org/1216833003/diff/20001/sky/engine/core/paintin...
sky/engine/core/painting/PaintingNode.h:25: SkPicture* onNewPictureSnapshot();
On 2015/07/06 20:50:57, abarth-chromium wrote:
> override?

Done.

https://codereview.chromium.org/1216833003/diff/20001/sky/engine/core/paintin...
sky/engine/core/painting/PaintingNode.h:37: class PaintingNode : public
RefCounted<PaintingNode>, public DartWrappable {
On 2015/07/06 20:50:57, abarth-chromium wrote:
> We prefer one class per file.  Would you be willing to put these classes in
> separate files?

Done.

https://codereview.chromium.org/1216833003/diff/20001/sky/engine/core/paintin...
sky/engine/core/painting/PaintingNode.h:50: PassRefPtr<Picture>
newPictureSnapshot();
On 2015/07/06 20:50:57, abarth-chromium wrote:
> Does anyone call this function?  If not, we should remove it.

I made this public and put in into the idl, although it isn't being used yet in
any sample code or framework code.

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

https://codereview.chromium.org/1216833003/diff/20001/sky/sdk/lib/rendering/o...
sky/sdk/lib/rendering/object.dart:257: sky.PaintingNode _paintingNode = new
sky.PaintingNode(); // only set if createNewDisplayList is true
On 2015/07/06 20:50:58, abarth-chromium wrote:
> Without |final|, this call to new will happen all the time.  Maybe leave it
with
> a default value of |null| ?

I made this final -- it's never reassigned.

https://codereview.chromium.org/1216833003/diff/20001/sky/sdk/lib/rendering/o...
sky/sdk/lib/rendering/object.dart:261: bool get createNewDisplayList => true;
On 2015/07/06 20:50:58, abarth-chromium wrote:
> I'm not sure we want the default behavior to be to create a new display list. 
> Most RenderObjects don't paint anything at all, so that seems very wasteful.

Done.

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

https://codereview.chromium.org/1216833003/diff/40001/sky/engine/core/paintin...
sky/engine/core/painting/PaintingNode.h:16: class PaintingNodeDrawable : public
SkDrawable {
On 2015/07/07 20:49:39, abarth-chromium wrote:
> One class per file, please.

Done.

https://codereview.chromium.org/1216833003/diff/40001/sky/engine/core/paintin...
sky/engine/core/painting/PaintingNode.h:20: ~PaintingNodeDrawable() override {}
On 2015/07/07 20:49:39, abarth-chromium wrote:
> Please move virtual (and override) implementations out of line.

Done.

[abarth-chromium, 2015-07-07 21:22:29]

lgtm

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

https://codereview.chromium.org/1216833003/diff/10015/sky/sdk/lib/rendering/o...
sky/sdk/lib/rendering/object.dart:256: final sky.PaintingNode _paintingNode =
new sky.PaintingNode(); // only set if createNewDisplayList is true
I'd remove this comment and add assert(createNewDisplayList) to `get
paintingNode` below.

https://codereview.chromium.org/1216833003/diff/10015/sky/sdk/lib/rendering/o...
sky/sdk/lib/rendering/object.dart:269: } else if (parent != null) {
Can we make this if check into an assert?  The only RenderObject that doesn't
have a parent is the RenderView, which should set createNewDisplayList to true

[abarth-chromium, 2015-07-08 21:22:51]

lgtm

[iansf, 2015-07-08 22:08:28]

Committed patchset #8 (id:130001) manually as
e011e35539906e621f4cf4f1de2df4b5f93c3a98 (presubmit successful).