mozart: Initial commit of the view manager.

mozart: Initial commit of the view manager.

The view manager implements the ViewManager interface and maintains
the View hierarchy on behalf of applications.

Note: This version does not support input yet.

Committed: https://chromium.googlesource.com/external/mojo/+/0b1c404b97aa59f787a0c6d2ab45dc5f3c633c2c

[jeffbrown, 2015-10-24 02:39:45]

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

[jamesr, 2015-10-26 18:18:41]

https://codereview.chromium.org/1415493003/diff/1/services/ui/view_manager/BU...
File services/ui/view_manager/BUILD.gn (right):

https://codereview.chromium.org/1415493003/diff/1/services/ui/view_manager/BU...
services/ui/view_manager/BUILD.gn:8:
mojo_native_application("view_manager_service") {
nit: the GN target name here is redundant

//services/ui/view_manager:view_manager_service

as a shorthand GN lets you refer to a target whose name matches the containing
directory by saying just the directory name. thus, you should name this target
'view_manager' so the target can be depended on like

//services/ui/view_manager

which isn't redundant. you can then use the 'output_name' GN variable to control
the name of the final binary so you can connect to this app with the name
'view_manager_service', if desired, like this

mojo_native_application('view_manager') {
  output_name = 'view_manager_service'
  ...

[jeffbrown, 2015-10-27 01:52:11]

https://codereview.chromium.org/1415493003/diff/1/services/ui/view_manager/BU...
File services/ui/view_manager/BUILD.gn (right):

https://codereview.chromium.org/1415493003/diff/1/services/ui/view_manager/BU...
services/ui/view_manager/BUILD.gn:8:
mojo_native_application("view_manager_service") {
On 2015/10/26 18:18:41, jamesr wrote:
> nit: the GN target name here is redundant
> 
> //services/ui/view_manager:view_manager_service
> 
> as a shorthand GN lets you refer to a target whose name matches the containing
> directory by saying just the directory name. thus, you should name this target
> 'view_manager' so the target can be depended on like
> 
> //services/ui/view_manager
> 
> which isn't redundant. you can then use the 'output_name' GN variable to
control
> the name of the final binary so you can connect to this app with the name
> 'view_manager_service', if desired, like this
> 
> mojo_native_application('view_manager') {
>   output_name = 'view_manager_service'
>   ...

Done.

[jeffbrown, 2015-10-27 22:59:47]

I'm going to submit this patch as-is.  Please feel free to comment on the code
and I'll address those comments in subsequent patches while I refine the layout
model.

[jamesr, 2015-10-27 23:13:11]

There's a bunch of style / C++ issues in this patch that I think you should
address before landing as they're mostly orthogonal to the layout model changes

https://codereview.chromium.org/1415493003/diff/1/services/ui/view_manager/su...
File services/ui/view_manager/surface_manager.h (right):

https://codereview.chromium.org/1415493003/diff/1/services/ui/view_manager/su...
services/ui/view_manager/surface_manager.h:16: SurfaceManager(mojo::SurfacePtr
surfaces);
please add 'explicit' to all one-argument constructors unless you really want
implicit conversion (which is very rarely the case). c++ sadly has the wrong
default :/

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

https://codereview.chromium.org/1415493003/diff/20001/services/ui/view_manage...
services/ui/view_manager/surface_manager.cc:15: : surfaces_(surfaces.Pass()),
surface_namespace_(0) {}
initialized unsigneds with '0u' please

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

https://codereview.chromium.org/1415493003/diff/20001/services/ui/view_manage...
services/ui/view_manager/surface_manager.h:16: SurfaceManager(mojo::SurfacePtr
surfaces);
explicit

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

https://codereview.chromium.org/1415493003/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_layout_request.cc:29: for (auto callback :
callbacks_)
const auto&, you don't wanna copy the callback every time you go through the
loop (And .Run() is const)

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

https://codereview.chromium.org/1415493003/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_layout_request.h:17: typedef
base::Callback<void(mojo::ui::ViewLayoutInfoPtr)> ViewLayoutCallback;
prefer 'using' for this sort of thing, it looks slightly saner (since it's like
other type / variable declarations)

https://codereview.chromium.org/1415493003/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_layout_request.h:22:
ViewLayoutRequest(mojo::ui::ViewLayoutParamsPtr layout_params);
explicit

https://codereview.chromium.org/1415493003/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_layout_request.h:24: // Dispatches null layout
info automatically if DispatchLayoutInfo was not
hmm, side-effecty destructors on types that also do other things (i.e. aren't
pure RAII types) is iffy

https://codereview.chromium.org/1415493003/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_layout_request.h:51: bool issued;
private, no public members on classes (add a getter if you need one, or split
mutable data members into a struct if you need that)

https://codereview.chromium.org/1415493003/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_layout_request.h:53: mojo::ui::ViewLayoutParamsPtr
layout_params_;
trailing _ in particular should never be public

https://codereview.chromium.org/1415493003/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_layout_request.h:57:
DISALLOW_COPY_AND_ASSIGN(ViewLayoutRequest);
and this only makes sense in a private section

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

https://codereview.chromium.org/1415493003/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_manager_impl.h:18: ViewManagerImpl(ViewRegistry*
registry);
explicit

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

https://codereview.chromium.org/1415493003/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_registry.cc:76: :
surface_manager_(surface_manager), next_view_token_value_(1) {}
1u

https://codereview.chromium.org/1415493003/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_registry.cc:167: view_trees_.erase(it);
erase-remove would be slightly more idiomatic, i.e.

view_trees_.erase(std::remove_if(....))

although it has some annoyances too (stupid STL)

https://codereview.chromium.org/1415493003/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_registry.cc:653: if (view_state) {
early return would be easier to read than this big block (it's really hard to
see if this thing ends or not)

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

https://codereview.chromium.org/1415493003/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_registry.h:26: ViewRegistry(SurfaceManager*
surface_manager);
explicit

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

https://codereview.chromium.org/1415493003/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_state.h:24: class ViewState : public
base::SupportsWeakPtr<ViewState> {
you should never inherit from base::SupportsWeakPtr<>. doing so obscures exactly
when the weak factory goes away since it's hidden in a base type instead of
being a member. compose a base::WeakPtrFactory<> and expose an appropriate
GetWeak() on the type

https://codereview.chromium.org/1415493003/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_state.h:72: std::unordered_map<uint32_t,
ViewState*> children;
no public members

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

https://codereview.chromium.org/1415493003/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_tree_state.h:23: class ViewTreeState : public
base::SupportsWeakPtr<ViewTreeState> {
ditto re SupportsWeak

https://codereview.chromium.org/1415493003/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_tree_state.h:25:
ViewTreeState(mojo::ui::ViewTreePtr view_tree);
explicit

https://codereview.chromium.org/1415493003/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_tree_state.h:54: bool root_was_set;
public member

[jeffbrown, 2015-10-28 01:37:29]

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

https://codereview.chromium.org/1415493003/diff/20001/services/ui/view_manage...
services/ui/view_manager/surface_manager.cc:15: : surfaces_(surfaces.Pass()),
surface_namespace_(0) {}
On 2015/10/27 23:13:10, jamesr wrote:
> initialized unsigneds with '0u' please

Done.

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

https://codereview.chromium.org/1415493003/diff/20001/services/ui/view_manage...
services/ui/view_manager/surface_manager.h:16: SurfaceManager(mojo::SurfacePtr
surfaces);
On 2015/10/27 23:13:10, jamesr wrote:
> explicit

Done.

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

https://codereview.chromium.org/1415493003/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_layout_request.cc:29: for (auto callback :
callbacks_)
On 2015/10/27 23:13:10, jamesr wrote:
> const auto&, you don't wanna copy the callback every time you go through the
> loop (And .Run() is const)

Done.

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

https://codereview.chromium.org/1415493003/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_layout_request.h:17: typedef
base::Callback<void(mojo::ui::ViewLayoutInfoPtr)> ViewLayoutCallback;
On 2015/10/27 23:13:10, jamesr wrote:
> prefer 'using' for this sort of thing, it looks slightly saner (since it's
like
> other type / variable declarations)

Done.

https://codereview.chromium.org/1415493003/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_layout_request.h:22:
ViewLayoutRequest(mojo::ui::ViewLayoutParamsPtr layout_params);
On 2015/10/27 23:13:10, jamesr wrote:
> explicit

Done.

https://codereview.chromium.org/1415493003/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_layout_request.h:24: // Dispatches null layout
info automatically if DispatchLayoutInfo was not
On 2015/10/27 23:13:10, jamesr wrote:
> hmm, side-effecty destructors on types that also do other things (i.e. aren't
> pure RAII types) is iffy

Agreed.  The side-effect could go at the point where the view is released
instead.  Going to leave this alone for now since this code will probably be
deleted in the next patch.

https://codereview.chromium.org/1415493003/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_layout_request.h:51: bool issued;
On 2015/10/27 23:13:10, jamesr wrote:
> private, no public members on classes (add a getter if you need one, or split
> mutable data members into a struct if you need that)

Got it.  Skipping this one since this code is doomed.

https://codereview.chromium.org/1415493003/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_layout_request.h:53: mojo::ui::ViewLayoutParamsPtr
layout_params_;
On 2015/10/27 23:13:10, jamesr wrote:
> trailing _ in particular should never be public

Somewhere along the way I dropped the private: line

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

https://codereview.chromium.org/1415493003/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_manager_impl.h:18: ViewManagerImpl(ViewRegistry*
registry);
On 2015/10/27 23:13:11, jamesr wrote:
> explicit

Done.

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

https://codereview.chromium.org/1415493003/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_registry.cc:76: :
surface_manager_(surface_manager), next_view_token_value_(1) {}
On 2015/10/27 23:13:11, jamesr wrote:
> 1u

Done.

https://codereview.chromium.org/1415493003/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_registry.cc:167: view_trees_.erase(it);
On 2015/10/27 23:13:11, jamesr wrote:
> erase-remove would be slightly more idiomatic, i.e.
> 
> view_trees_.erase(std::remove_if(....))
> 
> although it has some annoyances too (stupid STL)

There's only one match here so I can use erase(find_if(...));

https://codereview.chromium.org/1415493003/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_registry.cc:653: if (view_state) {
On 2015/10/27 23:13:11, jamesr wrote:
> early return would be easier to read than this big block (it's really hard to
> see if this thing ends or not)

Done.

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

https://codereview.chromium.org/1415493003/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_registry.h:26: ViewRegistry(SurfaceManager*
surface_manager);
On 2015/10/27 23:13:11, jamesr wrote:
> explicit

Done.

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

https://codereview.chromium.org/1415493003/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_state.h:24: class ViewState : public
base::SupportsWeakPtr<ViewState> {
On 2015/10/27 23:13:11, jamesr wrote:
> you should never inherit from base::SupportsWeakPtr<>. doing so obscures
exactly
> when the weak factory goes away since it's hidden in a base type instead of
> being a member. compose a base::WeakPtrFactory<> and expose an appropriate
> GetWeak() on the type

Done.

https://codereview.chromium.org/1415493003/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_state.h:72: std::unordered_map<uint32_t,
ViewState*> children;
On 2015/10/27 23:13:11, jamesr wrote:
> no public members

Done.

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

https://codereview.chromium.org/1415493003/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_tree_state.h:23: class ViewTreeState : public
base::SupportsWeakPtr<ViewTreeState> {
On 2015/10/27 23:13:11, jamesr wrote:
> ditto re SupportsWeak

Done.

https://codereview.chromium.org/1415493003/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_tree_state.h:25:
ViewTreeState(mojo::ui::ViewTreePtr view_tree);
On 2015/10/27 23:13:11, jamesr wrote:
> explicit

Done.

https://codereview.chromium.org/1415493003/diff/20001/services/ui/view_manage...
services/ui/view_manager/view_tree_state.h:54: bool root_was_set;
On 2015/10/27 23:13:11, jamesr wrote:
> public member

Done.

[jeffbrown, 2015-10-28 01:46:02]

Committed patchset #4 (id:60001) manually as
0b1c404b97aa59f787a0c6d2ab45dc5f3c633c2c.