Fix the full screen switching browser window dipping below launcher bar issue.

Fix the full screen switching browser window dipping below launcher bar issue.


BUG=114497
TEST=The browser window should not dipping under the launcher bar when toggling with full screen mode.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=124524

[sky, 2012-02-28 20:49:25]

You also need to do the following:
. make RootWindow::screen() return a const ScreenAura, or Screen. This prevents
folks from setting the insets on it.
. Update WorkspaceLayoutManager.
. Add test coverage.

http://codereview.chromium.org/9515003/diff/1/ash/wm/shelf_layout_manager.cc
File ash/wm/shelf_layout_manager.cc (right):

http://codereview.chromium.org/9515003/diff/1/ash/wm/shelf_layout_manager.cc#...
ash/wm/shelf_layout_manager.cc:156:
Shell::GetRootWindow()->OnScreenWorkAreaInsetsChanged();
Forcing callers to invoke a method when the insets changes is easy to forget.
Instead add a setter on RootWindow that sets the insets and notifies should be
added.

http://codereview.chromium.org/9515003/diff/1/ui/aura/root_window_observer.h
File ui/aura/root_window_observer.h (right):

http://codereview.chromium.org/9515003/diff/1/ui/aura/root_window_observer.h#...
ui/aura/root_window_observer.h:24: // Invoked after the screen's
work_area_insets_ changed.
nit: work_area_insets_ -> work area insets changes.

[jennyz, 2012-03-01 00:56:13]

Fix the issue for the managed mode in WorkSpaceManagerController and add 2 test
cases.

http://codereview.chromium.org/9515003/diff/1/ash/wm/shelf_layout_manager.cc
File ash/wm/shelf_layout_manager.cc (right):

http://codereview.chromium.org/9515003/diff/1/ash/wm/shelf_layout_manager.cc#...
ash/wm/shelf_layout_manager.cc:156:
Shell::GetRootWindow()->OnScreenWorkAreaInsetsChanged();
On 2012/02/28 20:49:26, sky wrote:
> Forcing callers to invoke a method when the insets changes is easy to forget.
> Instead add a setter on RootWindow that sets the insets and notifies should be
> added.

Done.

http://codereview.chromium.org/9515003/diff/1/ui/aura/root_window_observer.h
File ui/aura/root_window_observer.h (right):

http://codereview.chromium.org/9515003/diff/1/ui/aura/root_window_observer.h#...
ui/aura/root_window_observer.h:24: // Invoked after the screen's
work_area_insets_ changed.
On 2012/02/28 20:49:26, sky wrote:
> nit: work_area_insets_ -> work area insets changes.

Done.

[sky, 2012-03-01 01:43:19]

http://codereview.chromium.org/9515003/diff/6003/ash/wm/base_layout_manager.cc
File ash/wm/base_layout_manager.cc (right):

http://codereview.chromium.org/9515003/diff/6003/ash/wm/base_layout_manager.c...
ash/wm/base_layout_manager.cc:77: void
BaseLayoutManager::AdjustWindowSizesForScreenChange() {
Order should match that of header.

http://codereview.chromium.org/9515003/diff/6003/ash/wm/toplevel_layout_manag...
File ash/wm/toplevel_layout_manager_unittest.cc (right):

http://codereview.chromium.org/9515003/diff/6003/ash/wm/toplevel_layout_manag...
ash/wm/toplevel_layout_manager_unittest.cc:105: {
{ on previous line, or wrap if > 80.

http://codereview.chromium.org/9515003/diff/6003/ash/wm/workspace_controller.cc
File ash/wm/workspace_controller.cc (right):

http://codereview.chromium.org/9515003/diff/6003/ash/wm/workspace_controller....
ash/wm/workspace_controller.cc:79: workspace_manager_->SetWorkspaceBounds();
Shouldn't this be the same as OnRootWindowResized?

http://codereview.chromium.org/9515003/diff/6003/ui/aura/root_window.cc
File ui/aura/root_window.cc (right):

http://codereview.chromium.org/9515003/diff/6003/ui/aura/root_window.cc#newco...
ui/aura/root_window.cc:119: screen_->set_work_area_insets(insets);
Don't do anything if insets are the same.

http://codereview.chromium.org/9515003/diff/6003/ui/aura/root_window.h
File ui/aura/root_window.h (right):

http://codereview.chromium.org/9515003/diff/6003/ui/aura/root_window.h#newcode85
ui/aura/root_window.h:85: // Sets the screen's work area insets.
Add that this notifies observers too.

[jennyz, 2012-03-01 18:27:45]

http://codereview.chromium.org/9515003/diff/6003/ash/wm/base_layout_manager.cc
File ash/wm/base_layout_manager.cc (right):

http://codereview.chromium.org/9515003/diff/6003/ash/wm/base_layout_manager.c...
ash/wm/base_layout_manager.cc:77: void
BaseLayoutManager::AdjustWindowSizesForScreenChange() {
On 2012/03/01 01:43:19, sky wrote:
> Order should match that of header.

Done.

http://codereview.chromium.org/9515003/diff/6003/ash/wm/toplevel_layout_manag...
File ash/wm/toplevel_layout_manager_unittest.cc (right):

http://codereview.chromium.org/9515003/diff/6003/ash/wm/toplevel_layout_manag...
ash/wm/toplevel_layout_manager_unittest.cc:105: {
On 2012/03/01 01:43:19, sky wrote:
> { on previous line, or wrap if > 80.

Done.

http://codereview.chromium.org/9515003/diff/6003/ash/wm/workspace_controller.cc
File ash/wm/workspace_controller.cc (right):

http://codereview.chromium.org/9515003/diff/6003/ash/wm/workspace_controller....
ash/wm/workspace_controller.cc:79: workspace_manager_->SetWorkspaceBounds();
On 2012/03/01 01:43:19, sky wrote:
> Shouldn't this be the same as OnRootWindowResized?
OnScreenWorkAreaInsetsChanged() does not pass the new_size of the workspace in.
OnRootWindowResized needs to pass in the new_size of the work space, and set its
work_space_ to new_size, which we don't want it to be done for work area insets
change case, the work space does not change.

http://codereview.chromium.org/9515003/diff/6003/ui/aura/root_window.cc
File ui/aura/root_window.cc (right):

http://codereview.chromium.org/9515003/diff/6003/ui/aura/root_window.cc#newco...
ui/aura/root_window.cc:119: screen_->set_work_area_insets(insets);
On 2012/03/01 01:43:19, sky wrote:
> Don't do anything if insets are the same.

Done.

http://codereview.chromium.org/9515003/diff/6003/ui/aura/root_window.h
File ui/aura/root_window.h (right):

http://codereview.chromium.org/9515003/diff/6003/ui/aura/root_window.h#newcode85
ui/aura/root_window.h:85: // Sets the screen's work area insets.
On 2012/03/01 01:43:19, sky wrote:
> Add that this notifies observers too.

Done.

[sky, 2012-03-01 18:43:45]

http://codereview.chromium.org/9515003/diff/6003/ash/wm/workspace_controller.cc
File ash/wm/workspace_controller.cc (right):

http://codereview.chromium.org/9515003/diff/6003/ash/wm/workspace_controller....
ash/wm/workspace_controller.cc:79: workspace_manager_->SetWorkspaceBounds();
On 2012/03/01 18:27:45, jennyz wrote:
> On 2012/03/01 01:43:19, sky wrote:
> > Shouldn't this be the same as OnRootWindowResized?
> OnScreenWorkAreaInsetsChanged() does not pass the new_size of the workspace
in.
> OnRootWindowResized needs to pass in the new_size of the work space, and set
its
> work_space_ to new_size, which we don't want it to be done for work area
insets
> change case, the work space does not change.

I see what you're saying. In that case name the WorkspaceManager method the same
as this and have both it and SetWorkspaceSize call into a new method called
SetWorkspaceBounds.

[jennyz, 2012-03-01 19:13:11]

Did the change to SetWorkspaceBounds as we discussed. Also did a rebase, so
please ignore the rebase delta with the previous patch.

http://codereview.chromium.org/9515003/diff/6003/ash/wm/workspace_controller.cc
File ash/wm/workspace_controller.cc (right):

http://codereview.chromium.org/9515003/diff/6003/ash/wm/workspace_controller....
ash/wm/workspace_controller.cc:79: workspace_manager_->SetWorkspaceBounds();
On 2012/03/01 18:43:45, sky wrote:
> On 2012/03/01 18:27:45, jennyz wrote:
> > On 2012/03/01 01:43:19, sky wrote:
> > > Shouldn't this be the same as OnRootWindowResized?
> > OnScreenWorkAreaInsetsChanged() does not pass the new_size of the workspace
> in.
> > OnRootWindowResized needs to pass in the new_size of the work space, and set
> its
> > work_space_ to new_size, which we don't want it to be done for work area
> insets
> > change case, the work space does not change.
> 
> I see what you're saying. In that case name the WorkspaceManager method the
same
> as this and have both it and SetWorkspaceSize call into a new method called
> SetWorkspaceBounds.

Done.

[sky, 2012-03-01 20:52:03]

LGTM

[commit-bot: I haz the power, 2012-03-01 21:21:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jennyz@chromium.org/9515003/16019

[commit-bot: I haz the power, 2012-03-01 23:14:18]

Change committed as 124524