[Web Intents] Refactor picker to use WebIntentPickerModel.

chrome/browser/ui/intents/web_intent_picker_controller.cc

Index: chrome/browser/ui/intents/web_intent_picker_controller.cc
diff --git a/chrome/browser/ui/intents/web_intent_picker_controller.cc b/chrome/browser/ui/intents/web_intent_picker_controller.cc
index bd0cbc83596a8aae35b0e492ea6719049c334241..26841525b8b12ad681acb3780bae6b357c8ba628 100644
--- a/chrome/browser/ui/intents/web_intent_picker_controller.cc
+++ b/chrome/browser/ui/intents/web_intent_picker_controller.cc
@@ -16,7 +16,7 @@
 #include "chrome/browser/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/browser_navigator.h"
 #include "chrome/browser/ui/intents/web_intent_picker.h"
-#include "chrome/browser/ui/intents/web_intent_picker_factory.h"
+#include "chrome/browser/ui/intents/web_intent_picker_model.h"
 #include "chrome/browser/ui/tab_contents/tab_contents_wrapper.h"
 #include "chrome/browser/webdata/web_data_service.h"
 #include "chrome/common/chrome_notification_types.h"
@@ -25,11 +25,9 @@
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_intents_dispatcher.h"
 #include "ui/gfx/codec/png_codec.h"
+#include "ui/gfx/image/image.h"
 #include "webkit/glue/web_intent_service_data.h"
 
-using content::NavigationController;
-using content::WebContents;
-
 namespace {
 
 // Gets the favicon service for the profile in |tab_contents|.
@@ -42,6 +40,19 @@ WebIntentsRegistry* GetWebIntentsRegistry(TabContentsWrapper* wrapper) {
   return WebIntentsRegistryFactory::GetForProfile(wrapper->profile());
 }
 
+WebIntentPickerModel::Disposition ConvertDisposition(
+    webkit_glue::WebIntentServiceData::Disposition disposition) {
+  switch (disposition) {
+    case webkit_glue::WebIntentServiceData::DISPOSITION_INLINE:
+      return WebIntentPickerModel::DISPOSITION_INLINE;
+    case webkit_glue::WebIntentServiceData::DISPOSITION_WINDOW:
+      return WebIntentPickerModel::DISPOSITION_WINDOW;
+    default:
+      NOTREACHED();
+      return WebIntentPickerModel::DISPOSITION_WINDOW;
+  }
+}
+
 }  // namespace
 
 // A class that asynchronously fetches web intent data from the web intents
@@ -108,23 +119,24 @@ class WebIntentPickerController::FaviconFetcher {
 };
 
 WebIntentPickerController::WebIntentPickerController(
-    TabContentsWrapper* wrapper,
-    WebIntentPickerFactory* factory)
+    TabContentsWrapper* wrapper)
         : wrapper_(wrapper),

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

nit: Shouldn't this only be indented 4 spaces?

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

Looks like you're right. The line-wrap rules are the hardest for me to
extrapolate in cases like this.

-          picker_factory_(factory),
           web_intent_data_fetcher_(
               new WebIntentDataFetcher(this,
                                        GetWebIntentsRegistry(wrapper))),
           favicon_fetcher_(
               new FaviconFetcher(this, GetFaviconService(wrapper))),
           picker_(NULL),
+          picker_model_(new WebIntentPickerModel()),
           pending_async_count_(0),
+          picker_shown_(false),
           service_tab_(NULL) {
-  NavigationController* controller = &wrapper->web_contents()->GetController();
+  content::NavigationController* controller =
+      &wrapper->web_contents()->GetController();
   registrar_.Add(this, content::NOTIFICATION_LOAD_START,
-                 content::Source<NavigationController>(controller));
+                 content::Source<content::NavigationController>(controller));
   registrar_.Add(this, content::NOTIFICATION_TAB_CLOSING,
-                 content::Source<NavigationController>(controller));
+                 content::Source<content::NavigationController>(controller));
 }
 
 WebIntentPickerController::~WebIntentPickerController() {
@@ -141,16 +153,19 @@ void WebIntentPickerController::SetIntentsDispatcher(
 void WebIntentPickerController::ShowDialog(Browser* browser,
                                            const string16& action,
                                            const string16& type) {
-  if (picker_ != NULL)
+  // Only show a picker once.
+  if (picker_shown_)
     return;
 
-  picker_ = picker_factory_->Create(browser, wrapper_, this);
+  picker_model_->Clear();
 
-  // TODO(binji) Remove this check when there are implementations of the picker
-  // for windows and mac.
-  if (picker_ == NULL)
-    return;
+  // If picker is non-NULL, it was set by a test.
+  if (picker_ == NULL) {
+    picker_ = WebIntentPicker::Create(browser, wrapper_, this,
+                                      picker_model_.get());
+  }
 
+  picker_shown_ = true;
   web_intent_data_fetcher_->Fetch(action, type);
 }
 
@@ -163,34 +178,57 @@ void WebIntentPickerController::Observe(
   ClosePicker();
 }
 
-void WebIntentPickerController::OnServiceChosen(size_t index) {
-  DCHECK(index < urls_.size());
-
-  bool inline_disposition = service_data_[index].disposition ==
-      webkit_glue::WebIntentServiceData::DISPOSITION_INLINE;
-  WebContents* new_web_contents = NULL;
-  if (inline_disposition)
-    new_web_contents = picker_->SetInlineDisposition(urls_[index]);
-
-  if (new_web_contents == NULL) {
-    // TODO(gbillock): This really only handles the 'window' disposition in a
-    // quite prototype way. We need to flesh out what happens to the picker
-    // during the lifetime of the service url context, and that may mean we
-    // need to pass more information into the injector to find the picker again
-    // and close it. Also: the above conditional construction is just because
-    // there isn't Mac/Win support yet. When that's there, it'll be an else.
-    browser::NavigateParams params(NULL, urls_[index],
-                                   content::PAGE_TRANSITION_AUTO_BOOKMARK);
-    params.disposition = NEW_FOREGROUND_TAB;
-    params.profile = wrapper_->profile();
-    browser::Navigate(&params);
-    new_web_contents = params.target_contents->web_contents();
-    service_tab_ = new_web_contents;
-
-    ClosePicker();
+void WebIntentPickerController::OnServiceChosen(size_t index,
+                                                Disposition disposition) {
+  switch (disposition) {
+    case WebIntentPickerModel::DISPOSITION_INLINE:
+      // Set the model to inline disposition. It will notify the picker which
+      // will respond (via OnInlineDispositionWebContentsCreated) with the
+      // WebContents to dispatch the intent to.
+      picker_model_->SetInlineDisposition(index);
+      break;
+
+    case WebIntentPickerModel::DISPOSITION_WINDOW: {
+      // TODO(gbillock): This really only handles the 'window' disposition in a
+      // quite prototype way. We need to flesh out what happens to the picker
+      // during the lifetime of the service url context, and that may mean we
+      // need to pass more information into the injector to find the picker
+      // again and close it.
+      const WebIntentPickerModel::Item& item = picker_model_->GetItemAt(index);
+
+      browser::NavigateParams params(NULL, item.url,
+                                     content::PAGE_TRANSITION_AUTO_BOOKMARK);
+      params.disposition = NEW_FOREGROUND_TAB;
+      params.profile = wrapper_->profile();
+      browser::Navigate(&params);
+      content::WebContents* web_contents =
+          params.target_contents->web_contents();
+      service_tab_ = web_contents;
+
+      ClosePicker();
+
+      intents_dispatcher_->DispatchIntent(web_contents);
+      break;
+    }
+
+    default:
+      NOTREACHED();
+      break;
   }
+}
 
-  intents_dispatcher_->DispatchIntent(new_web_contents);
+void WebIntentPickerController::OnInlineDispositionWebContentsCreated(
+    content::WebContents* web_contents) {
+  if (web_contents) {
+    intents_dispatcher_->DispatchIntent(web_contents);
+  } else {
+    // TODO(binji): tab_contents should never be NULL; it is in this case

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

nit: web_contens

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

Done.

+    // because views doesn't have an implementation for inline disposition yet.
+    // Remove this else when it does. In the meantime, just reforward as if it
+    // were the window disposition.
+    OnServiceChosen(picker_model_->inline_disposition_index(),
+                    WebIntentPickerModel::DISPOSITION_WINDOW);
+  }
 }
 
 void WebIntentPickerController::OnCancelled() {
@@ -209,6 +247,8 @@ void WebIntentPickerController::OnCancelled() {
 }
 
 void WebIntentPickerController::OnClosing() {
+  picker_shown_ = false;
+  picker_ = NULL;
 }
 
 void WebIntentPickerController::OnSendReturnMessage() {
@@ -228,34 +268,31 @@ void WebIntentPickerController::OnSendReturnMessage() {
 
 void WebIntentPickerController::OnWebIntentDataAvailable(
     const std::vector<webkit_glue::WebIntentServiceData>& services) {
-  urls_.clear();
+  std::vector<GURL> urls;
   for (size_t i = 0; i < services.size(); ++i) {
-    urls_.push_back(services[i].service_url);
+    picker_model_->AddItem(services[i].title, services[i].service_url,
+                           ConvertDisposition(services[i].disposition));
+    urls.push_back(services[i].service_url);
   }
-  service_data_ = services;
 
   // Tell the picker to initialize N urls to the default favicon
-  picker_->SetServiceURLs(urls_);
-  favicon_fetcher_->Fetch(urls_);
+  favicon_fetcher_->Fetch(urls);
   pending_async_count_--;
 }
 
-void WebIntentPickerController::OnFaviconDataAvailable(
-    size_t index,
-    const SkBitmap& icon_bitmap) {
-  picker_->SetServiceIcon(index, icon_bitmap);
+void WebIntentPickerController::OnFaviconDataAvailable(size_t index,
+                                                       const gfx::Image& icon) {
+  picker_model_->UpdateFaviconAt(index, icon);
   pending_async_count_--;
 }
 
 void WebIntentPickerController::OnFaviconDataUnavailable(size_t index) {
-  picker_->SetDefaultServiceIcon(index);
   pending_async_count_--;
 }
 
 void WebIntentPickerController::ClosePicker() {
   if (picker_) {
-    picker_factory_->ClosePicker(picker_);
-    picker_ = NULL;
+    picker_->Close();
   }
 }
 
@@ -327,7 +364,8 @@ void WebIntentPickerController::FaviconFetcher::OnFaviconDataAvailable(
     if (gfx::PNGCodec::Decode(favicon_data.image_data->front(),
                               favicon_data.image_data->size(),
                               &icon_bitmap)) {
-      controller_->OnFaviconDataAvailable(index, icon_bitmap);
+      gfx::Image icon_image(new SkBitmap(icon_bitmap));
+      controller_->OnFaviconDataAvailable(index, icon_image);
       return;
     }
   }