ash: Fix variable names and setters in ShelfLayoutManager and tests

ash: Fix variable names and setters in ShelfLayoutManager and tests

Calls to set shelf state should go through Shelf, not ShelfLayoutManager.
* Move all calls to SetAutoHideBehavior() and SetAlignment() to Shelf instead
of ShelfLayoutManager.
* Add Shelf::GetVisibilityState() and Shelf::GetAutoHideState() and prefer
using them to accessing ShelfLayoutManager directly.
* Remove ShelfLayoutManager::GetAutoHideBehavior().
* Rename variables mis-named "shelf" to shelf_widget, shelf_layout_manager, etc.
so they match their actual types.

No functional changes.

BUG=615502
TEST=ash_unittests

Committed: https://crrev.com/cf8b648fcece89a78eec21726a1aa48667778dbe
Cr-Commit-Position: refs/heads/master@{#397214}

[James Cook, 2016-05-31 01:38:18]

jamescook@chromium.org changed reviewers:
+ msw@chromium.org

[James Cook, 2016-05-31 01:38:19]

msw, please take a look

I wouldn't spend too much time in shelf_layout_manager_unittest.cc -- it's just
a ton of tedious renames.

[James Cook, 2016-05-31 01:38:26]

The CQ bit was checked by jamescook@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-31 01:38:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2017413002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2017413002/20001

[commit-bot: I haz the power, 2016-05-31 02:57:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-31 02:57:57]

Dry run: This issue passed the CQ dry run.

[msw, 2016-06-01 00:53:24]

Just nits and qs. This makes me want to move more accessors from
ShelfLayoutManager to Shef (auto_hide_state, visibility, etc.).

https://codereview.chromium.org/2017413002/diff/40001/ash/root_window_control...
File ash/root_window_controller_unittest.cc (right):

https://codereview.chromium.org/2017413002/diff/40001/ash/root_window_control...
ash/root_window_controller_unittest.cc:139:
shelf->SetAutoHideBehavior(SHELF_AUTO_HIDE_BEHAVIOR_ALWAYS);
q: is this even necessary for the test?

https://codereview.chromium.org/2017413002/diff/40001/ash/screen_util_unittes...
File ash/screen_util_unittest.cc (right):

https://codereview.chromium.org/2017413002/diff/40001/ash/screen_util_unittes...
ash/screen_util_unittest.cc:10: #include "ash/shelf/shelf_widget.h"
nit: remove if not needed?

https://codereview.chromium.org/2017413002/diff/40001/ash/screen_util_unittes...
ash/screen_util_unittest.cc:31:
Shell::GetPrimaryRootWindowController()->GetShelf()->SetAutoHideBehavior(
nit: Shelf::ForPrimaryDisplay()?

https://codereview.chromium.org/2017413002/diff/40001/ash/screen_util_unittes...
ash/screen_util_unittest.cc:31:
Shell::GetPrimaryRootWindowController()->GetShelf()->SetAutoHideBehavior(
q: is this even needed for the test?

https://codereview.chromium.org/2017413002/diff/40001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager_unittest.cc (right):

https://codereview.chromium.org/2017413002/diff/40001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager_unittest.cc:67: return
Shell::GetPrimaryRootWindowController()->shelf_widget();
optional nit: return GetShelf()->shelf_widget();?

https://codereview.chromium.org/2017413002/diff/40001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager_unittest.cc:327: void
SetState(ShelfLayoutManager* layout_manager,
nit: remove this and inline ShelfLayoutManager::SetState() calls?

https://codereview.chromium.org/2017413002/diff/40001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager_unittest.cc:1428: ShelfLayoutManager* shelf_1 =
nit: layout_manager_1 (and layout_manager_2 below)?

https://codereview.chromium.org/2017413002/diff/40001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager_unittest.cc:2240: Shelf* shelf = GetShelf();
nit: it seems random when you choose to cache GetShelf or call it repeatedly.

https://codereview.chromium.org/2017413002/diff/40001/ash/shelf/shelf_view_un...
File ash/shelf/shelf_view_unittest.cc (right):

https://codereview.chromium.org/2017413002/diff/40001/ash/shelf/shelf_view_un...
ash/shelf/shelf_view_unittest.cc:21: #include "ash/shelf/shelf_layout_manager.h"
nit: remove

https://codereview.chromium.org/2017413002/diff/40001/ash/system/tray/system_...
File ash/system/tray/system_tray_unittest.cc (right):

https://codereview.chromium.org/2017413002/diff/40001/ash/system/tray/system_...
ash/system/tray/system_tray_unittest.cc:11: #include
"ash/shelf/shelf_layout_manager.h"
nit: include shelf.h instead?

https://codereview.chromium.org/2017413002/diff/40001/ash/system/user/tray_us...
File ash/system/user/tray_user_unittest.cc (right):

https://codereview.chromium.org/2017413002/diff/40001/ash/system/user/tray_us...
ash/system/user/tray_user_unittest.cc:57: ShelfLayoutManager*
shelf_layout_manager_;
nit: remove member altogether; it's only used once.

https://codereview.chromium.org/2017413002/diff/40001/ash/wm/maximize_mode/ma...
File ash/wm/maximize_mode/maximize_mode_window_manager_unittest.cc (right):

https://codereview.chromium.org/2017413002/diff/40001/ash/wm/maximize_mode/ma...
ash/wm/maximize_mode/maximize_mode_window_manager_unittest.cc:940: Shelf* shelf
= Shelf::ForPrimaryDisplay();
nit: why cache here but inline above? consistency is nice.

https://codereview.chromium.org/2017413002/diff/40001/ash/wm/panels/panel_win...
File ash/wm/panels/panel_window_resizer_unittest.cc (right):

https://codereview.chromium.org/2017413002/diff/40001/ash/wm/panels/panel_win...
ash/wm/panels/panel_window_resizer_unittest.cc:273: ShelfLayoutManager*
layout_manager = shelf->shelf_layout_manager();
nit: inline below?

https://codereview.chromium.org/2017413002/diff/40001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_window_resizer_unittest.cc (right):

https://codereview.chromium.org/2017413002/diff/40001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_window_resizer_unittest.cc:8: #include
"ash/root_window_controller.h"
nit: remove

https://codereview.chromium.org/2017413002/diff/40001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_window_resizer_unittest.cc:10: #include
"ash/shelf/shelf_layout_manager.h"
nit: remove

https://codereview.chromium.org/2017413002/diff/40001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_window_resizer_unittest.cc:189: ShelfLayoutManager*
shelf_layout_manager() {
nit: remove

https://codereview.chromium.org/2017413002/diff/40001/ash/wm/workspace_contro...
File ash/wm/workspace_controller_unittest.cc (right):

https://codereview.chromium.org/2017413002/diff/40001/ash/wm/workspace_contro...
ash/wm/workspace_controller_unittest.cc:676: Shelf* shelf =
Shelf::ForPrimaryDisplay();
ditto nit: inline/cache consistency (in this file)

[James Cook, 2016-06-01 15:45:22]

Description was changed from

==========
ash: Fix variable names and setters in ShelfLayoutManager and tests

Calls to set shelf state should go through Shelf, not ShelfLayoutManager.
* Move all calls to SetAutoHideBehavior() and SetAlignment() to Shelf instead
of ShelfLayoutManager.
* Remove ShelfLayoutManager::GetAutoHideBehavior().
* Rename variables mis-named "shelf" to shelf_widget, shelf_layout_manager, etc.
so they match their actual types.

No functional changes.

BUG=615502
TEST=ash_unittests
==========

to

==========
ash: Fix variable names and setters in ShelfLayoutManager and tests

Calls to set shelf state should go through Shelf, not ShelfLayoutManager.
* Move all calls to SetAutoHideBehavior() and SetAlignment() to Shelf instead
of ShelfLayoutManager.
* Add Shelf::GetVisibilityState() and Shelf::GetAutoHideState() and prefer
using them to accessing ShelfLayoutManager directly.
* Remove ShelfLayoutManager::GetAutoHideBehavior().
* Rename variables mis-named "shelf" to shelf_widget, shelf_layout_manager, etc.
so they match their actual types.

No functional changes.

BUG=615502
TEST=ash_unittests
==========

[James Cook, 2016-06-01 15:53:07]

msw, please take another look. I went ahead and added
Shelf::GetVisibilityState() and Shelf::GetAutoHideState() now. I think we'll
want those eventually and adding them touches exactly the same lines as the
renames in this CL, so I did it all at once.

https://codereview.chromium.org/2017413002/diff/40001/ash/root_window_control...
File ash/root_window_controller_unittest.cc (right):

https://codereview.chromium.org/2017413002/diff/40001/ash/root_window_control...
ash/root_window_controller_unittest.cc:139:
shelf->SetAutoHideBehavior(SHELF_AUTO_HIDE_BEHAVIOR_ALWAYS);
On 2016/06/01 00:53:23, msw wrote:
> q: is this even necessary for the test?

Removed and adjusted the expected bounds below. I kept hard-coded bounds rather
than trying to dynamically compute it based on kShelfSize, as that's similar to
what was there before.

https://codereview.chromium.org/2017413002/diff/40001/ash/screen_util_unittes...
File ash/screen_util_unittest.cc (right):

https://codereview.chromium.org/2017413002/diff/40001/ash/screen_util_unittes...
ash/screen_util_unittest.cc:10: #include "ash/shelf/shelf_widget.h"
On 2016/06/01 00:53:23, msw wrote:
> nit: remove if not needed?

Done.

https://codereview.chromium.org/2017413002/diff/40001/ash/screen_util_unittes...
ash/screen_util_unittest.cc:31:
Shell::GetPrimaryRootWindowController()->GetShelf()->SetAutoHideBehavior(
On 2016/06/01 00:53:23, msw wrote:
> q: is this even needed for the test?

Removed and updated bounds expectations.

https://codereview.chromium.org/2017413002/diff/40001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager_unittest.cc (right):

https://codereview.chromium.org/2017413002/diff/40001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager_unittest.cc:67: return
Shell::GetPrimaryRootWindowController()->shelf_widget();
On 2016/06/01 00:53:24, msw wrote:
> optional nit: return GetShelf()->shelf_widget();?

Done.

https://codereview.chromium.org/2017413002/diff/40001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager_unittest.cc:327: void
SetState(ShelfLayoutManager* layout_manager,
On 2016/06/01 00:53:24, msw wrote:
> nit: remove this and inline ShelfLayoutManager::SetState() calls?

ShelfLayoutManager::SetState() is private, so inlining would require several
FRIEND_TEST_ALL_PREFIXES. I added a comment that this exists to call the private
function.

https://codereview.chromium.org/2017413002/diff/40001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager_unittest.cc:1428: ShelfLayoutManager* shelf_1 =
On 2016/06/01 00:53:24, msw wrote:
> nit: layout_manager_1 (and layout_manager_2 below)?

Changed to Shelf*.

https://codereview.chromium.org/2017413002/diff/40001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager_unittest.cc:2240: Shelf* shelf = GetShelf();
On 2016/06/01 00:53:24, msw wrote:
> nit: it seems random when you choose to cache GetShelf or call it repeatedly.

Updated to cache GetShelf() everywhere, unless it was super-obvious there was a
single call (like one call at the top of the test).

https://codereview.chromium.org/2017413002/diff/40001/ash/shelf/shelf_view_un...
File ash/shelf/shelf_view_unittest.cc (right):

https://codereview.chromium.org/2017413002/diff/40001/ash/shelf/shelf_view_un...
ash/shelf/shelf_view_unittest.cc:21: #include "ash/shelf/shelf_layout_manager.h"
On 2016/06/01 00:53:24, msw wrote:
> nit: remove

Done.

https://codereview.chromium.org/2017413002/diff/40001/ash/system/tray/system_...
File ash/system/tray/system_tray_unittest.cc (right):

https://codereview.chromium.org/2017413002/diff/40001/ash/system/tray/system_...
ash/system/tray/system_tray_unittest.cc:11: #include
"ash/shelf/shelf_layout_manager.h"
On 2016/06/01 00:53:24, msw wrote:
> nit: include shelf.h instead?

Done.

https://codereview.chromium.org/2017413002/diff/40001/ash/system/user/tray_us...
File ash/system/user/tray_user_unittest.cc (right):

https://codereview.chromium.org/2017413002/diff/40001/ash/system/user/tray_us...
ash/system/user/tray_user_unittest.cc:57: ShelfLayoutManager*
shelf_layout_manager_;
On 2016/06/01 00:53:24, msw wrote:
> nit: remove member altogether; it's only used once.

Done.

https://codereview.chromium.org/2017413002/diff/40001/ash/wm/maximize_mode/ma...
File ash/wm/maximize_mode/maximize_mode_window_manager_unittest.cc (right):

https://codereview.chromium.org/2017413002/diff/40001/ash/wm/maximize_mode/ma...
ash/wm/maximize_mode/maximize_mode_window_manager_unittest.cc:940: Shelf* shelf
= Shelf::ForPrimaryDisplay();
On 2016/06/01 00:53:24, msw wrote:
> nit: why cache here but inline above? consistency is nice.

Changed to caching as noted in previous file.

https://codereview.chromium.org/2017413002/diff/40001/ash/wm/panels/panel_win...
File ash/wm/panels/panel_window_resizer_unittest.cc (right):

https://codereview.chromium.org/2017413002/diff/40001/ash/wm/panels/panel_win...
ash/wm/panels/panel_window_resizer_unittest.cc:273: ShelfLayoutManager*
layout_manager = shelf->shelf_layout_manager();
On 2016/06/01 00:53:24, msw wrote:
> nit: inline below?

Done.

https://codereview.chromium.org/2017413002/diff/40001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_window_resizer_unittest.cc (right):

https://codereview.chromium.org/2017413002/diff/40001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_window_resizer_unittest.cc:8: #include
"ash/root_window_controller.h"
On 2016/06/01 00:53:24, msw wrote:
> nit: remove

Done.

https://codereview.chromium.org/2017413002/diff/40001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_window_resizer_unittest.cc:10: #include
"ash/shelf/shelf_layout_manager.h"
On 2016/06/01 00:53:24, msw wrote:
> nit: remove

Done. Thanks for catching all these unused includes, btw.

https://codereview.chromium.org/2017413002/diff/40001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_window_resizer_unittest.cc:189: ShelfLayoutManager*
shelf_layout_manager() {
On 2016/06/01 00:53:24, msw wrote:
> nit: remove

Done.

https://codereview.chromium.org/2017413002/diff/40001/ash/wm/workspace_contro...
File ash/wm/workspace_controller_unittest.cc (right):

https://codereview.chromium.org/2017413002/diff/40001/ash/wm/workspace_contro...
ash/wm/workspace_controller_unittest.cc:676: Shelf* shelf =
Shelf::ForPrimaryDisplay();
On 2016/06/01 00:53:24, msw wrote:
> ditto nit: inline/cache consistency (in this file)

As above.

[msw, 2016-06-01 17:30:08]

lgtm with nits, thanks!

https://codereview.chromium.org/2017413002/diff/40001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager_unittest.cc (right):

https://codereview.chromium.org/2017413002/diff/40001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager_unittest.cc:327: void
SetState(ShelfLayoutManager* layout_manager,
On 2016/06/01 15:53:06, James Cook wrote:
> On 2016/06/01 00:53:24, msw wrote:
> > nit: remove this and inline ShelfLayoutManager::SetState() calls?
> 
> ShelfLayoutManager::SetState() is private, so inlining would require several
> FRIEND_TEST_ALL_PREFIXES. I added a comment that this exists to call the
private
> function.

Acknowledged.

https://codereview.chromium.org/2017413002/diff/80001/ash/shelf/shelf.h
File ash/shelf/shelf.h (right):

https://codereview.chromium.org/2017413002/diff/80001/ash/shelf/shelf.h#newco...
ash/shelf/shelf.h:81: // See enum description for details.
nit: remove comment, or add similar to GetAutoHideState.

https://codereview.chromium.org/2017413002/diff/80001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager_unittest.cc (right):

https://codereview.chromium.org/2017413002/diff/80001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager_unittest.cc:1870:
RunGestureDragTests(gfx::Vector2d(0, 120));
optional nit: set/expect bottom alignment before this.

https://codereview.chromium.org/2017413002/diff/80001/ash/wm/maximize_mode/ma...
File ash/wm/maximize_mode/maximize_mode_window_manager_unittest.cc (right):

https://codereview.chromium.org/2017413002/diff/80001/ash/wm/maximize_mode/ma...
ash/wm/maximize_mode/maximize_mode_window_manager_unittest.cc:11: #include
"ash/shelf/shelf_layout_manager.h"
nit: include shelf.h instead

[James Cook, 2016-06-01 17:56:03]

jamescook@chromium.org changed reviewers:
+ oshima@chromium.org

[James Cook, 2016-06-01 17:56:04]

oshima, can I get OWNERS?

https://codereview.chromium.org/2017413002/diff/80001/ash/shelf/shelf.h
File ash/shelf/shelf.h (right):

https://codereview.chromium.org/2017413002/diff/80001/ash/shelf/shelf.h#newco...
ash/shelf/shelf.h:81: // See enum description for details.
On 2016/06/01 17:30:08, msw wrote:
> nit: remove comment, or add similar to GetAutoHideState.

Done.

https://codereview.chromium.org/2017413002/diff/80001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager_unittest.cc (right):

https://codereview.chromium.org/2017413002/diff/80001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager_unittest.cc:1870:
RunGestureDragTests(gfx::Vector2d(0, 120));
On 2016/06/01 17:30:08, msw wrote:
> optional nit: set/expect bottom alignment before this.

Done.

https://codereview.chromium.org/2017413002/diff/80001/ash/wm/maximize_mode/ma...
File ash/wm/maximize_mode/maximize_mode_window_manager_unittest.cc (right):

https://codereview.chromium.org/2017413002/diff/80001/ash/wm/maximize_mode/ma...
ash/wm/maximize_mode/maximize_mode_window_manager_unittest.cc:11: #include
"ash/shelf/shelf_layout_manager.h"
On 2016/06/01 17:30:08, msw wrote:
> nit: include shelf.h instead

Done.

[oshima, 2016-06-01 18:10:06]

nice! lgtm

https://codereview.chromium.org/2017413002/diff/100001/ash/system/user/tray_u...
File ash/system/user/tray_user_unittest.cc (right):

https://codereview.chromium.org/2017413002/diff/100001/ash/system/user/tray_u...
ash/system/user/tray_user_unittest.cc:70: : tray_(nullptr), delegate_(nullptr),
tray_user_separator_(nullptr) {}
optiona: in-class initializer, and = defualt?

[James Cook, 2016-06-01 18:23:52]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2016-06-01 18:23:53]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/2017413002/#ps140001
(title: "review comments 3")

[commit-bot: I haz the power, 2016-06-01 18:24:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2017413002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2017413002/140001

[James Cook, 2016-06-01 18:24:21]

Thanks for the quick review.

https://codereview.chromium.org/2017413002/diff/100001/ash/system/user/tray_u...
File ash/system/user/tray_user_unittest.cc (right):

https://codereview.chromium.org/2017413002/diff/100001/ash/system/user/tray_u...
ash/system/user/tray_user_unittest.cc:70: : tray_(nullptr), delegate_(nullptr),
tray_user_separator_(nullptr) {}
On 2016/06/01 18:10:06, oshima wrote:
> optiona: in-class initializer, and = defualt?

Done.

[commit-bot: I haz the power, 2016-06-01 19:58:34]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-06-01 20:00:57]

Description was changed from

==========
ash: Fix variable names and setters in ShelfLayoutManager and tests

Calls to set shelf state should go through Shelf, not ShelfLayoutManager.
* Move all calls to SetAutoHideBehavior() and SetAlignment() to Shelf instead
of ShelfLayoutManager.
* Add Shelf::GetVisibilityState() and Shelf::GetAutoHideState() and prefer
using them to accessing ShelfLayoutManager directly.
* Remove ShelfLayoutManager::GetAutoHideBehavior().
* Rename variables mis-named "shelf" to shelf_widget, shelf_layout_manager, etc.
so they match their actual types.

No functional changes.

BUG=615502
TEST=ash_unittests
==========

to

==========
ash: Fix variable names and setters in ShelfLayoutManager and tests

Calls to set shelf state should go through Shelf, not ShelfLayoutManager.
* Move all calls to SetAutoHideBehavior() and SetAlignment() to Shelf instead
of ShelfLayoutManager.
* Add Shelf::GetVisibilityState() and Shelf::GetAutoHideState() and prefer
using them to accessing ShelfLayoutManager directly.
* Remove ShelfLayoutManager::GetAutoHideBehavior().
* Rename variables mis-named "shelf" to shelf_widget, shelf_layout_manager, etc.
so they match their actual types.

No functional changes.

BUG=615502
TEST=ash_unittests

Committed: https://crrev.com/cf8b648fcece89a78eec21726a1aa48667778dbe
Cr-Commit-Position: refs/heads/master@{#397214}
==========

[commit-bot: I haz the power, 2016-06-01 20:00:58]

Patchset 8 (id:??) landed as
https://crrev.com/cf8b648fcece89a78eec21726a1aa48667778dbe
Cr-Commit-Position: refs/heads/master@{#397214}