Add helpers for creating UI components.

Add helpers for creating UI components.

These helpers make it easier to use Mozart in C++ and greatly
reduce the amount of boilerplate involved in writing simple
applications.

View implementations:

- BaseView: A base implementation of the View interface.
- GLView: A View with an associated GLRenderer which takes care
  of allocating, binding, and recycling textures.
- GaneshView: A View with an associated GaneshRenderer which takes
  care of setting up a GaneshContext and drawing to canvas.

View providers:

- ViewProviderApp: Skeleton of a simple app which offers the
  ViewProvider interface and vends Views on demand.
- ContextViewerApp: Skeleton of a simple app which offers the
  ContentHandler interface and vends ViewProviders on demand.

Helpers:

- Choreographer: Coordinates the scheduling of drawing operations
  on behalf of a View and compensates for lag.
- InputHandler: Binds an InputListener on behalf of a View.

BUG=
R=abarth@google.com, viettrungluu@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/7c8fe6539eb6dc65f2030d9008d2ea65b28be543

[jeffbrown, 2015-12-31 11:20:23]

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

[viettrungluu, 2016-01-05 21:35:17]

https://codereview.chromium.org/1556803002/diff/20001/mojo/ui/base_view.h
File mojo/ui/base_view.h (right):

https://codereview.chromium.org/1556803002/diff/20001/mojo/ui/base_view.h#new...
mojo/ui/base_view.h:23: // Subclasses must handle layout and provide content for
the scene.
It'd be nice to have a comment (maybe in the class itself) indicating exactly
what method needs to be implemented.

Ditto for GaneshView (whence I came).

(I really got here from reading examples::ShapesView. Admittedly, such comments
would be fragile, but having to chase things up the class hierarchy is a bit
annoying.)

[viettrungluu, 2016-01-05 21:53:26]

https://codereview.chromium.org/1556803002/diff/20001/mojo/ui/ganesh_renderer.cc
File mojo/ui/ganesh_renderer.cc (right):

https://codereview.chromium.org/1556803002/diff/20001/mojo/ui/ganesh_renderer...
mojo/ui/ganesh_renderer.cc:28: const DrawSurfaceCallback& callback) {
It may just be me (and my naivete), but I'm a bit ambivalent about this API, in
particular having a callback that you always call inline.

E.g., in reading ShapesView::UpdateScene(), I got to the call to DrawSurface()
and then wondered when the callback (ShapesView::DrawContent()) would be run and
whether I should worry about object lifetime, etc.

I got the feeling that just having this code directly inline in
ShapesView::UpdateScene() would have been a little (but not a lot! -- since
using this helper class then required a separate method, etc.) more verbose, but
more obvious. It makes me wonder a bit about the value of this class.

https://codereview.chromium.org/1556803002/diff/20001/mojo/ui/ganesh_renderer.h
File mojo/ui/ganesh_renderer.h (right):

https://codereview.chromium.org/1556803002/diff/20001/mojo/ui/ganesh_renderer...
mojo/ui/ganesh_renderer.h:34: GaneshRenderer(mojo::skia::GaneshContext*
ganesh_context);
nit: |explicit|

[abarth, 2016-01-10 23:23:39]

Looks like you can remove the base dependency pretty easily from everything
except GaneshView.  I'd do that and put the code without a base dependency in
//mojo/services/ui/cpp.

LGTM but you probably want trung to finish his review.

https://codereview.chromium.org/1556803002/diff/20001/mojo/ui/BUILD.gn
File mojo/ui/BUILD.gn (right):

https://codereview.chromium.org/1556803002/diff/20001/mojo/ui/BUILD.gn#newcode20
mojo/ui/BUILD.gn:20: "//base",
The base dependency here is sad panda.  That basically means that no one else
can use this code.  How close are we to removing the base dependency?

https://codereview.chromium.org/1556803002/diff/20001/mojo/ui/content_viewer_...
File mojo/ui/content_viewer_app.cc (right):

https://codereview.chromium.org/1556803002/diff/20001/mojo/ui/content_viewer_...
mojo/ui/content_viewer_app.cc:43: auto command_line =
base::CommandLine::ForCurrentProcess();
I'd remove this interaction with the CommandLine static from these helper
classes.  We shouldn't make it easier for folks to fall into this antipattern.

https://codereview.chromium.org/1556803002/diff/20001/mojo/ui/ganesh_renderer.cc
File mojo/ui/ganesh_renderer.cc (right):

https://codereview.chromium.org/1556803002/diff/20001/mojo/ui/ganesh_renderer...
mojo/ui/ganesh_renderer.cc:28: const DrawSurfaceCallback& callback) {
On 2016/01/05 at 21:53:26, viettrungluu wrote:
> It may just be me (and my naivete), but I'm a bit ambivalent about this API,
in particular having a callback that you always call inline.
> 
> E.g., in reading ShapesView::UpdateScene(), I got to the call to DrawSurface()
and then wondered when the callback (ShapesView::DrawContent()) would be run and
whether I should worry about object lifetime, etc.
> 
> I got the feeling that just having this code directly inline in
ShapesView::UpdateScene() would have been a little (but not a lot! -- since
using this helper class then required a separate method, etc.) more verbose, but
more obvious. It makes me wonder a bit about the value of this class.

Yeah, I'd skip this function.  It's just making the callers more mysterious
without really adding any value.

https://codereview.chromium.org/1556803002/diff/20001/mojo/ui/input_handler.cc
File mojo/ui/input_handler.cc (right):

https://codereview.chromium.org/1556803002/diff/20001/mojo/ui/input_handler.c...
mojo/ui/input_handler.cc:26: InputHandler::~InputHandler() {}
I'd skip this class too.  It's just obfuscating what's going on.

[jeffbrown, 2016-01-26 07:25:15]

https://codereview.chromium.org/1556803002/diff/20001/mojo/ui/BUILD.gn
File mojo/ui/BUILD.gn (right):

https://codereview.chromium.org/1556803002/diff/20001/mojo/ui/BUILD.gn#newcode20
mojo/ui/BUILD.gn:20: "//base",
On 2016/01/10 23:23:39, abarth wrote:
> The base dependency here is sad panda.  That basically means that no one else
> can use this code.  How close are we to removing the base dependency?

I'll revisit this later.  The SDK story as a whole is pretty weak right now.

https://codereview.chromium.org/1556803002/diff/20001/mojo/ui/base_view.h
File mojo/ui/base_view.h (right):

https://codereview.chromium.org/1556803002/diff/20001/mojo/ui/base_view.h#new...
mojo/ui/base_view.h:23: // Subclasses must handle layout and provide content for
the scene.
On 2016/01/05 21:35:17, viettrungluu wrote:
> It'd be nice to have a comment (maybe in the class itself) indicating exactly
> what method needs to be implemented.
> 
> Ditto for GaneshView (whence I came).
> 
> (I really got here from reading examples::ShapesView. Admittedly, such
comments
> would be fragile, but having to chase things up the class hierarchy is a bit
> annoying.)

The View mojom is going to change shortly so for now I'll just add a comment
saying go look there.

https://codereview.chromium.org/1556803002/diff/20001/mojo/ui/content_viewer_...
File mojo/ui/content_viewer_app.cc (right):

https://codereview.chromium.org/1556803002/diff/20001/mojo/ui/content_viewer_...
mojo/ui/content_viewer_app.cc:43: auto command_line =
base::CommandLine::ForCurrentProcess();
On 2016/01/10 23:23:39, abarth wrote:
> I'd remove this interaction with the CommandLine static from these helper
> classes.  We shouldn't make it easier for folks to fall into this antipattern.

I'm going to leave this in for now since it's super useful for examples and
debugging (there's no way at all to set debug log verbosity otherwise). 
Honestly I shouldn't have to write this code at all.  It should be handled by
the ApplicationRunnerChromium as part of setting up the environment but we'll
need to refactor that first.

Created issue #633 to track that problem.

https://codereview.chromium.org/1556803002/diff/20001/mojo/ui/ganesh_renderer.cc
File mojo/ui/ganesh_renderer.cc (right):

https://codereview.chromium.org/1556803002/diff/20001/mojo/ui/ganesh_renderer...
mojo/ui/ganesh_renderer.cc:28: const DrawSurfaceCallback& callback) {
On 2016/01/10 23:23:39, abarth wrote:
> On 2016/01/05 at 21:53:26, viettrungluu wrote:
> > It may just be me (and my naivete), but I'm a bit ambivalent about this API,
> in particular having a callback that you always call inline.
> > 
> > E.g., in reading ShapesView::UpdateScene(), I got to the call to
DrawSurface()
> and then wondered when the callback (ShapesView::DrawContent()) would be run
and
> whether I should worry about object lifetime, etc.
> > 
> > I got the feeling that just having this code directly inline in
> ShapesView::UpdateScene() would have been a little (but not a lot! -- since
> using this helper class then required a separate method, etc.) more verbose,
but
> more obvious. It makes me wonder a bit about the value of this class.
> 
> Yeah, I'd skip this function.  It's just making the callers more mysterious
> without really adding any value.

There are some subtle things going on here such as the fact that the texture is
allocated before entering the Ganesh scope.  (And I just noticed that the call
to BindTextureResource should have happened outside of that scope.)

In practice, writing the drawing code inline obfuscates the little texture
ownership dance so you'll usually want to factor that out to another function
anyhow.  I agree it looks ugly with base::Bind but perhaps not so bad with a
lambda.

Anyhow, I'd like to work on a few more examples before deciding to ditch the
class entirely.  As it is, this pattern has proven to be quite useful so I'm
going to leave it in place for now.

I might still decide I hate it later.

(Historical note: Originally this was part of GaneshView with a simple OnDraw
method to override but coupling rendering to the View proved too restrictive so
I ended up with the callback approach instead to enable broader reuse.)

https://codereview.chromium.org/1556803002/diff/20001/mojo/ui/input_handler.cc
File mojo/ui/input_handler.cc (right):

https://codereview.chromium.org/1556803002/diff/20001/mojo/ui/input_handler.c...
mojo/ui/input_handler.cc:26: InputHandler::~InputHandler() {}
On 2016/01/10 23:23:39, abarth wrote:
> I'd skip this class too.  It's just obfuscating what's going on.

This is about to get much more interesting (we'll be decoding events to some
extent).  Leaving it for now.

[jeffbrown, 2016-01-26 09:21:07]

address feedback

[viettrungluu, 2016-01-26 18:38:40]

lgtm for now

https://codereview.chromium.org/1556803002/diff/20001/mojo/ui/ganesh_renderer.cc
File mojo/ui/ganesh_renderer.cc (right):

https://codereview.chromium.org/1556803002/diff/20001/mojo/ui/ganesh_renderer...
mojo/ui/ganesh_renderer.cc:28: const DrawSurfaceCallback& callback) {
On 2016/01/26 07:25:15, jeffbrown wrote:
> On 2016/01/10 23:23:39, abarth wrote:
> > On 2016/01/05 at 21:53:26, viettrungluu wrote:
> > > It may just be me (and my naivete), but I'm a bit ambivalent about this
API,
> > in particular having a callback that you always call inline.
> > > 
> > > E.g., in reading ShapesView::UpdateScene(), I got to the call to
> DrawSurface()
> > and then wondered when the callback (ShapesView::DrawContent()) would be run
> and
> > whether I should worry about object lifetime, etc.
> > > 
> > > I got the feeling that just having this code directly inline in
> > ShapesView::UpdateScene() would have been a little (but not a lot! -- since
> > using this helper class then required a separate method, etc.) more verbose,
> but
> > more obvious. It makes me wonder a bit about the value of this class.
> > 
> > Yeah, I'd skip this function.  It's just making the callers more mysterious
> > without really adding any value.
> 
> There are some subtle things going on here such as the fact that the texture
is
> allocated before entering the Ganesh scope.  (And I just noticed that the call
> to BindTextureResource should have happened outside of that scope.)
> 
> In practice, writing the drawing code inline obfuscates the little texture
> ownership dance so you'll usually want to factor that out to another function
> anyhow.  I agree it looks ugly with base::Bind but perhaps not so bad with a
> lambda.

To me the disadvantages are that it's not obvious (upon reading) when the
callback gets called, and that you may have to capture various local variables
(unless you use C++'s "capture everything that's used", which we don't like,
probably for good reason).

> 
> Anyhow, I'd like to work on a few more examples before deciding to ditch the
> class entirely.  As it is, this pattern has proven to be quite useful so I'm
> going to leave it in place for now.

I wonder if a scoper type thing wouldn't be better.

> 
> I might still decide I hate it later.
> 
> (Historical note: Originally this was part of GaneshView with a simple OnDraw
> method to override but coupling rendering to the View proved too restrictive
so
> I ended up with the callback approach instead to enable broader reuse.)

[jeffbrown, 2016-01-26 23:51:55]

Description was changed from

==========
Add helpers for creating UI components.

These helpers make it easier to use Mozart in C++ and greatly
reduce the amount of boilerplate involved in writing simple
applications.

View implementations:

- BaseView: A base implementation of the View interface.
- GLView: A View with an associated GLRenderer which takes care
  of allocating, binding, and recycling textures.
- GaneshView: A View with an associated GaneshRenderer which takes
  care of setting up a GaneshContext and drawing to canvas.

View providers:

- ViewProviderApp: Skeleton of a simple app which offers the
  ViewProvider interface and vends Views on demand.
- ContextViewerApp: Skeleton of a simple app which offers the
  ContentHandler interface and vends ViewProviders on demand.

Helpers:

- Choreographer: Coordinates the scheduling of drawing operations
  on behalf of a View and compensates for lag.
- InputHandler: Binds an InputListener on behalf of a View.

BUG=
==========

to

==========
Add helpers for creating UI components.

These helpers make it easier to use Mozart in C++ and greatly
reduce the amount of boilerplate involved in writing simple
applications.

View implementations:

- BaseView: A base implementation of the View interface.
- GLView: A View with an associated GLRenderer which takes care
  of allocating, binding, and recycling textures.
- GaneshView: A View with an associated GaneshRenderer which takes
  care of setting up a GaneshContext and drawing to canvas.

View providers:

- ViewProviderApp: Skeleton of a simple app which offers the
  ViewProvider interface and vends Views on demand.
- ContextViewerApp: Skeleton of a simple app which offers the
  ContentHandler interface and vends ViewProviders on demand.

Helpers:

- Choreographer: Coordinates the scheduling of drawing operations
  on behalf of a View and compensates for lag.
- InputHandler: Binds an InputListener on behalf of a View.

BUG=
R=abarth@google.com, viettrungluu@chromium.org

Committed:
https://chromium.googlesource.com/external/mojo/+/7c8fe6539eb6dc65f2030d9008d...
==========

[jeffbrown, 2016-01-26 23:51:56]

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