Refactor LauncherDelegate

ash/launcher/launcher_view.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ash/launcher/launcher_view.h"
 
 #include <algorithm>
 
 #include "ash/ash_constants.h"
 #include "ash/ash_switches.h"
 #include "ash/drag_drop/drag_image_view.h"
 #include "ash/launcher/alternate_app_list_button.h"
 #include "ash/launcher/app_list_button.h"
 #include "ash/launcher/launcher_button.h"
 #include "ash/launcher/launcher_delegate.h"
 #include "ash/launcher/launcher_icon_observer.h"
+#include "ash/launcher/launcher_item_delegate.h"
 #include "ash/launcher/launcher_model.h"
 #include "ash/launcher/launcher_tooltip_manager.h"
 #include "ash/launcher/overflow_bubble.h"
 #include "ash/launcher/overflow_button.h"
 #include "ash/launcher/tabbed_launcher_button.h"
 #include "ash/root_window_controller.h"
 #include "ash/scoped_target_root_window.h"
 #include "ash/shelf/shelf_layout_manager.h"
 #include "ash/shelf/shelf_widget.h"
 #include "ash/shell_delegate.h"
@@ skipping 933 matching lines @@
 }
 
 void LauncherView::PrepareForDrag(Pointer pointer,
                                   const ui::LocatedEvent& event) {
   DCHECK(!dragging());
   DCHECK(drag_view_);
   drag_pointer_ = pointer;
   start_drag_index_ = view_model_->GetIndexOfView(drag_view_);
 
   // If the item is no longer draggable, bail out.
+  LauncherItemDelegate* item_delegate = delegate_->GetLauncherItemDelegate(
+      model_->items()[start_drag_index_]);

[Mr4D (OOO till 08-26), 2013/08/13 18:18:24]

Should you do this assignment if start_drag_index_ == -1?

[simonhong_, 2013/08/13 19:54:56]

Done.

   if (start_drag_index_ == -1 ||
-      !delegate_->IsDraggable(model_->items()[start_drag_index_])) {
+      !item_delegate ||
+      !item_delegate->IsDraggable(model_->items()[start_drag_index_])) {
     CancelDrag(-1);
     return;
   }
 
   // Move the view to the front so that it appears on top of other views.
   ReorderChildView(drag_view_, -1);
   bounds_animator_->StopAnimatingView(drag_view_);
 }
 
 void LauncherView::ContinueDrag(const ui::LocatedEvent& event) {
   ShelfLayoutManager* shelf = tooltip_->shelf_layout_manager();
 
   // TODO: I don't think this works correctly with RTL.
   gfx::Point drag_point(event.location());
   views::View::ConvertPointToTarget(drag_view_, this, &drag_point);
   int current_index = view_model_->GetIndexOfView(drag_view_);
   DCHECK_NE(-1, current_index);
 
   // If the item is no longer draggable, bail out.
+  LauncherItemDelegate* item_delegate = delegate_->GetLauncherItemDelegate(
+      model_->items()[current_index]);

[Mr4D (OOO till 08-26), 2013/08/13 18:18:24]

Should you do this assignment if start_drag_index_ == -1?

[simonhong_, 2013/08/13 19:54:56]

I think you mean current_index not start_drag_index_.
"current_index == -1" can be removed because it is already checked by
above DCHECK_NE(-1, current_index).

   if (current_index == -1 ||
-      !delegate_->IsDraggable(model_->items()[current_index])) {
+      !item_delegate ||
+      !item_delegate->IsDraggable(model_->items()[current_index])) {
     CancelDrag(-1);
     return;
   }
 
   // Constrain the location to the range of valid indices for the type.
   std::pair<int, int> indices(GetDragRange(current_index));
   int first_drag_index = indices.first;
   int last_drag_index = indices.second;
   // If the last index isn't valid, we're overflowing. Constrain to the app list
   // (which is the last visible item).
@@ skipping 371 matching lines @@
 }
 
 void LauncherView::PointerPressedOnButton(views::View* view,
                                           Pointer pointer,
                                           const ui::LocatedEvent& event) {
   if (drag_view_)
     return;
 
   tooltip_->Close();
   int index = view_model_->GetIndexOfView(view);
+  LauncherItemDelegate* item_delegate =
+      delegate_->GetLauncherItemDelegate(model_->items()[index]);

[Mr4D (OOO till 08-26), 2013/08/13 18:18:24]

Should you do this assignment if start_drag_index_ == -1?

[simonhong_, 2013/08/13 19:54:56]

Done.

   if (index == -1 ||
       view_model_->view_size() <= 1 ||
-      !delegate_->IsDraggable(model_->items()[index]))
+      !item_delegate ||
+      !item_delegate->IsDraggable(model_->items()[index]))
     return;  // View is being deleted or not draggable, ignore request.
 
   ShelfLayoutManager* shelf = tooltip_->shelf_layout_manager();
 
   drag_view_ = view;
   drag_offset_ = shelf->PrimaryAxisValue(event.x(), event.y());
 }
 
 void LauncherView::PointerDraggedOnButton(views::View* view,
                                           Pointer pointer,
@@ skipping 54 matching lines @@
   if (view_index == -1)
     return base::string16();
 
   switch (model_->items()[view_index].type) {
     case TYPE_TABBED:
     case TYPE_APP_PANEL:
     case TYPE_APP_SHORTCUT:
     case TYPE_WINDOWED_APP:
     case TYPE_PLATFORM_APP:
     case TYPE_BROWSER_SHORTCUT:
-      return delegate_->GetTitle(model_->items()[view_index]);
-
-    case TYPE_APP_LIST:
-      return model_->status() == LauncherModel::STATUS_LOADING ?
-          l10n_util::GetStringUTF16(IDS_AURA_APP_LIST_SYNCING_TITLE) :
-          l10n_util::GetStringUTF16(IDS_AURA_APP_LIST_TITLE);
+    case TYPE_APP_LIST: {
+      LauncherItemDelegate* item_delegate =
+          delegate_->GetLauncherItemDelegate(model_->items()[view_index]);
+      DCHECK(item_delegate);
+      return item_delegate->GetTitle(model_->items()[view_index]);
+    }
+    default:

[Mr4D (OOO till 08-26), 2013/08/13 18:18:24]

Please do not use default here. Reason: If we are using a "switch" statement we
want to be informed by the compiler when a case was not covered.

That said: Here was never a default which means that in all cases you do the
same code. As such I think that you can get rid of the switch statement
alltogether?

[simonhong_, 2013/08/13 19:54:56]

Removed switch statement.

+      break;
   }
   return base::string16();
 }
 
 void LauncherView::ButtonPressed(views::Button* sender,
                                  const ui::Event& event) {
   // Do not handle mouse release during drag.
   if (dragging())
     return;
 
@@ skipping 25 matching lines @@
     }
 
     // Collect usage statistics before we decide what to do with the click.
     switch (model_->items()[view_index].type) {
       case TYPE_APP_SHORTCUT:
       case TYPE_WINDOWED_APP:
       case TYPE_PLATFORM_APP:
       case TYPE_BROWSER_SHORTCUT:
         Shell::GetInstance()->delegate()->RecordUserMetricsAction(
             UMA_LAUNCHER_CLICK_ON_APP);
         // Fallthrough

[Mr4D (OOO till 08-26), 2013/08/13 18:18:24]

I don't think so! Using this fall through you record incorrectly the UMA about
the applist button.

[simonhong_, 2013/08/13 19:54:56]

Done.

-      case TYPE_TABBED:
-      case TYPE_APP_PANEL:
-        delegate_->ItemSelected(model_->items()[view_index], event);
-        break;
-
       case TYPE_APP_LIST:
         Shell::GetInstance()->delegate()->RecordUserMetricsAction(
             UMA_LAUNCHER_CLICK_ON_APPLIST_BUTTON);
-        Shell::GetInstance()->ToggleAppList(GetWidget()->GetNativeView());
+        // Fallthrough
+      case TYPE_TABBED:
+      case TYPE_APP_PANEL: {
+        LauncherItemDelegate* item_delegate =
+            delegate_->GetLauncherItemDelegate(model_->items()[view_index]);
+        DCHECK(item_delegate);
+        item_delegate->ItemSelected(model_->items()[view_index], event);
         break;
+      }
     }
   }
 
-  if (model_->items()[view_index].type != TYPE_APP_LIST)
-    ShowListMenuForView(model_->items()[view_index], sender, event);
+  ShowListMenuForView(model_->items()[view_index], sender, event);
 }
 
 bool LauncherView::ShowListMenuForView(const LauncherItem& item,
                                        views::View* source,
                                        const ui::Event& event) {
   scoped_ptr<ash::LauncherMenuModel> menu_model;
-  menu_model.reset(delegate_->CreateApplicationMenu(item, event.flags()));
+  LauncherItemDelegate* item_delegate =
+      delegate_->GetLauncherItemDelegate(item);
+  if (item_delegate)
+    menu_model.reset(item_delegate->CreateApplicationMenu(item, event.flags()));

[Mr4D (OOO till 08-26), 2013/08/13 18:18:24]

And what else? It can be that another menu is still shown and the
menu_model.get() will result in != NULL. You have to menu_model.reset() here to
enforce any opened menus to close in that case.

[simonhong_, 2013/08/13 19:54:56]

Done.

 
   // Make sure we have a menu and it has at least two items in addition to the
   // application title and the 3 spacing separators.
   if (!menu_model.get() || menu_model->GetItemCount() <= 5)
     return false;
 
   ShowMenu(scoped_ptr<views::MenuModelAdapter>(
                new LauncherMenuModelAdapter(menu_model.get())),
            source,
            gfx::Point(),
            false,
            ui::GetMenuSourceTypeForEvent(event));
   return true;
 }
 
 void LauncherView::ShowContextMenuForView(views::View* source,
                                           const gfx::Point& point,
                                           ui:: MenuSourceType source_type) {
   int view_index = view_model_->GetIndexOfView(source);
+  // TODO: Create LauncherContextMenu for applist in its LauncherItemDelegate.
   if (view_index != -1 &&
       model_->items()[view_index].type == TYPE_APP_LIST) {
     view_index = -1;
   }
 
   tooltip_->Close();
 
   if (view_index == -1) {
     Shell::GetInstance()->ShowContextMenu(point, source_type);
     return;
   }
-  scoped_ptr<ui::MenuModel> menu_model(delegate_->CreateContextMenu(
-      model_->items()[view_index],
-      source->GetWidget()->GetNativeView()->GetRootWindow()));
+  scoped_ptr<ui::MenuModel> menu_model;
+  LauncherItemDelegate* item_delegate =
+      delegate_->GetLauncherItemDelegate(model_->items()[view_index]);
+  if (item_delegate) {
+    menu_model.reset(item_delegate->CreateContextMenu(
+        model_->items()[view_index],
+        source->GetWidget()->GetNativeView()->GetRootWindow()));
+  }
   if (!menu_model)
     return;
   base::AutoReset<LauncherID> reseter(
       &context_menu_id_,
       view_index == -1 ? 0 : model_->items()[view_index].id);
 
   ShowMenu(scoped_ptr<views::MenuModelAdapter>(
                new views::MenuModelAdapter(menu_model.get())),
            source,
            point,
@@ skipping 112 matching lines @@
   int view_index = view_model_->GetIndexOfView(view);
   if (view_index == -1)
     return NULL;
   return &(model_->items()[view_index]);
 }
 
 bool LauncherView::ShouldShowTooltipForView(const views::View* view) const {
   if (view == GetAppListButtonView() &&
       Shell::GetInstance()->GetAppListWindow())
     return false;
   const LauncherItem* item = LauncherItemForView(view);

[Mr4D (OOO till 08-26), 2013/08/13 18:18:24]

Shouldn't you check here if (!item) return true?

[simonhong_, 2013/08/13 19:54:56]

Done.
Q) Is "true" default value for showing tooltip?

[Mr4D (OOO till 08-26), 2013/08/14 15:52:19]

Look at the old code:

return (!item || ...);

[simonhong_, 2013/08/14 23:47:51]

Done.

-  return (!item || delegate_->ShouldShowTooltip(*item));
+  LauncherItemDelegate* item_delegate =
+      delegate_->GetLauncherItemDelegate(*item);
+  if (!item_delegate)
+    return false;
+  return item_delegate->ShouldShowTooltip(*item);
 }
 
 }  // namespace internal
 }  // namespace ash