[Web Intents] WebIntentsPickerController will now fetch icon for suggested extensions.

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 fbe21840679e0917dff040c7dda430bb257b33c8..1d3f326b568ce945bb55ea0f6f5b1102cca86019 100644
--- a/chrome/browser/ui/intents/web_intent_picker_controller.cc
+++ b/chrome/browser/ui/intents/web_intent_picker_controller.cc
@@ -23,10 +23,16 @@
 #include "chrome/browser/webdata/web_data_service.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "content/browser/intents/intent_injector.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_source.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_intents_dispatcher.h"
+#include "content/public/common/url_fetcher.h"
+#include "content/public/common/url_fetcher_delegate.h"
+#include "net/base/load_flags.h"
+#include "skia/ext/image_operations.h"
 #include "ui/gfx/codec/png_codec.h"
+#include "ui/gfx/favicon_size.h"
 #include "ui/gfx/image/image.h"
 #include "webkit/glue/web_intent_service_data.h"
 
@@ -60,6 +66,25 @@ WebIntentPickerModel::Disposition ConvertDisposition(
   }
 }
 
+class URLFetcherTrampoline : public content::URLFetcherDelegate {
+ public:
+  typedef base::Callback<void(const content::URLFetcher* source)> Callback;
+
+  explicit URLFetcherTrampoline(const Callback& callback)
+      : callback_(callback) {}
+  ~URLFetcherTrampoline() {}
+
+  // content::URLFetcherDelegate implementation.
+  virtual void OnURLFetchComplete(const content::URLFetcher* source) OVERRIDE {
+    callback_.Run(source);
+    delete source;
+    delete this;
+  }
+
+ private:
+  Callback callback_;
+};
+
 }  // namespace
 
 WebIntentPickerController::WebIntentPickerController(
@@ -285,6 +310,9 @@ void WebIntentPickerController::OnFaviconDataAvailable(
 
 void WebIntentPickerController::OnCWSIntentServicesAvailable(
     const CWSIntentsRegistry::IntentExtensionList& extensions) {
+  net::URLRequestContextGetter* context =
+      wrapper_->profile()->GetRequestContext();
+
   for (size_t i = 0; i < extensions.size(); ++i) {
     const CWSIntentsRegistry::IntentExtensionInfo& info = extensions[i];
     picker_model_->AddSuggestedExtension(
@@ -292,12 +320,92 @@ void WebIntentPickerController::OnCWSIntentServicesAvailable(
         info.id,
         info.average_rating);
 
-    // TODO(binji): Fetch extension icon.
+    pending_async_count_++;
+    content::URLFetcher* icon_url_fetcher = content::URLFetcher::Create(
+        0,
+        info.icon_url,
+        content::URLFetcher::GET,
+        new URLFetcherTrampoline(
+            base::Bind(
+                &WebIntentPickerController::OnExtensionIconURLFetchComplete,
+                weak_ptr_factory_.GetWeakPtr(), info.id)));
+
+    icon_url_fetcher->SetLoadFlags(
+        net::LOAD_DO_NOT_SEND_COOKIES | net::LOAD_DO_NOT_SAVE_COOKIES);
+    icon_url_fetcher->SetRequestContext(context);
+    icon_url_fetcher->Start();
   }
 
   AsyncOperationFinished();
 }
 
+void WebIntentPickerController::OnExtensionIconURLFetchComplete(

[Greg Billock, 2012/02/23 18:38:34]

This'll get called by the fetcher thread, right? Is it safe to use that thread
to do the icon decoding, or is that a no-no?

[binji, 2012/02/23 19:06:13]

AFAIK this is called back on the UI thread, so it seemed like we shouldn't do
much work on it (even if decoding and resizing a PNG is lightweight).

+    const string16& extension_id, const content::URLFetcher* source) {
+  if (source->GetResponseCode() != 200)
+    return;

[Greg Billock, 2012/02/23 18:38:34]

Need to AsyncOperationFinished here?

[binji, 2012/02/23 19:06:13]

Done.

+
+  scoped_ptr<std::string> response(new std::string());

[Greg Billock, 2012/02/23 18:38:34]

Can kill inner "()"

[binji, 2012/02/23 19:06:13]

Done.

+  if (source->GetResponseAsString(response.get())) {
+    DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+    // Decode PNG and resize on worker thread.
+    content::BrowserThread::PostBlockingPoolTask(

[Greg Billock, 2012/02/23 18:38:34]

These cascades are why the frontends ended up using state-machine-driven
callback systems. After a while it gets hard to track the closures and error
states. :-)

[binji, 2012/02/23 19:06:13]

Agreed. Though I think it's not too hard to follow here...

+        FROM_HERE,
+        base::Bind(&DecodeExtensionIconAndResize,
+                   base::Passed(&response),
+                   base::Bind(
+                       &WebIntentPickerController::OnExtensionIconAvailable,
+                       weak_ptr_factory_.GetWeakPtr(),
+                       extension_id),
+                   base::Bind(
+                       &WebIntentPickerController::OnExtensionIconUnavailable,
+                       weak_ptr_factory_.GetWeakPtr(),
+                       extension_id)));
+  } else {
+    AsyncOperationFinished();
+  }
+}
+
+// static
+void WebIntentPickerController::DecodeExtensionIconAndResize(
+    scoped_ptr<std::string> icon_response,
+    const ExtensionIconAvailableCallback& callback,
+    const base::Closure& unavailable_callback) {
+  SkBitmap icon_bitmap;
+  if (gfx::PNGCodec::Decode(
+        reinterpret_cast<const unsigned char*>(icon_response->data()),
+        icon_response->length(),
+        &icon_bitmap)) {
+    SkBitmap resized_icon = skia::ImageOperations::Resize(

[Greg Billock, 2012/02/23 18:38:34]

Is this doing an extra image copy?

[binji, 2012/02/23 19:06:13]

Yes, it doesn't resize in-place.

+        icon_bitmap,
+        skia::ImageOperations::RESIZE_BEST,
+        gfx::kFaviconSize, gfx::kFaviconSize);
+    gfx::Image icon_image(new SkBitmap(resized_icon));
+
+    content::BrowserThread::PostTask(
+        content::BrowserThread::UI,
+        FROM_HERE,
+        base::Bind(callback, icon_image));

[Greg Billock, 2012/02/23 18:38:34]

So gfx::Image is pointer-ish and cool to pass like this?

[binji, 2012/02/23 19:06:13]

Yes, gfx::Image has internal refcounted storage.

+  } else {
+    content::BrowserThread::PostTask(
+        content::BrowserThread::UI,
+        FROM_HERE,
+        unavailable_callback);
+  }
+}
+
+void WebIntentPickerController::OnExtensionIconAvailable(
+    const string16& extension_id,
+    const gfx::Image& icon_image) {
+  picker_model_->SetSuggestedExtensionIconWithId(extension_id, icon_image);
+  AsyncOperationFinished();
+}
+
+void WebIntentPickerController::OnExtensionIconUnavailable(
+    const string16& extension_id) {
+  AsyncOperationFinished();
+}
+
 void WebIntentPickerController::AsyncOperationFinished() {
   if (--pending_async_count_ == 0) {

[Greg Billock, 2012/02/23 18:38:34]

So we don't need a mutex here because Decode() is posting the callbacks back to
the browser thread? Would it just be easier to have them call directly? There's
very little contention risk... Or would the model freak out to be called
multi-threaded?

[binji, 2012/02/23 19:06:13]

This callback is only used to notify a test that all operations are finished.
Having it call multi-threaded makes those tests more complicated, I think.

I'll add a thread check for this function to make it a little clearer.

     picker_->OnPendingAsyncCompleted();