Make Mozart view manager use the new compositor.

Make Mozart view manager use the new compositor.

This patch updates the view manager to use the new compositor.

It also implements a few new features that were required to port all
of the UI examples to Mozart:

- Defined the ViewAssociate mechanism for offering services to views
  and view trees such as input dispatch.
- Added a skeleton implementation of an input manager service.  This
  initial implementation simply passes events dispatched through the
  view tree to the first view which registers to listen to them.
  Later on, we'll add hit testing and give input events a complete
  overhaul.
- Changed how scenes are associated with views (previously surfaces)
  to make the interface easier to use.  Clients only need to connect
  to the ViewManager to register their views; they don't need to
  connect to the Compositor since they can simply create the Scene
  through the ViewHost.  This approach is also much friendlier for
  multi-threaded clients.

The clients of the view manager are updated in following patches.

Some things we'll do later: refactor the view registry, design a
new input event dispatch protocol, invert how layout occurs to take
advantage of Scene versioning capabilities to avoid stalls, make
the View interface work more like Scene.

BUG=
R=abarth@google.com

Committed: https://chromium.googlesource.com/external/mojo/+/711b2f1e6d912e6514b3a4f74068104ddd79a4b5

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

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

[abarth, 2016-01-10 01:42:55]

LGTM

https://codereview.chromium.org/1552043002/diff/20001/mojo/services/ui/input/...
File mojo/services/ui/input/interfaces/input_connection.mojom (right):

https://codereview.chromium.org/1552043002/diff/20001/mojo/services/ui/input/...
mojo/services/ui/input/interfaces/input_connection.mojom:34: OnEvent(mojo.Event
event) => (bool handled);
I'd rename "handled" to "consumed" because that's what your comment says.  Other
systems have a tri-state result from event dispatching (i.e., ignored,
processed, and consumed) but we might not need that here.

https://codereview.chromium.org/1552043002/diff/20001/mojo/services/ui/input/...
File mojo/services/ui/input/interfaces/input_dispatcher.mojom (right):

https://codereview.chromium.org/1552043002/diff/20001/mojo/services/ui/input/...
mojo/services/ui/input/interfaces/input_dispatcher.mojom:39:
DispatchEvent(mojo.Event event);
I guess it's up to the InputDispatcher to decide who to dispatch the event to
(and in what order)?

https://codereview.chromium.org/1552043002/diff/20001/mojo/services/ui/views/...
File mojo/services/ui/views/interfaces/view_associates.mojom (right):

https://codereview.chromium.org/1552043002/diff/20001/mojo/services/ui/views/...
mojo/services/ui/views/interfaces/view_associates.mojom:23: // the same set of
associates.
That seems pretty limiting.

https://codereview.chromium.org/1552043002/diff/20001/mojo/services/ui/views/...
mojo/services/ui/views/interfaces/view_associates.mojom:42:
Connect(ViewInspector inspector) => (ViewAssociateInfo info);
It's interesting that you're doing initialization in this order.  My guess was
that you'd have the associates connect to the view manager and provide their
info.  It's hard to know what's better because we don't know the startup
sequence for the system.

https://codereview.chromium.org/1552043002/diff/20001/services/ui/input_manag...
File services/ui/input_manager/input_associate.h (right):

https://codereview.chromium.org/1552043002/diff/20001/services/ui/input_manag...
services/ui/input_manager/input_associate.h:19: explicit InputAssociate();
No need for "explicit" here.

https://codereview.chromium.org/1552043002/diff/20001/services/ui/launcher/la...
File services/ui/launcher/launcher_app.cc (right):

https://codereview.chromium.org/1552043002/diff/20001/services/ui/launcher/la...
services/ui/launcher/launcher_app.cc:28:
command_line->InitFromArgv(app_impl_->args());
Woah.  I guess that works.

https://codereview.chromium.org/1552043002/diff/20001/services/ui/launcher/la...
File services/ui/launcher/launcher_view_tree.cc (right):

https://codereview.chromium.org/1552043002/diff/20001/services/ui/launcher/la...
services/ui/launcher/launcher_view_tree.cc:36:
&LauncherViewTree::OnRendererConnectionError, base::Unretained(this)));
Do we not support lambdas here?

https://codereview.chromium.org/1552043002/diff/20001/services/ui/view_manage...
File services/ui/view_manager/view_associate_table.cc (right):

https://codereview.chromium.org/1552043002/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_associate_table.cc:19: }
Should we put this somewhere more general?

https://codereview.chromium.org/1552043002/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_associate_table.cc:49:
pending_connection_count_++;
I wonder if we can extract this pattern into something reusable.  In dynamic
language land, you get something like Future.wait(....) that does all the
bookkeeping for you.

https://codereview.chromium.org/1552043002/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_associate_table.cc:65: for (auto& data :
associates_) {
You didn't want to use a map?  I'm not sure what sort of scale you're expecting
here.

https://codereview.chromium.org/1552043002/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_associate_table.cc:128: void
ViewAssociateTable::CompleteDeferredWork() {
I'd add a DCHECK or a CHECK on pending_connection_count_ here.

https://codereview.chromium.org/1552043002/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_associate_table.cc:129: for (auto& work :
deferred_work_)
I'd probably use a stronger iteration pattern here.  If you have a local array
that you swap deferred_work_, then you can CHECK that nothing new gets added to
deferred_work_ during the iteration.

https://codereview.chromium.org/1552043002/diff/20001/services/ui/view_manage...
File services/ui/view_manager/view_manager_app.cc (right):

https://codereview.chromium.org/1552043002/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_manager_app.cc:47: std::vector<std::string>
associate_urls = command_line->GetArgs();
If you flip the registration of these associates, you can put this logic into
the launcher app.  IMHO, command line arguments are something of an antipattern
in Mojo apps.

https://codereview.chromium.org/1552043002/diff/20001/services/ui/view_manage...
File services/ui/view_manager/view_registry.h (right):

https://codereview.chromium.org/1552043002/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_registry.h:29:
ViewRegistry(mojo::gfx::composition::CompositorPtr compositor);
explicit

https://codereview.chromium.org/1552043002/diff/20001/services/ui/view_manage...
File services/ui/view_manager/view_state.cc (right):

https://codereview.chromium.org/1552043002/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_state.cc:54: mojo::ui::ViewLayoutInfoPtr
ViewState::CreateLayoutInfo() {
The difference between ViewLayoutInfo and ViewLayoutResult is confusing to me. 
Is there a better name for ViewLayoutInfo that is more descriptive than "info" ?

https://codereview.chromium.org/1552043002/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_state.cc:71: formatted_label_cache_ = s.str();
There's base::StringPrintf if you'd like something more concise.

https://codereview.chromium.org/1552043002/diff/20001/services/ui/view_manage...
File services/ui/view_manager/view_state.h (right):

https://codereview.chromium.org/1552043002/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_state.h:140: uint32_t key_ = 0u;
I'm not sure what the style guide says about this language feature.  I'm
assuming that it's ok if you're using it.

https://codereview.chromium.org/1552043002/diff/20001/services/ui/view_manage...
File services/ui/view_manager/view_tree_state.h (right):

https://codereview.chromium.org/1552043002/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_tree_state.h:83: std::string FormattedLabel();
If you've got a cache, you might as well return a const std::string&

[jeffbrown, 2016-01-26 05:59:13]

https://codereview.chromium.org/1552043002/diff/20001/mojo/services/ui/input/...
File mojo/services/ui/input/interfaces/input_connection.mojom (right):

https://codereview.chromium.org/1552043002/diff/20001/mojo/services/ui/input/...
mojo/services/ui/input/interfaces/input_connection.mojom:34: OnEvent(mojo.Event
event) => (bool handled);
On 2016/01/10 01:42:54, abarth wrote:
> I'd rename "handled" to "consumed" because that's what your comment says. 
Other
> systems have a tri-state result from event dispatching (i.e., ignored,
> processed, and consumed) but we might not need that here.

Done.

https://codereview.chromium.org/1552043002/diff/20001/mojo/services/ui/input/...
File mojo/services/ui/input/interfaces/input_dispatcher.mojom (right):

https://codereview.chromium.org/1552043002/diff/20001/mojo/services/ui/input/...
mojo/services/ui/input/interfaces/input_dispatcher.mojom:39:
DispatchEvent(mojo.Event event);
On 2016/01/10 01:42:54, abarth wrote:
> I guess it's up to the InputDispatcher to decide who to dispatch the event to
> (and in what order)?

Yup.  It owns all of the policy, this is just the entry point for events coming
into the system.

https://codereview.chromium.org/1552043002/diff/20001/mojo/services/ui/views/...
File mojo/services/ui/views/interfaces/view_associates.mojom (right):

https://codereview.chromium.org/1552043002/diff/20001/mojo/services/ui/views/...
mojo/services/ui/views/interfaces/view_associates.mojom:23: // the same set of
associates.
On 2016/01/10 01:42:54, abarth wrote:
> That seems pretty limiting.

Yes.  Deliberately so.  We'll see if this needs to change later.

https://codereview.chromium.org/1552043002/diff/20001/mojo/services/ui/views/...
mojo/services/ui/views/interfaces/view_associates.mojom:42:
Connect(ViewInspector inspector) => (ViewAssociateInfo info);
On 2016/01/10 01:42:54, abarth wrote:
> It's interesting that you're doing initialization in this order.  My guess was
> that you'd have the associates connect to the view manager and provide their
> info.  It's hard to know what's better because we don't know the startup
> sequence for the system.

Launching all of the associate services upfront eliminates a lot of ambiguity so
it's a good starting point.  For example, this makes it so we don't have to
handle the case of an application asking for a service which might be provided
by an associate eventually but that hasn't started yet.  I'm not sure
application developers are prepared for a world where their input system might
come and go on them, for example.

The purpose here is to prevent the view system from becoming a giant monolithic
mess, not necessarily to allow for independent fail-over of each piece.

https://codereview.chromium.org/1552043002/diff/20001/services/ui/input_manag...
File services/ui/input_manager/input_associate.h (right):

https://codereview.chromium.org/1552043002/diff/20001/services/ui/input_manag...
services/ui/input_manager/input_associate.h:19: explicit InputAssociate();
On 2016/01/10 01:42:54, abarth wrote:
> No need for "explicit" here.

Done.

https://codereview.chromium.org/1552043002/diff/20001/services/ui/launcher/la...
File services/ui/launcher/launcher_app.cc (right):

https://codereview.chromium.org/1552043002/diff/20001/services/ui/launcher/la...
services/ui/launcher/launcher_app.cc:28:
command_line->InitFromArgv(app_impl_->args());
On 2016/01/10 01:42:54, abarth wrote:
> Woah.  I guess that works.

(Nothing else does.)

https://codereview.chromium.org/1552043002/diff/20001/services/ui/launcher/la...
File services/ui/launcher/launcher_view_tree.cc (right):

https://codereview.chromium.org/1552043002/diff/20001/services/ui/launcher/la...
services/ui/launcher/launcher_view_tree.cc:36:
&LauncherViewTree::OnRendererConnectionError, base::Unretained(this)));
On 2016/01/10 01:42:54, abarth wrote:
> Do we not support lambdas here?

We do support lambdas but I've been avoiding mixing lambdas and base::Bind in
the same file.

https://codereview.chromium.org/1552043002/diff/20001/services/ui/view_manage...
File services/ui/view_manager/view_associate_table.cc (right):

https://codereview.chromium.org/1552043002/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_associate_table.cc:19: }
On 2016/01/10 01:42:55, abarth wrote:
> Should we put this somewhere more general?

I have a bug filed for making mojo::Array satisfy the standard library container
contract at which point this function won't be needed anymore.

https://codereview.chromium.org/1552043002/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_associate_table.cc:49:
pending_connection_count_++;
On 2016/01/10 01:42:55, abarth wrote:
> I wonder if we can extract this pattern into something reusable.  In dynamic
> language land, you get something like Future.wait(....) that does all the
> bookkeeping for you.

Yeah, some kind of chaining might be nice here.

https://codereview.chromium.org/1552043002/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_associate_table.cc:65: for (auto& data :
associates_) {
On 2016/01/10 01:42:55, abarth wrote:
> You didn't want to use a map?  I'm not sure what sort of scale you're
expecting
> here.

I'm just being lazy.  This is adequate for now and likely to get rewritten.

https://codereview.chromium.org/1552043002/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_associate_table.cc:128: void
ViewAssociateTable::CompleteDeferredWork() {
On 2016/01/10 01:42:55, abarth wrote:
> I'd add a DCHECK or a CHECK on pending_connection_count_ here.

Done.

https://codereview.chromium.org/1552043002/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_associate_table.cc:129: for (auto& work :
deferred_work_)
On 2016/01/10 01:42:55, abarth wrote:
> I'd probably use a stronger iteration pattern here.  If you have a local array
> that you swap deferred_work_, then you can CHECK that nothing new gets added
to
> deferred_work_ during the iteration.

Another trick is to swap() the vector with an empty one.  However here there's
an invariant which guarantees that nothing can be added to deferred_work_ once
pending_connection_count_ is 0.  Honestly I don't like this kind of deferred
work pattern anyhow so expect it to go away in a future version.

https://codereview.chromium.org/1552043002/diff/20001/services/ui/view_manage...
File services/ui/view_manager/view_manager_app.cc (right):

https://codereview.chromium.org/1552043002/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_manager_app.cc:47: std::vector<std::string>
associate_urls = command_line->GetArgs();
On 2016/01/10 01:42:55, abarth wrote:
> If you flip the registration of these associates, you can put this logic into
> the launcher app.  IMHO, command line arguments are something of an
antipattern
> in Mojo apps.

Command-line arguments are substituting for the lack of configuration files or
environment variables or any other means of persisting system configuration
data, so I'm tolerating this for now.

Adding a TODO to put a bit more thought into this.  Originally I was planning on
having the launcher do just as you suggest but technically the launcher can come
and go and in fact multiple instances could connect to the view manager at once
(each managing separate view trees).  So then we need to ask whether view
associates should be scoped to particular view trees or something and whether
views should be allowed to migrate between them (currently yes).

I suspect that the ViewManager interface will ultimately need to get split up
and different apps will see different instances in their environment depending
on where they live.  Hard to express that today given that Mojo Shell treats
everything like a singleton in the same universe.

https://codereview.chromium.org/1552043002/diff/20001/services/ui/view_manage...
File services/ui/view_manager/view_registry.h (right):

https://codereview.chromium.org/1552043002/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_registry.h:29:
ViewRegistry(mojo::gfx::composition::CompositorPtr compositor);
On 2016/01/10 01:42:55, abarth wrote:
> explicit

Done.

https://codereview.chromium.org/1552043002/diff/20001/services/ui/view_manage...
File services/ui/view_manager/view_state.cc (right):

https://codereview.chromium.org/1552043002/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_state.cc:54: mojo::ui::ViewLayoutInfoPtr
ViewState::CreateLayoutInfo() {
On 2016/01/10 01:42:55, abarth wrote:
> The difference between ViewLayoutInfo and ViewLayoutResult is confusing to me.

> Is there a better name for ViewLayoutInfo that is more descriptive than "info"
?

Agreed.  The whole layout protocol is going to be redone after I get this
checked in.

https://codereview.chromium.org/1552043002/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_state.cc:71: formatted_label_cache_ = s.str();
On 2016/01/10 01:42:55, abarth wrote:
> There's base::StringPrintf if you'd like something more concise.

Done.

https://codereview.chromium.org/1552043002/diff/20001/services/ui/view_manage...
File services/ui/view_manager/view_state.h (right):

https://codereview.chromium.org/1552043002/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_state.h:140: uint32_t key_ = 0u;
On 2016/01/10 01:42:55, abarth wrote:
> I'm not sure what the style guide says about this language feature.  I'm
> assuming that it's ok if you're using it.

You mean about inline initialization?  I can't find anything in the style guide
one way or the other.  It would seem odd to prohibit it.

https://codereview.chromium.org/1552043002/diff/20001/services/ui/view_manage...
File services/ui/view_manager/view_tree_state.h (right):

https://codereview.chromium.org/1552043002/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_tree_state.h:83: std::string FormattedLabel();
On 2016/01/10 01:42:55, abarth wrote:
> If you've got a cache, you might as well return a const std::string&

Done.

[jeffbrown, 2016-01-26 09:18:54]

apply review feedback

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

Description was changed from

==========
Make Mozart view manager use the new compositor.

This patch updates the view manager to use the new compositor.

It also implements a few new features that were required to port all
of the UI examples to Mozart:

- Defined the ViewAssociate mechanism for offering services to views
  and view trees such as input dispatch.
- Added a skeleton implementation of an input manager service.  This
  initial implementation simply passes events dispatched through the
  view tree to the first view which registers to listen to them.
  Later on, we'll add hit testing and give input events a complete
  overhaul.
- Changed how scenes are associated with views (previously surfaces)
  to make the interface easier to use.  Clients only need to connect
  to the ViewManager to register their views; they don't need to
  connect to the Compositor since they can simply create the Scene
  through the ViewHost.  This approach is also much friendlier for
  multi-threaded clients.

The clients of the view manager are updated in following patches.

Some things we'll do later: refactor the view registry, design a
new input event dispatch protocol, invert how layout occurs to take
advantage of Scene versioning capabilities to avoid stalls, make
the View interface work more like Scene.

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

to

==========
Make Mozart view manager use the new compositor.

This patch updates the view manager to use the new compositor.

It also implements a few new features that were required to port all
of the UI examples to Mozart:

- Defined the ViewAssociate mechanism for offering services to views
  and view trees such as input dispatch.
- Added a skeleton implementation of an input manager service.  This
  initial implementation simply passes events dispatched through the
  view tree to the first view which registers to listen to them.
  Later on, we'll add hit testing and give input events a complete
  overhaul.
- Changed how scenes are associated with views (previously surfaces)
  to make the interface easier to use.  Clients only need to connect
  to the ViewManager to register their views; they don't need to
  connect to the Compositor since they can simply create the Scene
  through the ViewHost.  This approach is also much friendlier for
  multi-threaded clients.

The clients of the view manager are updated in following patches.

Some things we'll do later: refactor the view registry, design a
new input event dispatch protocol, invert how layout occurs to take
advantage of Scene versioning capabilities to avoid stalls, make
the View interface work more like Scene.

BUG=
R=abarth@google.com

Committed:
https://chromium.googlesource.com/external/mojo/+/711b2f1e6d912e6514b3a4f7406...
==========

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

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