mozart: Port the spinning cube example.

mozart: Port the spinning cube example.

Initial port of the spinning cube example to the Mozart view system.

Note: This version does not support input yet.

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

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

[jeffbrown, 2015-10-24 02:58:35]

jeffbrown@google.com changed reviewers:
+ abarth@google.com, jamesr@chromium.org

[abarth, 2015-10-24 06:17:36]

LGTM

https://codereview.chromium.org/1411103004/diff/1/examples/ui/spinning_cube/s...
File examples/ui/spinning_cube/spinning_cube_view.cc (right):

https://codereview.chromium.org/1411103004/diff/1/examples/ui/spinning_cube/s...
examples/ui/spinning_cube/spinning_cube_view.cc:52: mojo::Array<uint32_t>
children_needing_layout,
Should we DCHECK something about |children_needing_layout| ?

https://codereview.chromium.org/1411103004/diff/1/examples/ui/spinning_cube/s...
examples/ui/spinning_cube/spinning_cube_view.cc:121: glClearColor(1, 0, 0, 0.5);
Should this GL boilerplate go somewhere common (e.g., in |SpinningCube|)?  I
suspect it's duplicated in several places by now.

https://codereview.chromium.org/1411103004/diff/1/examples/ui/spinning_cube/s...
examples/ui/spinning_cube/spinning_cube_view.cc:155:
base::TimeDelta::FromMilliseconds(30));
Here you're using a delay but in |launcher| you go right away when
OnSurfaceSubmitted is called.  Does that mean the |launcher| will submit frames
faster than vsync?

[jeffbrown, 2015-10-27 01:58:32]

https://codereview.chromium.org/1411103004/diff/1/examples/ui/spinning_cube/s...
File examples/ui/spinning_cube/spinning_cube_view.cc (right):

https://codereview.chromium.org/1411103004/diff/1/examples/ui/spinning_cube/s...
examples/ui/spinning_cube/spinning_cube_view.cc:52: mojo::Array<uint32_t>
children_needing_layout,
On 2015/10/24 06:17:36, abarth wrote:
> Should we DCHECK something about |children_needing_layout| ?

Not much to DCHECK here.  I think the validator will ensure it's non-null and
it's ok for it to be empty.  We're already assuming the view manager provided a
valid list of children (wouldn't make sense for each client to have to check).

https://codereview.chromium.org/1411103004/diff/1/examples/ui/spinning_cube/s...
examples/ui/spinning_cube/spinning_cube_view.cc:121: glClearColor(1, 0, 0, 0.5);
On 2015/10/24 06:17:36, abarth wrote:
> Should this GL boilerplate go somewhere common (e.g., in |SpinningCube|)?  I
> suspect it's duplicated in several places by now.

Perhaps.  But I'm hoping this will be replaced by some kind of swap chain /
buffer queue abstraction that does away with it entirely.

https://codereview.chromium.org/1411103004/diff/1/examples/ui/spinning_cube/s...
examples/ui/spinning_cube/spinning_cube_view.cc:155:
base::TimeDelta::FromMilliseconds(30));
On 2015/10/24 06:17:36, abarth wrote:
> Here you're using a delay but in |launcher| you go right away when
> OnSurfaceSubmitted is called.  Does that mean the |launcher| will submit
frames
> faster than vsync?

Ahh, I should add a comment on this.  This is to keep spinning cube from
completely overwhelming X11 today.

[jamesr, 2015-10-27 23:04:23]

lgtm w/ some comments

https://codereview.chromium.org/1411103004/diff/20001/examples/ui/spinning_cu...
File examples/ui/spinning_cube/spinning_cube_app.h (right):

https://codereview.chromium.org/1411103004/diff/20001/examples/ui/spinning_cu...
examples/ui/spinning_cube/spinning_cube_app.h:41: mojo::ApplicationImpl*
app_impl_;
put these in private (you may want to put all of the overrides in private,
there's no reason for them to be callable from outside on a SpinningCubeApp* )

https://codereview.chromium.org/1411103004/diff/20001/examples/ui/spinning_cu...
File examples/ui/spinning_cube/spinning_cube_view.cc (right):

https://codereview.chromium.org/1411103004/diff/20001/examples/ui/spinning_cu...
examples/ui/spinning_cube/spinning_cube_view.cc:71: mojo::ui::ViewLayoutInfoPtr
info = mojo::ui::ViewLayoutInfo::New();
i pretty much use 'auto' with these ::New() factories since the type name is
spelled out in full on the RHS, but that's personal preference

https://codereview.chromium.org/1411103004/diff/20001/examples/ui/spinning_cu...
examples/ui/spinning_cube/spinning_cube_view.cc:95:
context_owner_.context()->MakeCurrent();
unless the context has to be current for the GetTexture() call i'd move this
below the early return to avoid churning the context in the failure case (it
obviously doesn't really matter in this example but people will copy-paste)

https://codereview.chromium.org/1411103004/diff/20001/examples/ui/spinning_cu...
File examples/ui/spinning_cube/spinning_cube_view.h (right):

https://codereview.chromium.org/1411103004/diff/20001/examples/ui/spinning_cu...
examples/ui/spinning_cube/spinning_cube_view.h:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
2015 ?

[jeffbrown, 2015-10-27 23:48:16]

https://codereview.chromium.org/1411103004/diff/20001/examples/ui/spinning_cu...
File examples/ui/spinning_cube/spinning_cube_app.h (right):

https://codereview.chromium.org/1411103004/diff/20001/examples/ui/spinning_cu...
examples/ui/spinning_cube/spinning_cube_app.h:41: mojo::ApplicationImpl*
app_impl_;
On 2015/10/27 23:04:23, jamesr wrote:
> put these in private (you may want to put all of the overrides in private,
> there's no reason for them to be callable from outside on a SpinningCubeApp* )

Yup.  Just an oversight.

https://codereview.chromium.org/1411103004/diff/20001/examples/ui/spinning_cu...
File examples/ui/spinning_cube/spinning_cube_view.cc (right):

https://codereview.chromium.org/1411103004/diff/20001/examples/ui/spinning_cu...
examples/ui/spinning_cube/spinning_cube_view.cc:71: mojo::ui::ViewLayoutInfoPtr
info = mojo::ui::ViewLayoutInfo::New();
On 2015/10/27 23:04:23, jamesr wrote:
> i pretty much use 'auto' with these ::New() factories since the type name is
> spelled out in full on the RHS, but that's personal preference

Sure!  That makes sense.

https://codereview.chromium.org/1411103004/diff/20001/examples/ui/spinning_cu...
examples/ui/spinning_cube/spinning_cube_view.cc:95:
context_owner_.context()->MakeCurrent();
On 2015/10/27 23:04:23, jamesr wrote:
> unless the context has to be current for the GetTexture() call i'd move this
> below the early return to avoid churning the context in the failure case (it
> obviously doesn't really matter in this example but people will copy-paste)

It's needed by GetTexture although arguably that's a bug.  I'll go fix that.

https://codereview.chromium.org/1411103004/diff/20001/examples/ui/spinning_cu...
File examples/ui/spinning_cube/spinning_cube_view.h (right):

https://codereview.chromium.org/1411103004/diff/20001/examples/ui/spinning_cu...
examples/ui/spinning_cube/spinning_cube_view.h:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
On 2015/10/27 23:04:23, jamesr wrote:
> 2015 ?

Fixed here and in many other places.

[jeffbrown, 2015-10-28 01:47:42]

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