Elim ActivateAppListItem, ChromeAppListItem

This should simplify the view/model structure some by having the View call ItemModel->Activate() directly, rather than going through the delegate (which continues to manage the model and handle non item specific events). This will allow us to create folder items as part of the non Chrome specific model implementation, without having to make AppListViewDelegate a concrete class, or adding special folder handling to the view classes. BUG=303839 For ash/shell: TBR=jamescook@chromium.org Review URL: https://codereview.chromium.org/25859005 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@227108 0039d316-1c4b-4281-b951-d872f2087c98
marcosholgado · Oct 4, 2013 · fa14cbc · fa14cbc
1 parent d46634e
commit fa14cbc
Show file tree

Hide file tree

Showing 19 changed files with 66 additions and 97 deletions.
diff --git a/ash/shell/app_list.cc b/ash/shell/app_list.cc
@@ -99,7 +99,7 @@ class WindowTypeLauncherItem : public app_list::AppListItemModel {
     }
   }
 
-  static void Activate(Type type, int event_flags) {
+  static void ActivateItem(Type type, int event_flags) {
      switch (type) {
       case TOPLEVEL_WINDOW: {
         ToplevelWindow::CreateParams params;
@@ -130,8 +130,9 @@ class WindowTypeLauncherItem : public app_list::AppListItemModel {
     }
   }
 
-  void Activate(int event_flags) {
-    Activate(type_, event_flags);
+  // AppListItemModel
+  virtual void Activate(int event_flags) OVERRIDE {
+    ActivateItem(type_, event_flags);
   }
 
  private:
@@ -240,16 +241,11 @@ class ExampleAppListViewDelegate : public app_list::AppListViewDelegate {
       const base::Callback<void(const base::FilePath&)>& callback) OVERRIDE {
   }
 
-  virtual void ActivateAppListItem(app_list::AppListItemModel* item,
-                                   int event_flags) OVERRIDE {
-    static_cast<WindowTypeLauncherItem*>(item)->Activate(event_flags);
-  }
-
   virtual void OpenSearchResult(app_list::SearchResult* result,
                                 int event_flags) OVERRIDE {
     const ExampleSearchResult* example_result =
         static_cast<const ExampleSearchResult*>(result);
-    WindowTypeLauncherItem::Activate(example_result->type(), event_flags);
+    WindowTypeLauncherItem::ActivateItem(example_result->type(), event_flags);
   }
 
   virtual void InvokeSearchResultAction(app_list::SearchResult* result,

diff --git a/chrome/browser/ui/app_list/app_list_view_delegate.cc b/chrome/browser/ui/app_list/app_list_view_delegate.cc
@@ -17,7 +17,6 @@
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/app_list/app_list_controller_delegate.h"
 #include "chrome/browser/ui/app_list/apps_model_builder.h"
-#include "chrome/browser/ui/app_list/chrome_app_list_item.h"
 #include "chrome/browser/ui/app_list/search/search_controller.h"
 #include "chrome/browser/ui/app_list/start_page_service.h"
 #include "chrome/browser/ui/browser_finder.h"
@@ -164,13 +163,6 @@ app_list::SigninDelegate* AppListViewDelegate::GetSigninDelegate() {
   return &signin_delegate_;
 }
 
-void AppListViewDelegate::ActivateAppListItem(
-    app_list::AppListItemModel* item,
-    int event_flags) {
-  content::RecordAction(content::UserMetricsAction("AppList_ClickOnApp"));
-  static_cast<ChromeAppListItem*>(item)->Activate(event_flags);
-}
-
 void AppListViewDelegate::GetShortcutPathForApp(
     const std::string& app_id,
     const base::Callback<void(const base::FilePath&)>& callback) {

diff --git a/chrome/browser/ui/app_list/app_list_view_delegate.h b/chrome/browser/ui/app_list/app_list_view_delegate.h
@@ -63,8 +63,6 @@ class AppListViewDelegate : public app_list::AppListViewDelegate,
   virtual void GetShortcutPathForApp(
       const std::string& app_id,
       const base::Callback<void(const base::FilePath&)>& callback) OVERRIDE;
-  virtual void ActivateAppListItem(app_list::AppListItemModel* item,
-                                   int event_flags) OVERRIDE;
   virtual void StartSearch() OVERRIDE;
   virtual void StopSearch() OVERRIDE;
   virtual void OpenSearchResult(app_list::SearchResult* result,

diff --git a/chrome/browser/ui/app_list/apps_model_builder.cc b/chrome/browser/ui/app_list/apps_model_builder.cc
@@ -308,11 +308,7 @@ void AppsModelBuilder::UpdateHighlight() {
 
 ExtensionAppItem* AppsModelBuilder::GetAppAt(size_t index) {
   DCHECK_LT(index, model_->item_count());
-  ChromeAppListItem* item =
-      static_cast<ChromeAppListItem*>(model_->GetItemAt(index));
-  DCHECK_EQ(item->type(), ChromeAppListItem::TYPE_APP);
-
-  return static_cast<ExtensionAppItem*>(item);
+  return static_cast<ExtensionAppItem*>(model_->GetItemAt(index));
 }
 
 void AppsModelBuilder::ListItemsAdded(size_t start, size_t count) {

diff --git a/chrome/browser/ui/app_list/chrome_app_list_item.h b/chrome/browser/ui/app_list/chrome_app_list_item.h
diff --git a/chrome/browser/ui/app_list/extension_app_item.cc b/chrome/browser/ui/app_list/extension_app_item.cc
@@ -19,6 +19,7 @@
 #include "chrome/common/extensions/extension_icon_set.h"
 #include "chrome/common/extensions/manifest_handlers/icons_handler.h"
 #include "chrome/common/extensions/manifest_url_handler.h"
+#include "content/public/browser/user_metrics.h"
 #include "grit/theme_resources.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/canvas.h"
@@ -71,8 +72,7 @@ ExtensionAppItem::ExtensionAppItem(Profile* profile,
                                    const std::string& extension_name,
                                    const gfx::ImageSkia& installing_icon,
                                    bool is_platform_app)
-    : ChromeAppListItem(TYPE_APP),
-      profile_(profile),
+    : profile_(profile),
       extension_id_(extension_id),
       controller_(controller),
       extension_name_(extension_name),
@@ -263,6 +263,7 @@ void ExtensionAppItem::Activate(int event_flags) {
   if (RunExtensionEnableFlow())
     return;
 
+  content::RecordAction(content::UserMetricsAction("AppList_ClickOnApp"));
   CoreAppLauncherHandler::RecordAppListMainLaunch(extension);
   controller_->ActivateApp(profile_,
                            extension,

diff --git a/chrome/browser/ui/app_list/extension_app_item.h b/chrome/browser/ui/app_list/extension_app_item.h
@@ -10,9 +10,9 @@
 #include "base/memory/scoped_ptr.h"
 #include "chrome/browser/extensions/extension_icon_image.h"
 #include "chrome/browser/ui/app_list/app_context_menu_delegate.h"
-#include "chrome/browser/ui/app_list/chrome_app_list_item.h"
 #include "chrome/browser/ui/extensions/extension_enable_flow_delegate.h"
 #include "sync/api/string_ordinal.h"
+#include "ui/app_list/app_list_item_model.h"
 #include "ui/gfx/image/image_skia.h"
 
 class AppListControllerDelegate;
@@ -29,7 +29,7 @@ class Extension;
 }
 
 // ExtensionAppItem represents an extension app in app list.
-class ExtensionAppItem : public ChromeAppListItem,
+class ExtensionAppItem : public app_list::AppListItemModel,
                          public extensions::IconImage::Observer,
                          public ExtensionEnableFlowDelegate,
                          public app_list::AppContextMenuDelegate {
@@ -86,7 +86,7 @@ class ExtensionAppItem : public ChromeAppListItem,
   virtual void ExtensionEnableFlowFinished() OVERRIDE;
   virtual void ExtensionEnableFlowAborted(bool user_initiated) OVERRIDE;
 
-  // Overridden from ChromeAppListItem:
+  // Overridden from AppListItemModel:
   virtual void Activate(int event_flags) OVERRIDE;
   virtual ui::MenuModel* GetContextMenuModel() OVERRIDE;
 

diff --git a/chrome/chrome_browser_ui.gypi b/chrome/chrome_browser_ui.gypi
@@ -141,7 +141,6 @@
         'browser/ui/app_list/app_list_view_delegate.h',
         'browser/ui/app_list/apps_model_builder.cc',
         'browser/ui/app_list/apps_model_builder.h',
-        'browser/ui/app_list/chrome_app_list_item.h',
         'browser/ui/app_list/chrome_signin_delegate.cc',
         'browser/ui/app_list/chrome_signin_delegate.h',
         'browser/ui/app_list/extension_app_item.cc',

diff --git a/ui/app_list/app_list_item_model.cc b/ui/app_list/app_list_item_model.cc
@@ -72,6 +72,9 @@ void AppListItemModel::RemoveObserver(AppListItemModelObserver* observer) {
   observers_.RemoveObserver(observer);
 }
 
+void AppListItemModel::Activate(int event_flags) {
+}
+
 ui::MenuModel* AppListItemModel::GetContextMenuModel() {
   return NULL;
 }

diff --git a/ui/app_list/app_list_item_model.h b/ui/app_list/app_list_item_model.h
@@ -51,6 +51,9 @@ class APP_LIST_EXPORT AppListItemModel {
   void AddObserver(AppListItemModelObserver* observer);
   void RemoveObserver(AppListItemModelObserver* observer);
 
+  // Activates (opens) the item. Does nothing by default.
+  virtual void Activate(int event_flags);
+
   // Returns the context menu model for this item, or NULL if there is currently
   // no menu for the item (e.g. during install).
   // Note the returned menu model is owned by this item.

diff --git a/ui/app_list/app_list_view_delegate.h b/ui/app_list/app_list_view_delegate.h
@@ -52,10 +52,6 @@ class APP_LIST_EXPORT AppListViewDelegate {
       const std::string& app_id,
       const base::Callback<void(const base::FilePath&)>& callback) = 0;
 
-  // Invoked when an AppListeItemModelView is activated by click or enter key.
-  virtual void ActivateAppListItem(AppListItemModel* item,
-                                   int event_flags) = 0;
-
   // Invoked to start a new search. Delegate collects query input from
   // SearchBoxModel and populates SearchResults. Both models are sub models
   // of AppListModel.

diff --git a/ui/app_list/cocoa/apps_grid_controller.h b/ui/app_list/cocoa/apps_grid_controller.h
@@ -72,8 +72,7 @@ APP_LIST_EXPORT
 
 - (size_t)visiblePage;
 
-// Calls delegate_->ActivateAppListItem for the currently selected item by
-// simulating a click.
+// Calls item->Activate for the currently selected item by simulating a click.
 - (void)activateSelection;
 
 // Return the number of pages of icons in the grid.

diff --git a/ui/app_list/cocoa/apps_grid_controller.mm b/ui/app_list/cocoa/apps_grid_controller.mm
@@ -5,6 +5,7 @@
 #import "ui/app_list/cocoa/apps_grid_controller.h"
 
 #include "base/mac/foundation_util.h"
+#include "ui/app_list/app_list_item_model.h"
 #include "ui/app_list/app_list_model.h"
 #include "ui/app_list/app_list_model_observer.h"
 #include "ui/app_list/app_list_view_delegate.h"
@@ -382,7 +383,7 @@ - (void)onItemClicked:(id)sender {
   for (size_t i = 0; i < [items_ count]; ++i) {
     AppsGridViewItem* item = [self itemAtIndex:i];
     if ([[item button] isEqual:sender])
-      delegate_->ActivateAppListItem([item model], 0);
+      [item model]->Activate(0);
   }
 }
 

diff --git a/ui/app_list/cocoa/apps_grid_controller_unittest.mm b/ui/app_list/cocoa/apps_grid_controller_unittest.mm
@@ -209,8 +209,9 @@ void SetMenuReadyForTesting(bool ready) {
   // Launch the item.
   SimulateClick(subview);
   SinkEvents();
-  EXPECT_EQ(1, delegate()->activate_count());
-  EXPECT_EQ(std::string("Item 0"), delegate()->last_activated()->title());
+  EXPECT_EQ(1, model()->activate_count());
+  ASSERT_TRUE(model()->last_activated());
+  EXPECT_EQ(std::string("Item 0"), model()->last_activated()->title());
 }
 
 // Test activating an item on the second page (the 17th item).
@@ -227,8 +228,9 @@ void SetMenuReadyForTesting(bool ready) {
   // Launch the item.
   SimulateClick(subview);
   SinkEvents();
-  EXPECT_EQ(1, delegate()->activate_count());
-  EXPECT_EQ(std::string("Item 16"), delegate()->last_activated()->title());
+  EXPECT_EQ(1, model()->activate_count());
+  ASSERT_TRUE(model()->last_activated());
+  EXPECT_EQ(std::string("Item 16"), model()->last_activated()->title());
 }
 
 // Test setModel.
@@ -315,8 +317,9 @@ void SetMenuReadyForTesting(bool ready) {
   SimulateKeyAction(@selector(moveRight:));
   EXPECT_EQ(2u, [apps_grid_controller_ selectedItemIndex]);
   SimulateKeyAction(@selector(insertNewline:));
-  EXPECT_EQ(1, delegate()->activate_count());
-  EXPECT_EQ(std::string("Item 2"), delegate()->last_activated()->title());
+  EXPECT_EQ(1, model()->activate_count());
+  ASSERT_TRUE(model()->last_activated());
+  EXPECT_EQ(std::string("Item 2"), model()->last_activated()->title());
 }
 
 // Tests keyboard navigation across pages.

diff --git a/ui/app_list/test/app_list_test_model.cc b/ui/app_list/test/app_list_test_model.cc
@@ -10,7 +10,25 @@
 namespace app_list {
 namespace test {
 
-AppListTestModel::AppListTestModel() {
+class AppListTestModel::AppListTestItemModel : public AppListItemModel {
+ public:
+  explicit AppListTestItemModel(AppListTestModel* model)
+      : model_(model) {
+  }
+  virtual ~AppListTestItemModel() {}
+
+  virtual void Activate(int event_flags) OVERRIDE {
+    model_->ItemActivated(this);
+  }
+
+ private:
+  AppListTestModel* model_;
+  DISALLOW_COPY_AND_ASSIGN(AppListTestItemModel);
+};
+
+AppListTestModel::AppListTestModel()
+    : activate_count_(0),
+      last_activated_(NULL) {
   SetSignedIn(true);
 }
 
@@ -54,5 +72,10 @@ void AppListTestModel::HighlightItemAt(int index) {
   item->SetHighlighted(true);
 }
 
+void AppListTestModel::ItemActivated(AppListTestItemModel* item) {
+  last_activated_ = item;
+  ++activate_count_;
+}
+
 }  // namespace test
 }  // namespace app_list
diff --git a/ui/app_list/test/app_list_test_model.h b/ui/app_list/test/app_list_test_model.h
@@ -42,7 +42,17 @@ class AppListTestModel : public AppListModel {
   // Call SetHighlighted on the specified item.
   void HighlightItemAt(int index);
 
+  int activate_count() { return activate_count_; }
+  AppListItemModel* last_activated() { return last_activated_; }
+
  private:
+  class AppListTestItemModel;
+
+  void ItemActivated(AppListTestItemModel* item);
+
+  int activate_count_;
+  AppListItemModel* last_activated_;
+
   DISALLOW_COPY_AND_ASSIGN(AppListTestModel);
 };
 

diff --git a/ui/app_list/test/app_list_test_view_delegate.cc b/ui/app_list/test/app_list_test_view_delegate.cc
@@ -12,9 +12,7 @@ namespace app_list {
 namespace test {
 
 AppListTestViewDelegate::AppListTestViewDelegate()
-    : activate_count_(0),
-      dismiss_count_(0),
-      last_activated_(NULL),
+    : dismiss_count_(0),
       test_signin_delegate_(NULL) {
 }
 
@@ -30,12 +28,6 @@ void AppListTestViewDelegate::GetShortcutPathForApp(
   callback.Run(base::FilePath());
 }
 
-void AppListTestViewDelegate::ActivateAppListItem(AppListItemModel* item,
-                                                  int event_flags) {
-  last_activated_ = item;
-  ++activate_count_;
-}
-
 void AppListTestViewDelegate::Dismiss() {
   ++dismiss_count_;
 }

diff --git a/ui/app_list/test/app_list_test_view_delegate.h b/ui/app_list/test/app_list_test_view_delegate.h
@@ -18,9 +18,7 @@ class AppListTestViewDelegate  : public AppListViewDelegate {
   AppListTestViewDelegate();
   virtual ~AppListTestViewDelegate();
 
-  int activate_count() { return activate_count_; }
   int dismiss_count() { return dismiss_count_; }
-  AppListItemModel* last_activated() { return last_activated_; }
   void set_test_signin_delegate(SigninDelegate* signin_delegate) {
     test_signin_delegate_ = signin_delegate;
   }
@@ -32,8 +30,6 @@ class AppListTestViewDelegate  : public AppListViewDelegate {
   virtual void GetShortcutPathForApp(
       const std::string& app_id,
       const base::Callback<void(const base::FilePath&)>& callback) OVERRIDE;
-  virtual void ActivateAppListItem(AppListItemModel* item,
-                                   int event_flags) OVERRIDE;
   virtual void StartSearch() OVERRIDE {}
   virtual void StopSearch() OVERRIDE {}
   virtual void OpenSearchResult(SearchResult* result,
@@ -52,9 +48,7 @@ class AppListTestViewDelegate  : public AppListViewDelegate {
   virtual content::WebContents* GetStartPageContents() OVERRIDE;
 
  private:
-  int activate_count_;
   int dismiss_count_;
-  AppListItemModel* last_activated_;
   SigninDelegate* test_signin_delegate_;  // Weak. Owned by test.
 };
 

diff --git a/ui/app_list/views/app_list_main_view.cc b/ui/app_list/views/app_list_main_view.cc
@@ -190,8 +190,7 @@ void AppListMainView::OnItemIconLoaded(IconLoader* loader) {
 }
 
 void AppListMainView::ActivateApp(AppListItemModel* item, int event_flags) {
-  if (delegate_)
-    delegate_->ActivateAppListItem(item, event_flags);
+  item->Activate(event_flags);
 }
 
 void AppListMainView::GetShortcutPathForApp(