[Web Intents] Refactor picker to use WebIntentPickerModel.

chrome/browser/ui/intents/web_intent_picker_controller.h

Index: chrome/browser/ui/intents/web_intent_picker_controller.h
diff --git a/chrome/browser/ui/intents/web_intent_picker_controller.h b/chrome/browser/ui/intents/web_intent_picker_controller.h
index 278cc3f434b85bb87ce4f5caca0978d4b4260ceb..9352c705a66c88cd62413e522bb08f45f23f279b 100644
--- a/chrome/browser/ui/intents/web_intent_picker_controller.h
+++ b/chrome/browser/ui/intents/web_intent_picker_controller.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.

[groby-ooo-7-16, 2012/01/25 22:03:13]

2011-2012

[binji, 2012/01/26 00:27:41]

I don't see any examples of this in chrome.

 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -18,16 +18,20 @@
 
 class Browser;
 class GURL;
-class SkBitmap;
+class TabContents;
 class TabContentsWrapper;
 class WebIntentPicker;
-class WebIntentPickerFactory;
+class WebIntentPickerModel;
 
 namespace content {
 class WebContents;
 class WebIntentsDispatcher;
 }
 
+namespace gfx {
+class Image;
+}
+
 namespace webkit_glue {
 struct WebIntentServiceData;
 }
@@ -38,8 +42,7 @@ class WebIntentPickerController : public content::NotificationObserver,
                                   public WebIntentPickerDelegate {
  public:
   // Takes ownership of |factory|.
-  WebIntentPickerController(TabContentsWrapper* wrapper,
-                            WebIntentPickerFactory* factory);
+  explicit WebIntentPickerController(TabContentsWrapper* wrapper);
   virtual ~WebIntentPickerController();
 
   // Sets the intent data and return pathway handler object for which
@@ -60,21 +63,31 @@ class WebIntentPickerController : public content::NotificationObserver,
                        const content::NotificationDetails& details) OVERRIDE;
 
   // WebIntentPickerDelegate implementation.
-  virtual void OnServiceChosen(size_t index) OVERRIDE;
+  virtual void OnServiceChosen(size_t index, Disposition disposition) OVERRIDE;
+  virtual void OnInlineDispositionWebContentsCreated(
+      content::WebContents* web_contents) OVERRIDE;
   virtual void OnCancelled() OVERRIDE;
   virtual void OnClosing() OVERRIDE;
 
  private:
-  // Gets a notification when the return message is sent to the source tab,
-  // so we can close the picker dialog or service tab.
-  void OnSendReturnMessage();
-
-  friend class WebIntentPickerControllerTest;
   friend class WebIntentPickerControllerBrowserTest;
   friend class InvokingTabObserver;
   class WebIntentDataFetcher;
   class FaviconFetcher;
 
+  // Gets a notification when the return message is sent to the source tab,
+  // so we can close the picker dialog or service tab.
+  void OnSendReturnMessage();
+
+  // Exposed for tests only.
+  void set_picker(WebIntentPicker* picker) { picker_ = picker; }

[groby-ooo-7-16, 2012/01/25 22:03:13]

Why add methods? Tests are friends of ours, so they can reach in directly to the
member vars.

[binji, 2012/01/26 00:27:41]

True, though I thought it was a little clearer here which member variables are
exposed to tests (even though technically they all are.)

+
+  // Exposed for tests only.
+  void set_model_observer(WebIntentPickerModelObserver* observer) {
+    picker_model_->set_observer(observer);
+  }
+
+  // Exposed for tests only.
   int pending_async_count() const { return pending_async_count_; }
 
   // Called from the WebIntentDataFetcher when intent data is available.
@@ -82,7 +95,7 @@ class WebIntentPickerController : public content::NotificationObserver,
       const std::vector<webkit_glue::WebIntentServiceData>& services);
 
   // Called from the FaviconDataFetcher when a favicon is available.
-  void OnFaviconDataAvailable(size_t index, const SkBitmap& icon_bitmap);
+  void OnFaviconDataAvailable(size_t index, const gfx::Image& icon);
 
   // Called from the FaviconDataFetcher when a favicon is not available.
   void OnFaviconDataUnavailable(size_t index);
@@ -97,9 +110,6 @@ class WebIntentPickerController : public content::NotificationObserver,
   // to close the picker ui.
   content::NotificationRegistrar registrar_;
 
-  // A factory to create a new picker.
-  scoped_ptr<WebIntentPickerFactory> picker_factory_;
-
   // A helper class to fetch web intent data asynchronously.
   scoped_ptr<WebIntentDataFetcher> web_intent_data_fetcher_;
 
@@ -109,15 +119,16 @@ class WebIntentPickerController : public content::NotificationObserver,
   // A weak pointer to the picker this controller controls.
   WebIntentPicker* picker_;
 
-  // A list of URLs to display in the UI.
-  std::vector<GURL> urls_;
-
-  // A list of the service data on display in the UI.
-  std::vector<webkit_glue::WebIntentServiceData> service_data_;
+  // The model for the picker. Owned by this controller. It should not be NULL
+  // while this controller exists, even if the picker is not shown.
+  scoped_ptr<WebIntentPickerModel> picker_model_;
 
   // A count of the outstanding asynchronous calls.
   int pending_async_count_;
 
+  // Is true if the picker is currently visible.
+  bool picker_shown_;

[groby-ooo-7-16, 2012/01/25 22:03:13]

I might be wrong, but as far as I can tell, this is equivalent to "picker_ !=
NULL". No need to track extra state

[binji, 2012/01/26 00:27:41]

I suppose it is a little tricky. The picker can be set externally (only for
tests), which means that it is not yet shown, but it is also not NULL. Maybe a
comment is necessary?

+
   // The routing object for the renderer which launched the intent.
   // Contains the intent data and a way to signal back to the client page.
   scoped_ptr<content::WebIntentsDispatcher> intents_dispatcher_;