Split AppListController::SetVisible into Show and Dismiss.

ash/wm/app_list_controller.cc

Index: ash/wm/app_list_controller.cc
diff --git a/ash/wm/app_list_controller.cc b/ash/wm/app_list_controller.cc
index 99a0a653f5cef246b6f0e6152b4c25c1e57be1a2..25c854b752cacbdd12580c5a66313fdf94d37f4e 100644
--- a/ash/wm/app_list_controller.cc
+++ b/ash/wm/app_list_controller.cc
@@ -173,27 +173,18 @@ AppListController::~AppListController() {
   Shell::GetInstance()->RemoveShellObserver(this);
 }
 
-void AppListController::SetVisible(bool visible, aura::Window* window) {
-  if (visible == is_visible_)
+void AppListController::Show(aura::Window* window) {
+  if (is_visible_)
     return;
 
-  is_visible_ = visible;
+  is_visible_ = true;
 
   // App list needs to know the new shelf layout in order to calculate its
   // UI layout when AppListView visibility changes.
   Shell::GetPrimaryRootWindowController()->GetShelfLayoutManager()->
       UpdateAutoHideState();
 
-  if (view_) {
-    // Our widget is currently active. When the animation completes we'll hide
-    // the widget, changing activation. If a menu is shown before the animation
-    // completes then the activation change triggers the menu to close. By
-    // deactivating now we ensure there is no activation change when the
-    // animation completes and any menus stay open.
-    if (!visible)
-      view_->GetWidget()->Deactivate();
-    ScheduleAnimation();
-  } else if (is_visible_) {
+  if (!view_) {
     // AppListModel and AppListViewDelegate are owned by AppListView. They
     // will be released with AppListView on close.
     app_list::AppListView* view = new app_list::AppListView(
@@ -239,11 +230,43 @@ void AppListController::SetVisible(bool visible, aura::Window* window) {
     // handle items.
     SetDragAndDropHostOfCurrentAppList(
         Shelf::ForWindow(window)->GetDragAndDropHostForAppList());
+  } else {
+    ScheduleAnimation();

[Matt Giuca, 2014/08/19 08:26:29]

Is there a reason to invert the if statement? Why not just leave this where it
was (but delete the comment and the "if (!visible)" bit).

[calamity, 2014/08/20 00:44:53]

Done.

   }
+
   // Update applist button status when app list visibility is changed.
   Shelf::ForWindow(window)->GetAppListButtonView()->SchedulePaint();
 }
 
+void AppListController::Dismiss() {
+  if (!is_visible_)
+    return;
+
+  is_visible_ = false;
+
+  // App list needs to know the new shelf layout in order to calculate its
+  // UI layout when AppListView visibility changes.
+  Shell::GetPrimaryRootWindowController()
+      ->GetShelfLayoutManager()
+      ->UpdateAutoHideState();
+
+  if (!view_)

[Matt Giuca, 2014/08/19 08:26:29]

This slightly changes the logic in that you won't call SchedulePaint if !view_,
whereas previously you would have.

Is that important?

[calamity, 2014/08/20 00:44:53]

I don't think this will be a problem. !view_ should never happen except while
the widget is being destroyed. To be absolutely safe, we can add a
SchedulePaint() to ResetView().

[Matt Giuca, 2014/08/20 06:13:12]

Note: We discussed this and decided to replace this if statement with a
DCHECK(view_).

It *should* always be true; if it isn't true, returning from here would not be
correct.

+    return;
+
+  // Our widget is currently active. When the animation completes we'll hide
+  // the widget, changing activation. If a menu is shown before the animation
+  // completes then the activation change triggers the menu to close. By
+  // deactivating now we ensure there is no activation change when the
+  // animation completes and any menus stay open.
+  view_->GetWidget()->Deactivate();
+  ScheduleAnimation();
+
+  // Update applist button status when app list visibility is changed.
+  Shelf::ForWindow(view_->GetWidget()->GetNativeView())
+      ->GetAppListButtonView()
+      ->SchedulePaint();
+}
+
 bool AppListController::IsVisible() const {
   return view_ && view_->GetWidget()->IsVisible();
 }
@@ -354,7 +377,7 @@ void AppListController::ProcessLocatedEvent(ui::LocatedEvent* event) {
 
   aura::Window* window = view_->GetWidget()->GetNativeView()->parent();
   if (!window->Contains(target))
-    SetVisible(false, window);
+    Dismiss();
 }
 
 void AppListController::UpdateBounds() {
@@ -392,7 +415,7 @@ void AppListController::OnWindowFocused(aura::Window* gained_focus,
 
     if (applist_container->Contains(lost_focus) &&
         (!gained_focus || !applist_container->Contains(gained_focus))) {
-      SetVisible(false, applist_window);
+      Dismiss();
     }
   }
 }
@@ -421,7 +444,7 @@ void AppListController::OnImplicitAnimationsCompleted() {
 void AppListController::OnWidgetDestroying(views::Widget* widget) {
   DCHECK(view_->GetWidget() == widget);
   if (is_visible_)
-    SetVisible(false, widget->GetNativeView());
+    Dismiss();
   ResetView();
 }