Preliminary interactive layout of window manager's demo_launcher

Preliminary interactive layout of window manager's demo_launcher.

The window manager now handles root node resize by resizing its (two) toplevel nodes to match the initial layout. This is just an intermediate step towards generalizing window manager layout a little.

BUGi=393244

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283379

[hansmuller, 2014-07-11 17:18:10]

One small step towards improving the way the window manager lays out its top
level nodes.

[Ben Goodger (Google), 2014-07-11 18:18:23]

https://codereview.chromium.org/383123006/diff/1/mojo/examples/window_manager...
File mojo/examples/window_manager/window_manager.cc (right):

https://codereview.chromium.org/383123006/diff/1/mojo/examples/window_manager...
mojo/examples/window_manager/window_manager.cc:199: }
Looks like you're missing the embedded node(s) (e.g. the content area).

[sky, 2014-07-11 18:19:47]

https://codereview.chromium.org/383123006/diff/1/mojo/examples/browser/browse...
File mojo/examples/browser/browser.cc (right):

https://codereview.chromium.org/383123006/diff/1/mojo/examples/browser/browse...
mojo/examples/browser/browser.cc:237: const gfx::Rect& /*old_bounds*/,
We generally don't use this style in chrome code.

https://codereview.chromium.org/383123006/diff/1/mojo/examples/browser/browse...
mojo/examples/browser/browser.cc:239:
widget_->SetBounds(gfx::Rect(node->bounds().size()));
NativeWidgetViewManager is a NodeObserver. It should have this code. That'll
dramatically simplify this code:)

https://codereview.chromium.org/383123006/diff/1/mojo/examples/browser/browse...
mojo/examples/browser/browser.cc:240: contents_view_->Layout();
This should happen automatically as a result of setting the bounds of the
widget.

https://codereview.chromium.org/383123006/diff/1/mojo/examples/browser/browse...
mojo/examples/browser/browser.cc:248: views::View* contents_view_;
initialize this to NULL in the member initializer list.

[sky, 2014-07-11 18:20:07]

I nuked sky1 from the review list. No idea who that is.

[hansmuller, 2014-07-14 17:35:37]

This version lays out content nodes and the keyboard manager.

[sky, 2014-07-14 19:15:28]

https://codereview.chromium.org/383123006/diff/60001/mojo/examples/html_viewe...
File mojo/examples/html_viewer/html_document_view.cc (right):

https://codereview.chromium.org/383123006/diff/60001/mojo/examples/html_viewe...
mojo/examples/html_viewer/html_document_view.cc:106: node->AddObserver(this);
You need to remove this observer too.

https://codereview.chromium.org/383123006/diff/60001/mojo/examples/html_viewe...
mojo/examples/html_viewer/html_document_view.cc:192:
web_view_->resize(gfx::Size(node->bounds().size()));
You don't need the gfx::Size here. node->bounds().size() already returns a
gfx::Size.

https://codereview.chromium.org/383123006/diff/60001/mojo/examples/html_viewe...
File mojo/examples/html_viewer/html_document_view.h (right):

https://codereview.chromium.org/383123006/diff/60001/mojo/examples/html_viewe...
mojo/examples/html_viewer/html_document_view.h:72: const gfx::Rect&,
Google C++ style is to always have names for arguments, even if not used. Same
comment applies throughout this patch and you want the same in the definitions.

https://codereview.chromium.org/383123006/diff/60001/mojo/examples/media_view...
File mojo/examples/media_viewer/media_viewer.cc (right):

https://codereview.chromium.org/383123006/diff/60001/mojo/examples/media_view...
mojo/examples/media_viewer/media_viewer.cc:299: root->AddObserver(this);
You need to remove the observer when done.

https://codereview.chromium.org/383123006/diff/60001/mojo/examples/png_viewer...
File mojo/examples/png_viewer/png_viewer.cc (right):

https://codereview.chromium.org/383123006/diff/60001/mojo/examples/png_viewer...
mojo/examples/png_viewer/png_viewer.cc:169: root->AddObserver(this);
Same thing about removing observer.

https://codereview.chromium.org/383123006/diff/60001/mojo/examples/window_man...
File mojo/examples/window_manager/window_manager.cc (right):

https://codereview.chromium.org/383123006/diff/60001/mojo/examples/window_man...
mojo/examples/window_manager/window_manager.cc:115: parent->AddObserver(this);
Same thing about removing.

https://codereview.chromium.org/383123006/diff/60001/mojo/examples/window_man...
mojo/examples/window_manager/window_manager.cc:174:
RootLayoutManager(ViewManager* view_manager, Node* root,
Style guide says when you can't fit all the arguments on a single line, then
each argument gets its own line.

https://codereview.chromium.org/383123006/diff/60001/mojo/examples/window_man...
mojo/examples/window_manager/window_manager.cc:178: : root_(root),
Style guide says the : should be indented 4 spaces from RootRootLayout (see line
59).

https://codereview.chromium.org/383123006/diff/60001/mojo/examples/window_man...
mojo/examples/window_manager/window_manager.cc:201:
view_manager_->GetNodeById(launcher_ui_node_id_);
indent 2 more.

https://codereview.chromium.org/383123006/diff/60001/mojo/examples/window_man...
mojo/examples/window_manager/window_manager.cc:212: for (Node* node :
content_node->children()) {
We're not yet using c++11 yet.

https://codereview.chromium.org/383123006/diff/60001/mojo/examples/window_man...
mojo/examples/window_manager/window_manager.cc:226: Id launcher_ui_node_id_;
I believe you can make these const.

https://codereview.chromium.org/383123006/diff/60001/mojo/views/native_widget...
File mojo/views/native_widget_view_manager.h (right):

https://codereview.chromium.org/383123006/diff/60001/mojo/views/native_widget...
mojo/views/native_widget_view_manager.h:56: virtual void
OnNodeBoundsChanged(view_manager::Node* node,
Move this with other NodeObserver overrides above.

[hansmuller, 2014-07-14 22:16:43]

I've made the suggested fixes, including removing observers when the observed
node is being destroyed, and when the observer's destructor runs.

https://codereview.chromium.org/383123006/diff/60001/mojo/examples/html_viewe...
File mojo/examples/html_viewer/html_document_view.cc (right):

https://codereview.chromium.org/383123006/diff/60001/mojo/examples/html_viewe...
mojo/examples/html_viewer/html_document_view.cc:106: node->AddObserver(this);
On 2014/07/14 19:15:27, sky wrote:
> You need to remove this observer too.

Done.

https://codereview.chromium.org/383123006/diff/60001/mojo/examples/html_viewe...
mojo/examples/html_viewer/html_document_view.cc:192:
web_view_->resize(gfx::Size(node->bounds().size()));
On 2014/07/14 19:15:27, sky wrote:
> You don't need the gfx::Size here. node->bounds().size() already returns a
> gfx::Size.

Done.

https://codereview.chromium.org/383123006/diff/60001/mojo/examples/html_viewe...
File mojo/examples/html_viewer/html_document_view.h (right):

https://codereview.chromium.org/383123006/diff/60001/mojo/examples/html_viewe...
mojo/examples/html_viewer/html_document_view.h:72: const gfx::Rect&,
On 2014/07/14 19:15:27, sky wrote:
> Google C++ style is to always have names for arguments, even if not used. Same
> comment applies throughout this patch and you want the same in the
definitions.

Done.

https://codereview.chromium.org/383123006/diff/60001/mojo/examples/media_view...
File mojo/examples/media_viewer/media_viewer.cc (right):

https://codereview.chromium.org/383123006/diff/60001/mojo/examples/media_view...
mojo/examples/media_viewer/media_viewer.cc:299: root->AddObserver(this);
On 2014/07/14 19:15:28, sky wrote:
> You need to remove the observer when done.

Done.

https://codereview.chromium.org/383123006/diff/60001/mojo/examples/png_viewer...
File mojo/examples/png_viewer/png_viewer.cc (right):

https://codereview.chromium.org/383123006/diff/60001/mojo/examples/png_viewer...
mojo/examples/png_viewer/png_viewer.cc:169: root->AddObserver(this);
On 2014/07/14 19:15:28, sky wrote:
> Same thing about removing observer.

Done.

https://codereview.chromium.org/383123006/diff/60001/mojo/examples/window_man...
File mojo/examples/window_manager/window_manager.cc (right):

https://codereview.chromium.org/383123006/diff/60001/mojo/examples/window_man...
mojo/examples/window_manager/window_manager.cc:115: parent->AddObserver(this);
On 2014/07/14 19:15:28, sky wrote:
> Same thing about removing.

Done.

https://codereview.chromium.org/383123006/diff/60001/mojo/examples/window_man...
mojo/examples/window_manager/window_manager.cc:174:
RootLayoutManager(ViewManager* view_manager, Node* root,
On 2014/07/14 19:15:28, sky wrote:
> Style guide says when you can't fit all the arguments on a single line, then
> each argument gets its own line.

Done.

https://codereview.chromium.org/383123006/diff/60001/mojo/examples/window_man...
mojo/examples/window_manager/window_manager.cc:178: : root_(root),
On 2014/07/14 19:15:28, sky wrote:
> Style guide says the : should be indented 4 spaces from RootRootLayout (see
line
> 59).

Done.

https://codereview.chromium.org/383123006/diff/60001/mojo/examples/window_man...
mojo/examples/window_manager/window_manager.cc:201:
view_manager_->GetNodeById(launcher_ui_node_id_);
On 2014/07/14 19:15:28, sky wrote:
> indent 2 more.

Done.

https://codereview.chromium.org/383123006/diff/60001/mojo/examples/window_man...
mojo/examples/window_manager/window_manager.cc:212: for (Node* node :
content_node->children()) {
On 2014/07/14 19:15:28, sky wrote:
> We're not yet using c++11 yet.

Done.

https://codereview.chromium.org/383123006/diff/60001/mojo/examples/window_man...
mojo/examples/window_manager/window_manager.cc:226: Id launcher_ui_node_id_;
On 2014/07/14 19:15:28, sky wrote:
> I believe you can make these const.

Done.

https://codereview.chromium.org/383123006/diff/60001/mojo/views/native_widget...
File mojo/views/native_widget_view_manager.h (right):

https://codereview.chromium.org/383123006/diff/60001/mojo/views/native_widget...
mojo/views/native_widget_view_manager.h:56: virtual void
OnNodeBoundsChanged(view_manager::Node* node,
On 2014/07/14 19:15:28, sky wrote:
> Move this with other NodeObserver overrides above.

Done.

[sky, 2014-07-15 00:07:49]

https://codereview.chromium.org/383123006/diff/120001/mojo/examples/html_view...
File mojo/examples/html_viewer/html_document_view.cc (right):

https://codereview.chromium.org/383123006/diff/120001/mojo/examples/html_view...
mojo/examples/html_viewer/html_document_view.cc:191: // Overridden from
NodeObserver:
Style guide says not to duplicate comments like this in the definition.

https://codereview.chromium.org/383123006/diff/120001/mojo/examples/html_view...
mojo/examples/html_viewer/html_document_view.cc:202: node->RemoveObserver(this);
Shouldn't you set root_ to NULL here too? This comment applies to similar
overrides.

[hansmuller, 2014-07-15 00:27:43]

Removed the comments that aren't style-guide compliant.

https://codereview.chromium.org/383123006/diff/120001/mojo/examples/html_view...
File mojo/examples/html_viewer/html_document_view.cc (right):

https://codereview.chromium.org/383123006/diff/120001/mojo/examples/html_view...
mojo/examples/html_viewer/html_document_view.cc:191: // Overridden from
NodeObserver:
On 2014/07/15 00:07:49, sky wrote:
> Style guide says not to duplicate comments like this in the definition.

Done.

https://codereview.chromium.org/383123006/diff/120001/mojo/examples/html_view...
mojo/examples/html_viewer/html_document_view.cc:202: node->RemoveObserver(this);
On 2014/07/15 00:07:49, sky wrote:
> Shouldn't you set root_ to NULL here too? This comment applies to similar
> overrides.

That implies that other NodeObserver methods can't rely on root_ once the
OnNodeDestroying method has run. They don't know but they could. Why set root_
to NULL here?

[sky, 2014-07-15 04:08:37]

https://codereview.chromium.org/383123006/diff/120001/mojo/examples/html_view...
File mojo/examples/html_viewer/html_document_view.cc (right):

https://codereview.chromium.org/383123006/diff/120001/mojo/examples/html_view...
mojo/examples/html_viewer/html_document_view.cc:202: node->RemoveObserver(this);
On 2014/07/15 00:27:43, hansmuller wrote:
> On 2014/07/15 00:07:49, sky wrote:
> > Shouldn't you set root_ to NULL here too? This comment applies to similar
> > overrides.
> 
> That implies that other NodeObserver methods can't rely on root_ once the
> OnNodeDestroying method has run. They don't know but they could. Why set root_
> to NULL here?

By adding this override you're saying root_ may be destroyed before this. If
that happens you'll crash on 94 because root_ will not be NULL yet root_ has
been deleted.

[hansmuller, 2014-07-15 15:04:08]

I've moved all of the node observer removals from destroyING to destroyED.  Each
method now clears the corresponding (destroyed) node member, if there is one.

Removing the observer as "late" as possible makes sense, since other
notifications, like lost-focus, might get lost otherwise. On the other hand, if
OnNodeDestroyed() is a post-facto notification, then it's not clear why node
operations, like RemoveObserver() would still be valid.

[hansmuller, 2014-07-15 16:18:38]

Fixed the last of the node observer leaks and made comments/formatting more
style-guide compliant.

[sky, 2014-07-15 17:47:49]

https://codereview.chromium.org/383123006/diff/200001/mojo/examples/window_ma...
File mojo/examples/window_manager/window_manager.cc (right):

https://codereview.chromium.org/383123006/diff/200001/mojo/examples/window_ma...
mojo/examples/window_manager/window_manager.cc:100: if (node_)
node_ is never set to NULL, so I'm not sure this check is buying anything as you
have it. I think if you set node_ to NULL in OnNodeDestroyed below it'll be ok.

https://codereview.chromium.org/383123006/diff/200001/mojo/views/native_widge...
File mojo/views/native_widget_view_manager.cc (right):

https://codereview.chromium.org/383123006/diff/200001/mojo/views/native_widge...
mojo/views/native_widget_view_manager.cc:139: node->RemoveObserver(this);
You should remove the observer from the active_view, if any, as well. In fact
this class should override OnViewDestroyed for the cleanup too. I'm not sure of
the exact order here, meaning whether active_view() has been cleared by the time
OnNodeDestroyed is called.

[hansmuller, 2014-07-15 20:07:34]

Updated the changes per review feedback.

https://codereview.chromium.org/383123006/diff/200001/mojo/examples/window_ma...
File mojo/examples/window_manager/window_manager.cc (right):

https://codereview.chromium.org/383123006/diff/200001/mojo/examples/window_ma...
mojo/examples/window_manager/window_manager.cc:100: if (node_)
On 2014/07/15 17:47:49, sky wrote:
> node_ is never set to NULL, so I'm not sure this check is buying anything as
you
> have it. I think if you set node_ to NULL in OnNodeDestroyed below it'll be
ok.

OK

https://codereview.chromium.org/383123006/diff/200001/mojo/views/native_widge...
File mojo/views/native_widget_view_manager.cc (right):

https://codereview.chromium.org/383123006/diff/200001/mojo/views/native_widge...
mojo/views/native_widget_view_manager.cc:139: node->RemoveObserver(this);
On 2014/07/15 17:47:49, sky wrote:
> You should remove the observer from the active_view, if any, as well. In fact
> this class should override OnViewDestroyed for the cleanup too. I'm not sure
of
> the exact order here, meaning whether active_view() has been cleared by the
time
> OnNodeDestroyed is called.

I'll remove the view's observer when the view is destroyed.

[sky, 2014-07-15 20:46:48]

https://codereview.chromium.org/383123006/diff/220001/mojo/views/native_widge...
File mojo/views/native_widget_view_manager.cc (right):

https://codereview.chromium.org/383123006/diff/220001/mojo/views/native_widge...
mojo/views/native_widget_view_manager.cc:121: }
I think you're missing the case of the node_ having been destroyed, but not the
view.

[hansmuller, 2014-07-15 21:08:44]

Handle the case where the NativeWidgetViewManager's node is destroyed and then
its destructor runs.

[sky, 2014-07-15 21:45:38]

LGTM with the following changes

https://codereview.chromium.org/383123006/diff/240001/mojo/views/native_widge...
File mojo/views/native_widget_view_manager.cc (right):

https://codereview.chromium.org/383123006/diff/240001/mojo/views/native_widge...
mojo/views/native_widget_view_manager.cc:95: view_ = node_->active_view();
move this to member initializer.

https://codereview.chromium.org/383123006/diff/240001/mojo/views/native_widge...
mojo/views/native_widget_view_manager.cc:96: view_->AddObserver(this);
view_ may be NULL here.

[hansmuller, 2014-07-15 22:11:30]

The CQ bit was checked by hansmuller@chromium.org

[commit-bot: I haz the power, 2014-07-15 22:14:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hansmuller@chromium.org/383123006/260001

[commit-bot: I haz the power, 2014-07-16 07:10:19]

Change committed as 283379