Refactor: Shelf Widget

Refactor: Shelf Widget

Refactor the classes related with displaying the shelf (background behind the launcher/status area widget) to be a separate class. Removing background delegates from the launcher and status area widget (represented on tray views).

TBR=ben@chromium.org
BUG=163002
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=187122

[Harry McCleave, 2013-02-27 04:05:38]

Please take a look when you get a chance.

https://codereview.chromium.org/12313118/diff/8001/ui/compositor/layer_animat...
File ui/compositor/layer_animation_element.cc (right):

https://codereview.chromium.org/12313118/diff/8001/ui/compositor/layer_animat...
ui/compositor/layer_animation_element.cc:556: t = std::max(0.0,
elapsed.InMillisecondsF() / duration_.InMillisecondsF());
This is temporary and vollick@ is landing a patch that will take care of this,
(this code will be removed with rebase before submission).

[Mr4D (OOO till 08-26), 2013-02-27 19:42:51]

Due to the size of the review I need to do this in turns... 

Here are a few comments for the time being.

https://codereview.chromium.org/12313118/diff/8001/ash/focus_cycler_unittest.cc
File ash/focus_cycler_unittest.cc (right):

https://codereview.chromium.org/12313118/diff/8001/ash/focus_cycler_unittest....
ash/focus_cycler_unittest.cc:58:
Launcher::ForPrimaryDisplay()->shelf_widget()->SetFocusCycler(NULL);
Couldn't you use here "shelf_widget()" (as defined below)?

https://codereview.chromium.org/12313118/diff/8001/ash/launcher/launcher.h
File ash/launcher/launcher.h (right):

https://codereview.chromium.org/12313118/diff/8001/ash/launcher/launcher.h#ne...
ash/launcher/launcher.h:99: void SetViewBounds(gfx::Rect bounds);
Could you please add a comment for the new functions? Also - "SetViewBounds": In
most cases we have a SetBounds and fro animations something like
SetTargetBounds, why do we need another one?

https://codereview.chromium.org/12313118/diff/8001/ash/launcher/launcher_tool...
File ash/launcher/launcher_tooltip_manager.cc (right):

https://codereview.chromium.org/12313118/diff/8001/ash/launcher/launcher_tool...
ash/launcher/launcher_tooltip_manager.cc:12: #include
"ash/wm/session_state_observer.h"
Are you sure you need these 2?

https://codereview.chromium.org/12313118/diff/8001/ash/launcher/launcher_tool...
File ash/launcher/launcher_tooltip_manager.h (right):

https://codereview.chromium.org/12313118/diff/8001/ash/launcher/launcher_tool...
ash/launcher/launcher_tooltip_manager.h:11: #include
"ash/wm/session_state_observer.h"
Are you sure you need this include here?

https://codereview.chromium.org/12313118/diff/8001/ash/root_window_controller.cc
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/12313118/diff/8001/ash/root_window_controller...
ash/root_window_controller.cc:271: //  todo (harrym) remove when status area is
view.
// TODO(harrym): remove..

https://codereview.chromium.org/12313118/diff/8001/ash/root_window_controller...
ash/root_window_controller.cc:274: shelf_ = new ash::ShelfWidget(
I am fairly certain that shelf_ should be a scoped_ptr since you create it and
simply set it to NULL upon destruction.

https://codereview.chromium.org/12313118/diff/8001/ash/root_window_controller...
ash/root_window_controller.cc:424: }
empty line here

https://codereview.chromium.org/12313118/diff/8001/ash/root_window_controller...
ash/root_window_controller.cc:427: }
empty line again

[Harry McCleave, 2013-03-01 04:31:19]

https://codereview.chromium.org/12313118/diff/8001/ash/focus_cycler_unittest.cc
File ash/focus_cycler_unittest.cc (right):

https://codereview.chromium.org/12313118/diff/8001/ash/focus_cycler_unittest....
ash/focus_cycler_unittest.cc:58:
Launcher::ForPrimaryDisplay()->shelf_widget()->SetFocusCycler(NULL);
On 2013/02/27 19:42:51, Mr4D wrote:
> Couldn't you use here "shelf_widget()" (as defined below)?

Done.

https://codereview.chromium.org/12313118/diff/8001/ash/launcher/launcher.h
File ash/launcher/launcher.h (right):

https://codereview.chromium.org/12313118/diff/8001/ash/launcher/launcher.h#ne...
ash/launcher/launcher.h:99: void SetViewBounds(gfx::Rect bounds);
On 2013/02/27 19:42:51, Mr4D wrote:
> Could you please add a comment for the new functions? Also - "SetViewBounds":
In
> most cases we have a SetBounds and fro animations something like
> SetTargetBounds, why do we need another one? 

Will add comment, but would like some input on correct name first, these
functions provide access to get/set the bounds of the launcher_view view, where
it is not publicly exposed by the launcher (except via a function required by
test framework marked todo:remove), they might be possible to remove when the
status area becomes a view, as they are used right now to size/position the
major axis of the launcher so it is the correct size/side of the status area
widget.

https://codereview.chromium.org/12313118/diff/8001/ash/launcher/launcher_tool...
File ash/launcher/launcher_tooltip_manager.cc (right):

https://codereview.chromium.org/12313118/diff/8001/ash/launcher/launcher_tool...
ash/launcher/launcher_tooltip_manager.cc:12: #include
"ash/wm/session_state_observer.h"
On 2013/02/27 19:42:51, Mr4D wrote:
> Are you sure you need these 2?

Done.

https://codereview.chromium.org/12313118/diff/8001/ash/launcher/launcher_tool...
File ash/launcher/launcher_tooltip_manager.h (right):

https://codereview.chromium.org/12313118/diff/8001/ash/launcher/launcher_tool...
ash/launcher/launcher_tooltip_manager.h:11: #include
"ash/wm/session_state_observer.h"
On 2013/02/27 19:42:51, Mr4D wrote:
> Are you sure you need this include here?

Done.

https://codereview.chromium.org/12313118/diff/8001/ash/root_window_controller.cc
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/12313118/diff/8001/ash/root_window_controller...
ash/root_window_controller.cc:271: //  todo (harrym) remove when status area is
view.
On 2013/02/27 19:42:51, Mr4D wrote:
> // TODO(harrym): remove..

Done.

https://codereview.chromium.org/12313118/diff/8001/ash/root_window_controller...
ash/root_window_controller.cc:274: shelf_ = new ash::ShelfWidget(
On 2013/02/27 19:42:51, Mr4D wrote:
> I am fairly certain that shelf_ should be a scoped_ptr since you create it and
> simply set it to NULL upon destruction.

Done.

https://codereview.chromium.org/12313118/diff/8001/ash/root_window_controller...
ash/root_window_controller.cc:424: }
On 2013/02/27 19:42:51, Mr4D wrote:
> empty line here

Done.

https://codereview.chromium.org/12313118/diff/8001/ash/root_window_controller...
ash/root_window_controller.cc:427: }
On 2013/02/27 19:42:51, Mr4D wrote:
> empty line again

Done.

[Mr4D (OOO till 08-26), 2013-03-02 02:02:12]

Okay. Lots of comments (sorry) - but we are getting there!

(good for a first round!)

https://codereview.chromium.org/12313118/diff/8001/ash/launcher/launcher.h
File ash/launcher/launcher.h (right):

https://codereview.chromium.org/12313118/diff/8001/ash/launcher/launcher.h#ne...
ash/launcher/launcher.h:99: void SetViewBounds(gfx::Rect bounds);
So as a general comment: If a function is only used for tests it should have a
name like "<bla>_for_test(..)" or similar. It is is also used elsewhere I would
name it "SetBounds()".
So if only a unit test: SetViewBoundsForTest(..), otherwise only SetBounds. In
the end Sky might have a different opinion there. (but: Eclipse - Refactor /
rename, done!)

https://codereview.chromium.org/12313118/diff/8001/ash/shelf/shelf_layout_man...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/12313118/diff/8001/ash/shelf/shelf_layout_man...
ash/shelf/shelf_layout_manager.cc:230: }
No brackets needed here.

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_layout_ma...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager.cc:230: }
These brackets are unnecessary

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager.cc:504: if
(RootWindowController::ForLauncher(window)->shelf())
You might want to assign this to a variable to avoid calling this thing twice.

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager.cc:562: ui::ScopedLayerAnimationSettings
launcher_animation_setter(
According to line 524 shelf_ can get NULL (as launcher_widget() was able to). I
guess you should add a test to several of the following lines? If that is not
required - you should be able to remove the test above as well?

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager.cc:625: } else {
These brackets are not necessary - see on the left.

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_layout_ma...
File ash/shelf/shelf_layout_manager_unittest.cc (right):

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager_unittest.cc:267: bottom +
(shelf->shelf_layout_manager()->GetIdealBounds().height() -
wouldn't this be 'manager'?

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_types.h
File ash/shelf/shelf_types.h (right):

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_types.h#n...
ash/shelf/shelf_types.h:5: #ifndef ASH_SHELF_TYPES_H_
I assume this has moved to a new home? :)

-> ASH_SHELF_SHELF_TYPES_H_

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_widget.cc
File ash/shelf/shelf_widget.cc (right):

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_widget.cc...
ash/shelf/shelf_widget.cc:62: void SetDimmed(bool dimmed) {
Comment? What is dimmed exactly?

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_widget.cc...
ash/shelf/shelf_widget.cc:68: bool GetDimmed() const { return dimmed_; }
newline

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_widget.cc...
ash/shelf/shelf_widget.cc:76: virtual const views::Widget* GetWidget() const
OVERRIDE {
virtual + OVERRIDE + body might make clang barf.

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_widget.cc...
ash/shelf/shelf_widget.cc:105: ShelfWidget* shelf_;
Again - comments?

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_widget.cc...
ash/shelf/shelf_widget.cc:292: } else {
no brackets if only one line like here (if & else)

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_widget.h
File ash/shelf/shelf_widget.h (right):

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_widget.h#...
ash/shelf/shelf_widget.h:65: void CreateLauncher();
Here are only a few comments. Maybe add a few?

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_widget.h#...
ash/shelf/shelf_widget.h:81: aura::Window* window_container() { return
window_container_; }
empty line before private

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_widget_un...
File ash/shelf/shelf_widget_unittest.cc (right):

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_widget_un...
ash/shelf/shelf_widget_unittest.cc:34: }
}  // namespace
<newline>
...

https://codereview.chromium.org/12313118/diff/15001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/12313118/diff/15001/ash/shell.cc#newcode714
ash/shell.cc:714: }
newline

https://codereview.chromium.org/12313118/diff/15001/ash/shell.cc#newcode719
ash/shell.cc:719: iter != controllers.end(); ++iter)
one space

https://codereview.chromium.org/12313118/diff/15001/ash/shell.cc#newcode730
ash/shell.cc:730: iter != controllers.end(); ++iter)
one space

https://codereview.chromium.org/12313118/diff/15001/ash/shell/context_menu.h
File ash/shell/context_menu.h (right):

https://codereview.chromium.org/12313118/diff/15001/ash/shell/context_menu.h#...
ash/shell/context_menu.h:5: #ifndef ASH_WM_SHELL_CONTEXT_MENU_H_
Shouldn't this be
ASH_SHELL_CONTEXT_MENU_H_?

https://codereview.chromium.org/12313118/diff/15001/ash/system/ime/tray_ime.cc
File ash/system/ime/tray_ime.cc (right):

https://codereview.chromium.org/12313118/diff/15001/ash/system/ime/tray_ime.c...
ash/system/ime/tray_ime.cc:11: #include "ash/shelf/shelf_widget.h"
Sure you need this?

https://codereview.chromium.org/12313118/diff/15001/ash/system/web_notificati...
File ash/system/web_notification/web_notification_tray.cc (right):

https://codereview.chromium.org/12313118/diff/15001/ash/system/web_notificati...
ash/system/web_notification/web_notification_tray.cc:16: #include
"base/message_loop.h"
Are you sure you need this?

https://codereview.chromium.org/12313118/diff/15001/ash/system/web_notificati...
ash/system/web_notification/web_notification_tray.cc:17: #include
"base/stringprintf.h"
Are you sure you need this?

https://codereview.chromium.org/12313118/diff/15001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/shell_window_launcher_item_controller.cc
(right):

https://codereview.chromium.org/12313118/diff/15001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/shell_window_launcher_item_controller.cc:82: bool
ShellWindowLauncherItemController::HasWindow(aura::Window* window) const {
Not that I don't think that this is a good change - but the CL is humangous as
it is. Maybe you leave it the way it is?

[Harry McCleave, 2013-03-04 02:47:40]

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_layout_ma...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager.cc:230: }
On 2013/03/02 02:02:12, Mr4D wrote:
> These brackets are unnecessary

Done.

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager.cc:504: if
(RootWindowController::ForLauncher(window)->shelf())
On 2013/03/02 02:02:12, Mr4D wrote:
> You might want to assign this to a variable to avoid calling this thing twice.

Done.

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager.cc:562: ui::ScopedLayerAnimationSettings
launcher_animation_setter(
On 2013/03/02 02:02:12, Mr4D wrote:
> According to line 524 shelf_ can get NULL (as launcher_widget() was able to).
I
> guess you should add a test to several of the following lines? If that is not
> required - you should be able to remove the test above as well?

Done.

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager.cc:625: } else {
On 2013/03/02 02:02:12, Mr4D wrote:
> These brackets are not necessary - see on the left.

Done.

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_layout_ma...
File ash/shelf/shelf_layout_manager_unittest.cc (right):

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager_unittest.cc:267: bottom +
(shelf->shelf_layout_manager()->GetIdealBounds().height() -
On 2013/03/02 02:02:12, Mr4D wrote:
> wouldn't this be 'manager'?

Done.

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_types.h
File ash/shelf/shelf_types.h (right):

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_types.h#n...
ash/shelf/shelf_types.h:5: #ifndef ASH_SHELF_TYPES_H_
On 2013/03/02 02:02:12, Mr4D wrote:
> I assume this has moved to a new home? :)
> 
> -> ASH_SHELF_SHELF_TYPES_H_

Done.

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_widget.cc
File ash/shelf/shelf_widget.cc (right):

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_widget.cc...
ash/shelf/shelf_widget.cc:62: void SetDimmed(bool dimmed) {
On 2013/03/02 02:02:12, Mr4D wrote:
> Comment? What is dimmed exactly?

Done.

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_widget.cc...
ash/shelf/shelf_widget.cc:68: bool GetDimmed() const { return dimmed_; }
On 2013/03/02 02:02:12, Mr4D wrote:
> newline

Done.

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_widget.cc...
ash/shelf/shelf_widget.cc:76: virtual const views::Widget* GetWidget() const
OVERRIDE {
On 2013/03/02 02:02:12, Mr4D wrote:
> virtual + OVERRIDE + body might make clang barf.

Done.

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_widget.cc...
ash/shelf/shelf_widget.cc:105: ShelfWidget* shelf_;
On 2013/03/02 02:02:12, Mr4D wrote:
> Again - comments?

Done.

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_widget.cc...
ash/shelf/shelf_widget.cc:292: } else {
On 2013/03/02 02:02:12, Mr4D wrote:
> no brackets if only one line like here (if & else)

Done.

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_widget.h
File ash/shelf/shelf_widget.h (right):

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_widget.h#...
ash/shelf/shelf_widget.h:65: void CreateLauncher();
On 2013/03/02 02:02:12, Mr4D wrote:
> Here are only a few comments. Maybe add a few?

Done.

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_widget.h#...
ash/shelf/shelf_widget.h:81: aura::Window* window_container() { return
window_container_; }
On 2013/03/02 02:02:12, Mr4D wrote:
> empty line before private

Done.

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_widget.h#...
ash/shelf/shelf_widget.h:81: aura::Window* window_container() { return
window_container_; }
On 2013/03/02 02:02:12, Mr4D wrote:
> empty line before private

Done.

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_widget_un...
File ash/shelf/shelf_widget_unittest.cc (right):

https://codereview.chromium.org/12313118/diff/15001/ash/shelf/shelf_widget_un...
ash/shelf/shelf_widget_unittest.cc:34: }
On 2013/03/02 02:02:12, Mr4D wrote:
> }  // namespace
> <newline>
> ...

Done.

https://codereview.chromium.org/12313118/diff/15001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/12313118/diff/15001/ash/shell.cc#newcode714
ash/shell.cc:714: }
On 2013/03/02 02:02:12, Mr4D wrote:
> newline

Done.

https://codereview.chromium.org/12313118/diff/15001/ash/shell.cc#newcode719
ash/shell.cc:719: iter != controllers.end(); ++iter)
On 2013/03/02 02:02:12, Mr4D wrote:
> one space

Done.

https://codereview.chromium.org/12313118/diff/15001/ash/shell.cc#newcode730
ash/shell.cc:730: iter != controllers.end(); ++iter)
On 2013/03/02 02:02:12, Mr4D wrote:
> one space

Done.

https://codereview.chromium.org/12313118/diff/15001/ash/shell/context_menu.h
File ash/shell/context_menu.h (right):

https://codereview.chromium.org/12313118/diff/15001/ash/shell/context_menu.h#...
ash/shell/context_menu.h:5: #ifndef ASH_WM_SHELL_CONTEXT_MENU_H_
On 2013/03/02 02:02:12, Mr4D wrote:
> Shouldn't this be
> ASH_SHELL_CONTEXT_MENU_H_?

Done.

https://codereview.chromium.org/12313118/diff/15001/ash/system/ime/tray_ime.cc
File ash/system/ime/tray_ime.cc (right):

https://codereview.chromium.org/12313118/diff/15001/ash/system/ime/tray_ime.c...
ash/system/ime/tray_ime.cc:11: #include "ash/shelf/shelf_widget.h"
On 2013/03/02 02:02:12, Mr4D wrote:
> Sure you need this?

Done.

https://codereview.chromium.org/12313118/diff/15001/ash/system/web_notificati...
File ash/system/web_notification/web_notification_tray.cc (right):

https://codereview.chromium.org/12313118/diff/15001/ash/system/web_notificati...
ash/system/web_notification/web_notification_tray.cc:16: #include
"base/message_loop.h"
On 2013/03/02 02:02:12, Mr4D wrote:
> Are you sure you need this?

Done.

https://codereview.chromium.org/12313118/diff/15001/ash/system/web_notificati...
ash/system/web_notification/web_notification_tray.cc:17: #include
"base/stringprintf.h"
On 2013/03/02 02:02:12, Mr4D wrote:
> Are you sure you need this?

Done.

https://codereview.chromium.org/12313118/diff/15001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/shell_window_launcher_item_controller.cc
(right):

https://codereview.chromium.org/12313118/diff/15001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/shell_window_launcher_item_controller.cc:82: bool
ShellWindowLauncherItemController::HasWindow(aura::Window* window) const {
On 2013/03/02 02:02:12, Mr4D wrote:
> Not that I don't think that this is a good change - but the CL is humangous as
> it is. Maybe you leave it the way it is?

Done.

[James Cook, 2013-03-04 18:03:22]

Ping me when you're ready for OWNERS.

[Mr4D (OOO till 08-26), 2013-03-04 19:18:04]

I got to run into an IV - but to get you going again, I post what I have so far.

https://codereview.chromium.org/12313118/diff/14002/ash/launcher/launcher.cc
File ash/launcher/launcher.cc (right):

https://codereview.chromium.org/12313118/diff/14002/ash/launcher/launcher.cc#...
ash/launcher/launcher.cc:63: Launcher::~Launcher() {
Since there isn't anything left you should possibly move this into the header
file.

https://codereview.chromium.org/12313118/diff/14002/ash/launcher/launcher_vie...
File ash/launcher/launcher_view_unittest.cc (right):

https://codereview.chromium.org/12313118/diff/14002/ash/launcher/launcher_vie...
ash/launcher/launcher_view_unittest.cc:185: gfx::Rect(0, 0, total_width,
shelf_size.height()));
I would do it like this:
shelf_size.set_width(..);
then 
SetLauncherViewBounds(gfx::Rect(shelf_size));

https://codereview.chromium.org/12313118/diff/14002/ash/root_window_controlle...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/12313118/diff/14002/ash/root_window_controlle...
ash/root_window_controller.cc:287: if
(Shell::GetInstance()->delegate()->IsUserLoggedIn())
Are you sure that you want to move this into the "if
(IsLauncherPerDisplayEnabled...) "section?

https://codereview.chromium.org/12313118/diff/14002/ash/root_window_controlle...
ash/root_window_controller.cc:319:
panel_layout_manager_->SetLauncher(shelf_->launcher());
Shouldn't this be: if (panel_layout_manager_) {..} like it was in the old code?

https://codereview.chromium.org/12313118/diff/14002/ash/root_window_controlle...
ash/root_window_controller.cc:323: if (!shelf_->launcher())
Since shelf_ can also be NULL - shouldn't we test if it is != NULL?

https://codereview.chromium.org/12313118/diff/14002/ash/root_window_controlle...
ash/root_window_controller.cc:338: if (shelf_->status_area_widget())
same here.

https://codereview.chromium.org/12313118/diff/14002/ash/root_window_controlle...
ash/root_window_controller.cc:386: if (shelf_.get())
You don't need the if (shelf_.get()) here - you can always reset.

https://codereview.chromium.org/12313118/diff/14002/ash/root_window_controlle...
ash/root_window_controller.cc:426: return shelf_.get() ?
shelf_->shelf_layout_manager() : NULL;
Somehow this makes me worry. It could be that we return NULL here? So there are
a lot of calls which might crash (e.g. line 461). In the mentioned case I can
see that it would not be a new problem since shelf_ didn't get there tested in
the past either, but maybe there are other places where we should be more
careful?

https://codereview.chromium.org/12313118/diff/14002/ash/root_window_controller.h
File ash/root_window_controller.h (right):

https://codereview.chromium.org/12313118/diff/14002/ash/root_window_controlle...
ash/root_window_controller.h:83: ShelfWidget* shelf() const { return
shelf_.get(); }
Please add a short comment.

https://codereview.chromium.org/12313118/diff/14002/ash/root_window_controlle...
ash/root_window_controller.h:84: ShelfLayoutManager* shelf_layout_manager()
const;
Here as well. Note that the function might return NULL when shelf() is NULL.

https://codereview.chromium.org/12313118/diff/14002/ash/shelf/shelf_layout_ma...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/12313118/diff/14002/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager.cc:522: if (workspace_controller_) {
Looking at line 482 it appears that it can happen that shelf_ can be NULL (and
the calls to this function do not exclude the possibility either - unless I am
missing something), so shouldn't this be "if (shelf_ &&" ? And if so - what is
then with line 560-569, 596 - 563 (there used to be an "if (launcher_widget())"
around it as well? But again, maybe there is a reason that this cannot happen?

https://codereview.chromium.org/12313118/diff/14002/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager.cc:612: void ShelfLayoutManager::StopAnimating()
{
According to line 839 the existence of "shelf_" is similar to the existence of
"launcher_widget()". Since that test was done here as well in the past - 
shouldn't we also check that "shelf_" is not NULL?

https://codereview.chromium.org/12313118/diff/14002/ash/shelf/shelf_layout_ma...
File ash/shelf/shelf_layout_manager.h (right):

https://codereview.chromium.org/12313118/diff/14002/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager.h:5: #ifndef ASH_WM_SHELF_LAYOUT_MANAGER_H_
"WM_" -> "SHELF_"

https://codereview.chromium.org/12313118/diff/14002/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager.h:191: ShelfWidget* shelf_widget() const { return
shelf_; }
New line. Or better: Move up to the other accessor functions (125-126)

https://codereview.chromium.org/12313118/diff/14002/ash/shelf/shelf_widget.cc
File ash/shelf/shelf_widget.cc (right):

https://codereview.chromium.org/12313118/diff/14002/ash/shelf/shelf_widget.cc...
ash/shelf/shelf_widget.cc:260: // Inform the root window controller (for
PanelLayoutManager).
Not sure if we should mention that this is for the PanelLayoutManager. Seems
rather implementation specific and might change over time.

[tfarina, 2013-03-04 19:19:58]

https://codereview.chromium.org/12313118/diff/14002/ash/launcher/launcher.cc
File ash/launcher/launcher.cc (right):

https://codereview.chromium.org/12313118/diff/14002/ash/launcher/launcher.cc#...
ash/launcher/launcher.cc:63: Launcher::~Launcher() {
On 2013/03/04 19:18:04, Mr4D wrote:
> Since there isn't anything left you should possibly move this into the header
> file.
why? no, keep the declaration in the header and the definition in the source.
clang will probably complain otherwise.

[Harry McCleave, 2013-03-04 20:29:24]

https://codereview.chromium.org/12313118/diff/14002/ash/launcher/launcher_vie...
File ash/launcher/launcher_view_unittest.cc (right):

https://codereview.chromium.org/12313118/diff/14002/ash/launcher/launcher_vie...
ash/launcher/launcher_view_unittest.cc:185: gfx::Rect(0, 0, total_width,
shelf_size.height()));
On 2013/03/04 19:18:04, Mr4D wrote:
> I would do it like this:
> shelf_size.set_width(..);
> then 
> SetLauncherViewBounds(gfx::Rect(shelf_size));

Done.

https://codereview.chromium.org/12313118/diff/14002/ash/root_window_controlle...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/12313118/diff/14002/ash/root_window_controlle...
ash/root_window_controller.cc:287: if
(Shell::GetInstance()->delegate()->IsUserLoggedIn())
On 2013/03/04 19:18:04, Mr4D wrote:
> Are you sure that you want to move this into the "if
> (IsLauncherPerDisplayEnabled...) "section?

Done.

https://codereview.chromium.org/12313118/diff/14002/ash/root_window_controlle...
ash/root_window_controller.cc:319:
panel_layout_manager_->SetLauncher(shelf_->launcher());
On 2013/03/04 19:18:04, Mr4D wrote:
> Shouldn't this be: if (panel_layout_manager_) {..} like it was in the old
code?

Done.

https://codereview.chromium.org/12313118/diff/14002/ash/root_window_controlle...
ash/root_window_controller.cc:323: if (!shelf_->launcher())
On 2013/03/04 19:18:04, Mr4D wrote:
> Since shelf_ can also be NULL - shouldn't we test if it is != NULL?

Done.

https://codereview.chromium.org/12313118/diff/14002/ash/root_window_controlle...
ash/root_window_controller.cc:338: if (shelf_->status_area_widget())
On 2013/03/04 19:18:04, Mr4D wrote:
> same here.

Done.

https://codereview.chromium.org/12313118/diff/14002/ash/root_window_controlle...
ash/root_window_controller.cc:386: if (shelf_.get())
On 2013/03/04 19:18:04, Mr4D wrote:
> You don't need the if (shelf_.get()) here - you can always reset.

Done.

https://codereview.chromium.org/12313118/diff/14002/ash/root_window_controlle...
ash/root_window_controller.cc:426: return shelf_.get() ?
shelf_->shelf_layout_manager() : NULL;
Currently the classes that use this only exist once the shelf (and thus layout
manager) have been created so the assumption of the layout manager existing
was/is ok, however as more functions (eg helper alignment functions) get moved
from the layout manager to the shelf_widget this shortcut will be used less
(read hopefully not at all). 
On 2013/03/04 19:18:04, Mr4D wrote:
> Somehow this makes me worry. It could be that we return NULL here? So there
are
> a lot of calls which might crash (e.g. line 461). In the mentioned case I can
> see that it would not be a new problem since shelf_ didn't get there tested in
> the past either, but maybe there are other places where we should be more
> careful?

https://codereview.chromium.org/12313118/diff/14002/ash/root_window_controller.h
File ash/root_window_controller.h (right):

https://codereview.chromium.org/12313118/diff/14002/ash/root_window_controlle...
ash/root_window_controller.h:83: ShelfWidget* shelf() const { return
shelf_.get(); }
On 2013/03/04 19:18:04, Mr4D wrote:
> Please add a short comment.

Done.

https://codereview.chromium.org/12313118/diff/14002/ash/root_window_controlle...
ash/root_window_controller.h:84: ShelfLayoutManager* shelf_layout_manager()
const;
On 2013/03/04 19:18:04, Mr4D wrote:
> Here as well. Note that the function might return NULL when shelf() is NULL.

Done.

https://codereview.chromium.org/12313118/diff/14002/ash/shelf/shelf_layout_ma...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/12313118/diff/14002/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager.cc:522: if (workspace_controller_) {
I have removed the shelf_ test from 482, the shelf will always exist if the
layout manager exists, the launcher however may not (which is why when the
widget[shelf] was a child of launcher this was not certain).

On 2013/03/04 19:18:04, Mr4D wrote:
> Looking at line 482 it appears that it can happen that shelf_ can be NULL (and
> the calls to this function do not exclude the possibility either - unless I am
> missing something), so shouldn't this be "if (shelf_ &&" ? And if so - what is
> then with line 560-569, 596 - 563 (there used to be an "if
(launcher_widget())"
> around it as well? But again, maybe there is a reason that this cannot happen?

https://codereview.chromium.org/12313118/diff/14002/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager.cc:612: void ShelfLayoutManager::StopAnimating()
{
See above comment.

On 2013/03/04 19:18:04, Mr4D wrote:
> According to line 839 the existence of "shelf_" is similar to the existence of
> "launcher_widget()". Since that test was done here as well in the past - 
> shouldn't we also check that "shelf_" is not NULL?

https://codereview.chromium.org/12313118/diff/14002/ash/shelf/shelf_layout_ma...
File ash/shelf/shelf_layout_manager.h (right):

https://codereview.chromium.org/12313118/diff/14002/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager.h:191: ShelfWidget* shelf_widget() const { return
shelf_; }
On 2013/03/04 19:18:04, Mr4D wrote:
> New line. Or better: Move up to the other accessor functions (125-126)

Done.

https://codereview.chromium.org/12313118/diff/14002/ash/shelf/shelf_widget.cc
File ash/shelf/shelf_widget.cc (right):

https://codereview.chromium.org/12313118/diff/14002/ash/shelf/shelf_widget.cc...
ash/shelf/shelf_widget.cc:260: // Inform the root window controller (for
PanelLayoutManager).
On 2013/03/04 19:18:04, Mr4D wrote:
> Not sure if we should mention that this is for the PanelLayoutManager. Seems
> rather implementation specific and might change over time.

Done.

[Mr4D (OOO till 08-26), 2013-03-04 22:59:00]

lgtm

A few more nits and then lgtm

https://codereview.chromium.org/12313118/diff/14002/ash/root_window_controlle...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/12313118/diff/14002/ash/root_window_controlle...
ash/root_window_controller.cc:426: return shelf_.get() ?
shelf_->shelf_layout_manager() : NULL;
Okay. Sounds good to me then.

https://codereview.chromium.org/12313118/diff/14002/ash/shelf/shelf_layout_ma...
File ash/shelf/shelf_layout_manager.h (right):

https://codereview.chromium.org/12313118/diff/14002/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager.h:5: #ifndef ASH_WM_SHELF_LAYOUT_MANAGER_H_
I think you have missed this one.

https://codereview.chromium.org/12313118/diff/12097/ash/shelf/shelf_layout_ma...
File ash/shelf/shelf_layout_manager.h (right):

https://codereview.chromium.org/12313118/diff/12097/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager.h:6: #define ASH_WM_SHELF_LAYOUT_MANAGER_H_
As before: "..WM_.." -> "..SHELF_.."

https://codereview.chromium.org/12313118/diff/12097/ash/system/tray/tray_back...
File ash/system/tray/tray_background_view.cc (right):

https://codereview.chromium.org/12313118/diff/12097/ash/system/tray/tray_back...
ash/system/tray/tray_background_view.cc:28: namespace {
One new line between namespace and the first item.

[Harry McCleave, 2013-03-05 05:59:19]

Ready for Owner review.

https://codereview.chromium.org/12313118/diff/12097/ash/shelf/shelf_layout_ma...
File ash/shelf/shelf_layout_manager.h (right):

https://codereview.chromium.org/12313118/diff/12097/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager.h:6: #define ASH_WM_SHELF_LAYOUT_MANAGER_H_
On 2013/03/04 22:59:00, Mr4D wrote:
> As before: "..WM_.." -> "..SHELF_.."

Done.

https://codereview.chromium.org/12313118/diff/12097/ash/system/tray/tray_back...
File ash/system/tray/tray_background_view.cc (right):

https://codereview.chromium.org/12313118/diff/12097/ash/system/tray/tray_back...
ash/system/tray/tray_background_view.cc:28: namespace {
On 2013/03/04 22:59:00, Mr4D wrote:
> One new line between namespace and the first item.

Done.

[James Cook, 2013-03-05 18:17:01]

I should get to it this afternoon.

[James Cook, 2013-03-05 20:30:38]

Almost there!  This is a good cleanup.

When this is all done I think you should submit ShelfWidget and tests for a C++
readability review, if you haven't done that yet.

https://codereview.chromium.org/12313118/diff/27001/ash/focus_cycler_unittest.cc
File ash/focus_cycler_unittest.cc (right):

https://codereview.chromium.org/12313118/diff/27001/ash/focus_cycler_unittest...
ash/focus_cycler_unittest.cc:130: // Cycle focus to the shelf
nit: Period at end of comment.

https://codereview.chromium.org/12313118/diff/27001/ash/focus_cycler_unittest...
ash/focus_cycler_unittest.cc:172: // Cycle focus to the shelf
ditto

https://codereview.chromium.org/12313118/diff/27001/ash/launcher/launcher.cc
File ash/launcher/launcher.cc (right):

https://codereview.chromium.org/12313118/diff/27001/ash/launcher/launcher.cc#...
ash/launcher/launcher.cc:75:
internal::RootWindowController::ForLauncher(window)->shelf()->launcher();
Break line after ->

https://codereview.chromium.org/12313118/diff/27001/ash/launcher/launcher.cc#...
ash/launcher/launcher.cc:81: // ShelfLayoutManager will resize the launcher.
Does the resize handle scheduling a paint?

https://codereview.chromium.org/12313118/diff/27001/ash/launcher/launcher.cc#...
ash/launcher/launcher.cc:180: if (launcher_view_)
launcher_view_ seems to be created in the constructor. Do you need these if's?

https://codereview.chromium.org/12313118/diff/27001/ash/launcher/launcher.h
File ash/launcher/launcher.h (right):

https://codereview.chromium.org/12313118/diff/27001/ash/launcher/launcher.h#n...
ash/launcher/launcher.h:83: bool visible() const;
IsVisible() since it is not inline. Or inline it.

https://codereview.chromium.org/12313118/diff/27001/ash/launcher/launcher.h#n...
ash/launcher/launcher.h:97: ShelfWidget* shelf_widget() const { return
shelf_widget_; }
Generally getters that return a pointer to an internal object are not const,
since the caller can modify the underlying object freely.

https://codereview.chromium.org/12313118/diff/27001/ash/launcher/launcher.h#n...
ash/launcher/launcher.h:101: gfx::Rect launcher_view_bounds() const;
GetLauncherViewBounds(). Only inline functions should be linux_hacker_style.

https://codereview.chromium.org/12313118/diff/27001/ash/root_window_controlle...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/12313118/diff/27001/ash/root_window_controlle...
ash/root_window_controller.cc:366: // TODO(HarryM): Remove when Status Area
Widget is a child view.
HarryM -> harrym

https://codereview.chromium.org/12313118/diff/27001/ash/root_window_controller.h
File ash/root_window_controller.h (right):

https://codereview.chromium.org/12313118/diff/27001/ash/root_window_controlle...
ash/root_window_controller.h:85: ShelfWidget* shelf() const { return
shelf_.get(); }
no const

https://codereview.chromium.org/12313118/diff/27001/ash/root_window_controlle...
ash/root_window_controller.h:89: ShelfLayoutManager* shelf_layout_manager()
const;
inline or GetShelfLayoutManager

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_layout_ma...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager.cc:597: // TODO(harrym): Remove when status_area
is view.
Do you have a bug open for this "convert to view" item? If so, reference it
here.

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager.cc:664: target_bounds->shelf_bounds_in_root =
SelectValueForShelfAlignment(
Wow. I hope there are unit tests for this. :-)

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_layout_ma...
File ash/shelf/shelf_layout_manager.h (right):

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager.h:121: ShelfWidget* shelf_widget() const { return
shelf_; }
no const

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_layout_ma...
File ash/shelf/shelf_layout_manager_unittest.cc (right):

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager_unittest.cc:59: }
newline after }

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager_unittest.cc:619: TEST_F(ShelfLayoutManagerTest,
MAYBE_SetAlignment) {
Hooray, there are tests!

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_widget.cc
File ash/shelf/shelf_widget.cc (right):

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_widget.cc...
ash/shelf/shelf_widget.cc:71: // views::View overrides
overrides: with colon

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_widget.cc...
ash/shelf/shelf_widget.cc:82: bool CanActivate() const OVERRIDE {
Maybe don't inline these, since they are long and branchy?

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_widget.cc...
ash/shelf/shelf_widget.cc:93: void Layout() OVERRIDE {
ditto

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_widget.cc...
ash/shelf/shelf_widget.cc:97: child_at(i)->width(),height());
space after commas

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_widget.cc...
ash/shelf/shelf_widget.cc:254: Shell::GetInstance()->GetLauncherDelegate(),
Either cache a pointer to Shell::GetInstance() and left-align these, or wrap all
to next line and indent 4.

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_widget.cc...
ash/shelf/shelf_widget.cc:258: ShellDelegate* delegate =
Shell::GetInstance()->delegate();
Can this be moved down near where delegate is used?

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_widget.h
File ash/shelf/shelf_widget.h (right):

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_widget.h#...
ash/shelf/shelf_widget.h:37: // Set the alignment of the shelf.
This comment is not useful. Either explain that layout is left/right/etc or
delete it.

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_widget.h#...
ash/shelf/shelf_widget.h:41: // Sets/gets the shelf auto-hide behavior.
ditto

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_widget.h#...
ash/shelf/shelf_widget.h:45: // Sets whether the shelf paints a background.
Default is false, but is set
For example, this is a much more useful comment.

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_widget.h#...
ash/shelf/shelf_widget.h:68: // Set visibility of the launcher component is
visible on the shelf.
"Set visibility of the launcher component on the shelf." ?

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_widget.h#...
ash/shelf/shelf_widget.h:70: bool LauncherVisible() const;
Maybe IsLauncherVisible()?

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_widget.h#...
ash/shelf/shelf_widget.h:84: aura::Window* window_container() { return
window_container_; }
Hooray, no const!

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_widget.h#...
ash/shelf/shelf_widget.h:92: DelegateView* delegate_view_;
Comment on the memory ownership of this view (since you aren't explicitly
cleaning it up).

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_widget_un...
File ash/shelf/shelf_widget_unittest.cc (right):

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_widget_un...
ash/shelf/shelf_widget_unittest.cc:139: // Test only makes sense if the status
is > 0, which is better be.
"it better be"

https://codereview.chromium.org/12313118/diff/27001/ash/wm/panel_layout_manag...
File ash/wm/panel_layout_manager.cc (right):

https://codereview.chromium.org/12313118/diff/27001/ash/wm/panel_layout_manag...
ash/wm/panel_layout_manager.cc:134:
aura::client::GetActivationClient(panel_container_->GetRootWindow())->
Is this part of the same patch? It looks OK but I'm not sure if it's related?

https://codereview.chromium.org/12313118/diff/27001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_cycler_animator.cc (right):

https://codereview.chromium.org/12313118/diff/27001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_cycler_animator.cc:664: aura::Window* launcher_window
= workspaces_[0]->window();
Are you sure this is right? The old code is looking for the launcher's widget's
native window, whereas your code is getting the workspace's desktop window.

https://codereview.chromium.org/12313118/diff/27001/ui/compositor/layer_anima...
File ui/compositor/layer_animation_element.cc (right):

https://codereview.chromium.org/12313118/diff/27001/ui/compositor/layer_anima...
ui/compositor/layer_animation_element.cc:558: t = std::max(0.0,
elapsed.InMillisecondsF() / duration_.InMillisecondsF());
Maybe comment why this is needed?

[Mr4D (OOO till 08-26), 2013-03-05 20:42:10]

One other tiny nit.

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_widget.cc
File ash/shelf/shelf_widget.cc (right):

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_widget.cc...
ash/shelf/shelf_widget.cc:97: child_at(i)->width(),height());
And also but {} around this since it spawns two lines.

[Harry McCleave, 2013-03-06 01:59:49]

https://codereview.chromium.org/12313118/diff/27001/ash/focus_cycler_unittest.cc
File ash/focus_cycler_unittest.cc (right):

https://codereview.chromium.org/12313118/diff/27001/ash/focus_cycler_unittest...
ash/focus_cycler_unittest.cc:130: // Cycle focus to the shelf
On 2013/03/05 20:30:38, James Cook (Chromium) wrote:
> nit: Period at end of comment.

Done.

https://codereview.chromium.org/12313118/diff/27001/ash/focus_cycler_unittest...
ash/focus_cycler_unittest.cc:172: // Cycle focus to the shelf
On 2013/03/05 20:30:38, James Cook (Chromium) wrote:
> ditto

Done.

https://codereview.chromium.org/12313118/diff/27001/ash/launcher/launcher.cc
File ash/launcher/launcher.cc (right):

https://codereview.chromium.org/12313118/diff/27001/ash/launcher/launcher.cc#...
ash/launcher/launcher.cc:75:
internal::RootWindowController::ForLauncher(window)->shelf()->launcher();
On 2013/03/05 20:30:38, James Cook (Chromium) wrote:
> Break line after ->

Done.

https://codereview.chromium.org/12313118/diff/27001/ash/launcher/launcher.cc#...
ash/launcher/launcher.cc:81: // ShelfLayoutManager will resize the launcher.
On 2013/03/05 20:30:38, James Cook (Chromium) wrote:
> Does the resize handle scheduling a paint?

Yes and no, the shelf widget is repainted, however as issue 173298 identifies,
the launcher_view is not but that will be addressed in another patch.

https://codereview.chromium.org/12313118/diff/27001/ash/launcher/launcher.cc#...
ash/launcher/launcher.cc:180: if (launcher_view_)
On 2013/03/05 20:30:38, James Cook (Chromium) wrote:
> launcher_view_ seems to be created in the constructor. Do you need these if's?

Done.

https://codereview.chromium.org/12313118/diff/27001/ash/launcher/launcher.h
File ash/launcher/launcher.h (right):

https://codereview.chromium.org/12313118/diff/27001/ash/launcher/launcher.h#n...
ash/launcher/launcher.h:83: bool visible() const;
On 2013/03/05 20:30:38, James Cook (Chromium) wrote:
> IsVisible() since it is not inline. Or inline it.

Done.

https://codereview.chromium.org/12313118/diff/27001/ash/launcher/launcher.h#n...
ash/launcher/launcher.h:97: ShelfWidget* shelf_widget() const { return
shelf_widget_; }
On 2013/03/05 20:30:38, James Cook (Chromium) wrote:
> Generally getters that return a pointer to an internal object are not const,
> since the caller can modify the underlying object freely.

Done.

https://codereview.chromium.org/12313118/diff/27001/ash/launcher/launcher.h#n...
ash/launcher/launcher.h:101: gfx::Rect launcher_view_bounds() const;
On 2013/03/05 20:30:38, James Cook (Chromium) wrote:
> GetLauncherViewBounds(). Only inline functions should be linux_hacker_style.

Done.

https://codereview.chromium.org/12313118/diff/27001/ash/root_window_controlle...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/12313118/diff/27001/ash/root_window_controlle...
ash/root_window_controller.cc:366: // TODO(HarryM): Remove when Status Area
Widget is a child view.
On 2013/03/05 20:30:38, James Cook (Chromium) wrote:
> HarryM -> harrym

Done.

https://codereview.chromium.org/12313118/diff/27001/ash/root_window_controller.h
File ash/root_window_controller.h (right):

https://codereview.chromium.org/12313118/diff/27001/ash/root_window_controlle...
ash/root_window_controller.h:85: ShelfWidget* shelf() const { return
shelf_.get(); }
On 2013/03/05 20:30:38, James Cook (Chromium) wrote:
> no const

Done.

https://codereview.chromium.org/12313118/diff/27001/ash/root_window_controlle...
ash/root_window_controller.h:89: ShelfLayoutManager* shelf_layout_manager()
const;
On 2013/03/05 20:30:38, James Cook (Chromium) wrote:
> inline or GetShelfLayoutManager

Done.

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_layout_ma...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager.cc:597: // TODO(harrym): Remove when status_area
is view.
On 2013/03/05 20:30:38, James Cook (Chromium) wrote:
> Do you have a bug open for this "convert to view" item? If so, reference it
> here.

Created: https://code.google.com/p/chromium/issues/detail?id=180422

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager.cc:664: target_bounds->shelf_bounds_in_root =
SelectValueForShelfAlignment(
On 2013/03/05 20:30:38, James Cook (Chromium) wrote:
> Wow. I hope there are unit tests for this. :-)
ShelfWidgetTest.LauncherInitiallySized
ShelfLayoutManagerTest.MAYBE_AutoHide [Maybe=-windows]
ShelfLayoutManagerTest.SetAutoHideBehavior
ShelfLayoutManagerTest.MAYBE_SetAlignment [Maybe=-windows]
ScreenAshTest.Bounds
All of these tests make assertions around the various states, positions and
sizes described within this [now slightly smaller] mess.

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_layout_ma...
File ash/shelf/shelf_layout_manager.h (right):

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager.h:121: ShelfWidget* shelf_widget() const { return
shelf_; }
On 2013/03/05 20:30:38, James Cook (Chromium) wrote:
> no const

Done.

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_layout_ma...
File ash/shelf/shelf_layout_manager_unittest.cc (right):

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager_unittest.cc:59: }
On 2013/03/05 20:30:38, James Cook (Chromium) wrote:
> newline after }

Done.

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager_unittest.cc:619: TEST_F(ShelfLayoutManagerTest,
MAYBE_SetAlignment) {
On 2013/03/05 20:30:38, James Cook (Chromium) wrote:
> Hooray, there are tests!
:)

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_widget.cc
File ash/shelf/shelf_widget.cc (right):

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_widget.cc...
ash/shelf/shelf_widget.cc:71: // views::View overrides
On 2013/03/05 20:30:38, James Cook (Chromium) wrote:
> overrides: with colon

Done.

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_widget.cc...
ash/shelf/shelf_widget.cc:82: bool CanActivate() const OVERRIDE {
On 2013/03/05 20:30:38, James Cook (Chromium) wrote:
> Maybe don't inline these, since they are long and branchy?

Done.

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_widget.cc...
ash/shelf/shelf_widget.cc:93: void Layout() OVERRIDE {
On 2013/03/05 20:30:38, James Cook (Chromium) wrote:
> ditto

Done.

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_widget.cc...
ash/shelf/shelf_widget.cc:97: child_at(i)->width(),height());
On 2013/03/05 20:30:38, James Cook (Chromium) wrote:
> space after commas

Done.

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_widget.cc...
ash/shelf/shelf_widget.cc:254: Shell::GetInstance()->GetLauncherDelegate(),
On 2013/03/05 20:30:38, James Cook (Chromium) wrote:
> Either cache a pointer to Shell::GetInstance() and left-align these, or wrap
all
> to next line and indent 4.

Done.

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_widget.cc...
ash/shelf/shelf_widget.cc:258: ShellDelegate* delegate =
Shell::GetInstance()->delegate();
On 2013/03/05 20:30:38, James Cook (Chromium) wrote:
> Can this be moved down near where delegate is used?

Done.

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_widget.h
File ash/shelf/shelf_widget.h (right):

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_widget.h#...
ash/shelf/shelf_widget.h:37: // Set the alignment of the shelf.
On 2013/03/05 20:30:38, James Cook (Chromium) wrote:
> This comment is not useful. Either explain that layout is left/right/etc or
> delete it.

Done.

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_widget.h#...
ash/shelf/shelf_widget.h:41: // Sets/gets the shelf auto-hide behavior.
On 2013/03/05 20:30:38, James Cook (Chromium) wrote:
> ditto

Done.

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_widget.h#...
ash/shelf/shelf_widget.h:68: // Set visibility of the launcher component is
visible on the shelf.
On 2013/03/05 20:30:38, James Cook (Chromium) wrote:
> "Set visibility of the launcher component on the shelf." ?

I can has english readability plz?

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_widget.h#...
ash/shelf/shelf_widget.h:70: bool LauncherVisible() const;
On 2013/03/05 20:30:38, James Cook (Chromium) wrote:
> Maybe IsLauncherVisible()?

Done.

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_widget.h#...
ash/shelf/shelf_widget.h:92: DelegateView* delegate_view_;
On 2013/03/05 20:30:38, James Cook (Chromium) wrote:
> Comment on the memory ownership of this view (since you aren't explicitly
> cleaning it up).

Done.

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_widget_un...
File ash/shelf/shelf_widget_unittest.cc (right):

https://codereview.chromium.org/12313118/diff/27001/ash/shelf/shelf_widget_un...
ash/shelf/shelf_widget_unittest.cc:139: // Test only makes sense if the status
is > 0, which is better be.
On 2013/03/05 20:30:38, James Cook (Chromium) wrote:
> "it better be"

Done.

https://codereview.chromium.org/12313118/diff/27001/ash/wm/panel_layout_manag...
File ash/wm/panel_layout_manager.cc (right):

https://codereview.chromium.org/12313118/diff/27001/ash/wm/panel_layout_manag...
ash/wm/panel_layout_manager.cc:134:
aura::client::GetActivationClient(panel_container_->GetRootWindow())->
On 2013/03/05 20:30:38, James Cook (Chromium) wrote:
> Is this part of the same patch? It looks OK but I'm not sure if it's related?

Done.

https://codereview.chromium.org/12313118/diff/27001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_cycler_animator.cc (right):

https://codereview.chromium.org/12313118/diff/27001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_cycler_animator.cc:664: aura::Window* launcher_window
= workspaces_[0]->window();
On 2013/03/05 20:30:38, James Cook (Chromium) wrote:
> Are you sure this is right? The old code is looking for the launcher's
widget's
> native window, whereas your code is getting the workspace's desktop window.

Done.

https://codereview.chromium.org/12313118/diff/27001/ui/compositor/layer_anima...
File ui/compositor/layer_animation_element.cc (right):

https://codereview.chromium.org/12313118/diff/27001/ui/compositor/layer_anima...
ui/compositor/layer_animation_element.cc:558: t = std::max(0.0,
elapsed.InMillisecondsF() / duration_.InMillisecondsF());
On 2013/03/05 20:30:38, James Cook (Chromium) wrote:
> Maybe comment why this is needed?

Removed this, it was a place holder for the proper fix which vollick@ has now
submitted.

[James Cook, 2013-03-06 02:39:03]

LGTM with one nit. Patch it, land it, and let the pain be over.

https://codereview.chromium.org/12313118/diff/45001/ash/shelf/shelf_widget.cc
File ash/shelf/shelf_widget.cc (right):

https://codereview.chromium.org/12313118/diff/45001/ash/shelf/shelf_widget.cc...
ash/shelf/shelf_widget.cc:172: for(int i = 0; i < this->child_count(); ++i) {
nit: Don't need "this", just child_count()

[commit-bot: I haz the power, 2013-03-06 03:27:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/harrym@chromium.org/12313118/74001

[commit-bot: I haz the power, 2013-03-06 03:28:09]

Presubmit check for 12313118-74001 failed and returned exit status 1.

INFO:root:Found 88 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/ash/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/ui/views/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/browser/ui/views/immersive_mode_controller_browsertest.cc

Presubmit checks took 6.9s to calculate.

[commit-bot: I haz the power, 2013-03-06 03:33:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/harrym@chromium.org/12313118/74001

[commit-bot: I haz the power, 2013-03-06 03:57:46]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[Ben Goodger (Google), 2013-03-06 17:49:24]

lgtm for c/b/u/v

[commit-bot: I haz the power, 2013-03-07 01:05:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/harrym@chromium.org/12313118/92001

[commit-bot: I haz the power, 2013-03-07 02:12:59]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-03-07 22:34:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/harrym@chromium.org/12313118/104001

[commit-bot: I haz the power, 2013-03-07 22:35:12]

Failed to apply patch for ash/wm/panel_layout_manager.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file ash/wm/panel_layout_manager.cc
  Hunk #1 FAILED at 9.
  Hunk #2 FAILED at 136.
  Hunk #3 FAILED at 148.
  Hunk #4 FAILED at 322.
  4 out of 4 hunks FAILED -- saving rejects to file
ash/wm/panel_layout_manager.cc.rej

Patch:       ash/wm/panel_layout_manager.cc
Index: ash/wm/panel_layout_manager.cc
diff --git a/ash/wm/panel_layout_manager.cc b/ash/wm/panel_layout_manager.cc
index
54df0c29dae1efc4c3df52235e44e363ed2040d0..60e47ef8f432d1fb6f0e685e70b00e8102e0f4c0
100644
--- a/ash/wm/panel_layout_manager.cc
+++ b/ash/wm/panel_layout_manager.cc
@@ -9,6 +9,7 @@
 
 #include "ash/launcher/launcher.h"
 #include "ash/screen_ash.h"
+#include "ash/shelf/shelf_widget.h"
 #include "ash/shell.h"
 #include "ash/wm/frame_painter.h"
 #include "ash/wm/property_util.h"
@@ -136,8 +137,6 @@ PanelLayoutManager::PanelLayoutManager(aura::Window*
panel_container)
 
 PanelLayoutManager::~PanelLayoutManager() {
   Shutdown();
-  if (launcher_)
-    launcher_->RemoveIconObserver(this);
   aura::client::GetActivationClient(Shell::GetPrimaryRootWindow())->
       RemoveObserver(this);
 }
@@ -148,6 +147,9 @@ void PanelLayoutManager::Shutdown() {
     delete iter->callout_widget;
   }
   panel_windows_.clear();
+  if (launcher_)
+    launcher_->RemoveIconObserver(this);
+  launcher_ = NULL;
 }
 
 void PanelLayoutManager::StartDragging(aura::Window* panel) {
@@ -322,14 +324,14 @@ void PanelLayoutManager::RestorePanel(aura::Window* panel)
{
 }
 
 void PanelLayoutManager::Relayout() {
-  if (!launcher_ || !launcher_->widget())
+  if (!launcher_ || !launcher_->shelf_widget())
     return;
 
   if (in_layout_)
     return;
   base::AutoReset<bool> auto_reset_in_layout(&in_layout_, true);
 
-  int launcher_top = launcher_->widget()->GetWindowBoundsInScreen().y();
+  int launcher_top = launcher_->shelf_widget()->GetWindowBoundsInScreen().y();
   int panel_left_bounds = kPanelIdealSpacing;
   int panel_right_bounds =
       panel_container_->bounds().width() - kPanelIdealSpacing;

[commit-bot: I haz the power, 2013-03-07 23:54:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/harrym@chromium.org/12313118/106001

[commit-bot: I haz the power, 2013-03-08 01:32:38]

Retried try job too often on linux_chromeos for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-03-08 03:26:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/harrym@chromium.org/12313118/123001

[commit-bot: I haz the power, 2013-03-08 04:42:38]

Retried try job too often on linux_chromeos for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-03-08 23:42:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/harrym@chromium.org/12313118/76004

[commit-bot: I haz the power, 2013-03-09 02:51:03]

Change committed as 187122