Split Shell::ToggleAppList() into ShowAppList and DismissAppList.

Split Shell::ToggleAppList() into ShowAppList and DismissAppList.

This CL is in preparation for passing a reason for showing the app list.
This split is necessary to pass a reason only when showing and not when
dismissing.

BUG=None

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

[Matt Giuca, 2014-08-14 04:17:07]

https://codereview.chromium.org/460933002/diff/40001/ash/accelerators/acceler...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/460933002/diff/40001/ash/accelerators/acceler...
ash/accelerators/accelerator_controller.cc:488: if
(ash::Shell::GetInstance()->GetAppListTargetVisibility())
I think it would be good to preserve ToggleAppList as a wrapper around
ShowAppList and DismissAppList, so you don't need this four-line logic
duplicated in potentially multiple places (I count 2 in this CL, but more may
follow).

https://codereview.chromium.org/460933002/diff/40001/ash/shelf/app_list_shelf...
File ash/shelf/app_list_shelf_item_delegate.cc (right):

https://codereview.chromium.org/460933002/diff/40001/ash/shelf/app_list_shelf...
ash/shelf/app_list_shelf_item_delegate.cc:27: if
(Shell::GetInstance()->GetAppListTargetVisibility())
... here's the second one.

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

https://codereview.chromium.org/460933002/diff/40001/ash/shelf/shelf_layout_m...
ash/shelf/shelf_layout_manager_unittest.cc:1281: // Toggle app list to show, and
the shelf stays visible.
// Show app list, ...

https://codereview.chromium.org/460933002/diff/40001/ash/shelf/shelf_layout_m...
ash/shelf/shelf_layout_manager_unittest.cc:1286: // Toggle app list to hide, and
the shelf stays visible.
// Hide app list, ...

(and continue throughout this file)

https://codereview.chromium.org/460933002/diff/40001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/460933002/diff/40001/ash/shell.cc#newcode307
ash/shell.cc:307: app_list_controller_->SetVisible(false,
GetTargetRootWindow());
Are you sure this is equivalent? i.e., does AppListController::SetVisible()
ignore its second argument if its first argument is false?

If not, then you are changing the behaviour here when ToggleAppList was called
with a non-NULL window when the window is already visible.

Having said that, the only instance I can find of ToggleAppList with a non-NULL
window is the test I wrote, which always uses it to show the app list, so this
is probably fine. Just making sure you thought about it.

https://codereview.chromium.org/460933002/diff/40001/chrome/browser/ui/ash/ap...
File chrome/browser/ui/ash/app_list/app_list_service_ash.cc (right):

https://codereview.chromium.org/460933002/diff/40001/chrome/browser/ui/ash/ap...
chrome/browser/ui/ash/app_list/app_list_service_ash.cc:41: bool
AppListServiceAsh::IsAppListVisible() const {
You can delete this now.

[calamity, 2014-08-14 05:01:00]

https://codereview.chromium.org/460933002/diff/40001/ash/accelerators/acceler...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/460933002/diff/40001/ash/accelerators/acceler...
ash/accelerators/accelerator_controller.cc:488: if
(ash::Shell::GetInstance()->GetAppListTargetVisibility())
On 2014/08/14 04:17:06, Matt Giuca wrote:
> I think it would be good to preserve ToggleAppList as a wrapper around
> ShowAppList and DismissAppList, so you don't need this four-line logic
> duplicated in potentially multiple places (I count 2 in this CL, but more may
> follow).

Done.

https://codereview.chromium.org/460933002/diff/40001/ash/shelf/app_list_shelf...
File ash/shelf/app_list_shelf_item_delegate.cc (right):

https://codereview.chromium.org/460933002/diff/40001/ash/shelf/app_list_shelf...
ash/shelf/app_list_shelf_item_delegate.cc:27: if
(Shell::GetInstance()->GetAppListTargetVisibility())
On 2014/08/14 04:17:07, Matt Giuca wrote:
> ... here's the second one.

Done.

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

https://codereview.chromium.org/460933002/diff/40001/ash/shelf/shelf_layout_m...
ash/shelf/shelf_layout_manager_unittest.cc:1281: // Toggle app list to show, and
the shelf stays visible.
On 2014/08/14 04:17:07, Matt Giuca wrote:
> // Show app list, ...

Done.

https://codereview.chromium.org/460933002/diff/40001/ash/shelf/shelf_layout_m...
ash/shelf/shelf_layout_manager_unittest.cc:1286: // Toggle app list to hide, and
the shelf stays visible.
On 2014/08/14 04:17:07, Matt Giuca wrote:
> // Hide app list, ...
> 
> (and continue throughout this file)

Done.

https://codereview.chromium.org/460933002/diff/40001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/460933002/diff/40001/ash/shell.cc#newcode307
ash/shell.cc:307: app_list_controller_->SetVisible(false,
GetTargetRootWindow());
On 2014/08/14 04:17:07, Matt Giuca wrote:
> Are you sure this is equivalent? i.e., does AppListController::SetVisible()
> ignore its second argument if its first argument is false?
> 
> If not, then you are changing the behaviour here when ToggleAppList was called
> with a non-NULL window when the window is already visible.
> 
> Having said that, the only instance I can find of ToggleAppList with a
non-NULL
> window is the test I wrote, which always uses it to show the app list, so this
> is probably fine. Just making sure you thought about it.

I checked the method definition and it looks fine. I also plan to split
SetVisible into 2 methods next CL.

https://codereview.chromium.org/460933002/diff/40001/chrome/browser/ui/ash/ap...
File chrome/browser/ui/ash/app_list/app_list_service_ash.cc (right):

https://codereview.chromium.org/460933002/diff/40001/chrome/browser/ui/ash/ap...
chrome/browser/ui/ash/app_list/app_list_service_ash.cc:41: bool
AppListServiceAsh::IsAppListVisible() const {
On 2014/08/14 04:17:07, Matt Giuca wrote:
> You can delete this now.

It's an override.

[Matt Giuca, 2014-08-14 05:05:36]

lgtm

https://codereview.chromium.org/460933002/diff/40001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/460933002/diff/40001/ash/shell.cc#newcode307
ash/shell.cc:307: app_list_controller_->SetVisible(false,
GetTargetRootWindow());
On 2014/08/14 05:00:59, calamity wrote:
> On 2014/08/14 04:17:07, Matt Giuca wrote:
> > Are you sure this is equivalent? i.e., does AppListController::SetVisible()
> > ignore its second argument if its first argument is false?
> > 
> > If not, then you are changing the behaviour here when ToggleAppList was
called
> > with a non-NULL window when the window is already visible.
> > 
> > Having said that, the only instance I can find of ToggleAppList with a
> non-NULL
> > window is the test I wrote, which always uses it to show the app list, so
this
> > is probably fine. Just making sure you thought about it.
> 
> I checked the method definition and it looks fine. I also plan to split
> SetVisible into 2 methods next CL.

Acknowledged.

https://codereview.chromium.org/460933002/diff/40001/chrome/browser/ui/ash/ap...
File chrome/browser/ui/ash/app_list/app_list_service_ash.cc (right):

https://codereview.chromium.org/460933002/diff/40001/chrome/browser/ui/ash/ap...
chrome/browser/ui/ash/app_list/app_list_service_ash.cc:41: bool
AppListServiceAsh::IsAppListVisible() const {
On 2014/08/14 05:00:59, calamity wrote:
> On 2014/08/14 04:17:07, Matt Giuca wrote:
> > You can delete this now.
> 
> It's an override.

Acknowledged.

[James Cook, 2014-08-14 20:49:36]

LGTM but please consider my optional suggestion

https://codereview.chromium.org/460933002/diff/100001/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/460933002/diff/100001/ash/shell.h#newcode237
ash/shell.h:237: void ShowAppList(aura::Window* anchor);
Another option would be to provide "SetAppListVisible(bool visible,
aura::Window* anchor, Reason reason)" instead of these three methods.

The Shell API surface is already huge and that would keep things a bit smaller.

[Matt Giuca, 2014-08-15 01:00:21]

slgtm

https://codereview.chromium.org/460933002/diff/100001/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/460933002/diff/100001/ash/shell.h#newcode237
ash/shell.h:237: void ShowAppList(aura::Window* anchor);
I have a slight preference to keep things the way they are (I think the current
3 methods provide a nice simple interface for a few dozen call sites; having
just 1 method makes all the call sites slightly more complex).

But I wouldn't mind if you change this.

[calamity, 2014-08-15 03:06:33]

I think this API is more consistent with the rest of the app list code which has
a Show/Dismiss.

Hope you don't me passing on this.

[calamity, 2014-08-15 03:06:37]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-08-15 03:08:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/calamity@chromium.org/460933002/100001

[commit-bot: I haz the power, 2014-08-15 16:03:15]

Committed patchset #3 (100001) as 289889

[James Cook, 2014-08-15 16:17:42]

On 2014/08/15 16:03:15, I haz the power (commit-bot) wrote:
> Committed patchset #3 (100001) as 289889

No worries, I do mean optional when I say optional. :-)  LGTM