Refactor views app list services to allow more code sharing

chrome/browser/ui/app_list/app_list_shower_views.h

Index: chrome/browser/ui/app_list/app_list_shower_views.h
diff --git a/chrome/browser/ui/app_list/app_list_shower.h b/chrome/browser/ui/app_list/app_list_shower_views.h
similarity index 49%
rename from chrome/browser/ui/app_list/app_list_shower.h
rename to chrome/browser/ui/app_list/app_list_shower_views.h
index 9c7463c1ede5c01f192811a89bd93caa8a8d1191..3ea50562da3a90ce57bdbc81a81a1b4920be0b4d 100644
--- a/chrome/browser/ui/app_list/app_list_shower.h
+++ b/chrome/browser/ui/app_list/app_list_shower_views.h
@@ -2,65 +2,73 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef CHROME_BROWSER_UI_APP_LIST_APP_LIST_SHOWER_H_
-#define CHROME_BROWSER_UI_APP_LIST_APP_LIST_SHOWER_H_
+#ifndef CHROME_BROWSER_UI_APP_LIST_APP_LIST_SHOWER_VIEWS_H_
+#define CHROME_BROWSER_UI_APP_LIST_APP_LIST_SHOWER_VIEWS_H_
 
 #include "base/memory/scoped_ptr.h"
-#include "chrome/browser/ui/app_list/app_list.h"
-#include "chrome/browser/ui/app_list/app_list_factory.h"
 #include "ui/app_list/pagination_model.h"
 #include "ui/gfx/native_widget_types.h"
 
 namespace app_list {
-class AppListModel;
+class AppListView;
 }
 
+class AppListShowerDelegate;
 class AppListShowerUnitTest;
 class Profile;
 class ScopedKeepAlive;
 
-// Creates and shows an AppList as needed for non-Ash desktops. It is owned
-// by AppListService.
+// Creates and shows an AppList as needed for non-Ash desktops. It is owned by
+// AppListServiceViews.
 class AppListShower {

[Matt Giuca, 2014/04/29 03:52:51]

Shouldn't this be called AppListShowerViews?

(Actually, I'm not sure why you can't just call it, and the file name,
AppListShower if there isn't going to be a non-views one. Same for the other new
*Views things you're adding in this CL.)

[tapted, 2014/04/29 04:20:30]

app_list_shower.cc is currently compiled on Mac even though it is not used there
at all. Adding _views ensures it's only compiled when toolkit_views=1 (which
will soon "sometimes" be true on Mac).

As for whether the class name should correspond for this use case - there's not
much consistency around Chrome. I think it should be AppListShowerViews and only
left it as `AppListShower`to keep the diff more sane :).

So I'll rename towards the end of review, or in a follow-up CL.

(I had actually already renamed AppListShower to AppListCreator in earlier
iterations since `Shower` always seemed a bit awkward, but all that was
orthogonal and messed up the diff).

[Matt Giuca, 2014/04/30 00:41:23]

OK. I'd prefer not to use Creator because AFAIK the app list is not created /
destroyed, just hidden / shown. Is that correct?

[tapted, 2014/04/30 03:26:03]

It does create, but then it's owned by the native widget (so not destroyed
here). I backed away from 'Creator' after I decided not to move the show/hide
functions into AppListServiceViews (which could easily be done and would remove
another layer of delegation). Leaving show/hide on Shower makes the tests a bit
more compelling, but really there should be more tests on the Service as well.

Probably 'Controller' is the right name... but the ControllerDelegate already
exists, and is not this guy's delegate, so that would be confusing.

A nice follow-up might be to have AppListShower implement
AppListControllerDelegate and merge it with AppListControllerDelegateViews --
that's probably more consistent with typical design patterns, but it might make
testing more convoluted too.

  public:
-  AppListShower(scoped_ptr<AppListFactory> factory,
-                AppListService* service);
-  ~AppListShower();
-
-  void set_can_close(bool can_close) {
-    can_close_app_list_ = can_close;
-  }
+  explicit AppListShower(AppListShowerDelegate* delegate);
+  virtual ~AppListShower();
 
   void ShowForProfile(Profile* requested_profile);
   gfx::NativeWindow GetWindow();
 
-  AppList* app_list() { return app_list_.get(); }
+  app_list::AppListView* app_list() { return app_list_; }
   Profile* profile() const { return profile_; }
 
   // Create or recreate, and initialize |app_list_| from |requested_profile|.
   void CreateViewForProfile(Profile* requested_profile);
 
   void DismissAppList();
-  void HandleViewBeingDestroyed();
-  bool IsAppListVisible() const;
+  virtual void HandleViewBeingDestroyed();

[Matt Giuca, 2014/04/30 00:41:23]

Maybe:
// Virtual functions mocked out in tests.

[tapted, 2014/04/30 03:26:03]

Done.

+  virtual bool IsAppListVisible() const;
   void WarmupForProfile(Profile* profile);
-  bool HasView() const;
+  virtual bool HasView() const;
+
+ protected:
+  // Virtual functions mocked out in tests.
+  virtual app_list::AppListView* MakeViewForCurrentProfile();
+  virtual void UpdateViewForNewProfile();
+
+  // Shows the app list, activates it, and ensures the taskbar icon is updated.
+  virtual void Show();
+  virtual void Hide();
 
  private:
   friend class ::AppListShowerUnitTest;
 
   void ResetKeepAlive();
 
-  scoped_ptr<AppListFactory> factory_;
-  scoped_ptr<ScopedKeepAlive> keep_alive_;
-  scoped_ptr<AppList> app_list_;
-  AppListService* service_;  // Weak ptr, owns this.
+  AppListShowerDelegate* delegate_;  // Weak. Owns this.
+
+  // The profile currently shown by |app_list_|.
   Profile* profile_;
-  bool can_close_app_list_;
+
+  // The view, once created. Owned by native widget.
+  app_list::AppListView* app_list_;
 
   // Used to keep the browser process alive while the app list is visible.
+  scoped_ptr<ScopedKeepAlive> keep_alive_;

[Matt Giuca, 2014/04/30 00:41:23]

Confused about why this comment was there before but not the variable... :|

[tapted, 2014/04/30 03:26:03]

Yeah - it was like this when app_list_shower.h first came into existence in
r225655

+
+  bool window_icon_updated_;
+  app_list::PaginationModel pagination_model_;
 
   DISALLOW_COPY_AND_ASSIGN(AppListShower);
 };
 
-#endif  // CHROME_BROWSER_UI_APP_LIST_APP_LIST_SHOWER_H_
+#endif  // CHROME_BROWSER_UI_APP_LIST_APP_LIST_SHOWER_VIEWS_H_