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

chrome/browser/ui/intents/web_intent_picker_controller.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/intents/web_intent_picker_controller.h"
 
 #include <vector>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/favicon/favicon_service.h"
 #include "chrome/browser/intents/web_intents_registry_factory.h"
 #include "chrome/browser/intents/cws_intents_registry_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/tab_contents/tab_util.h"
 #include "chrome/browser/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/browser_list.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_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"
 #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"
 
 namespace {
 
 // Gets the favicon service for the profile in |tab_contents|.
 FaviconService* GetFaviconService(TabContentsWrapper* wrapper) {
   return wrapper->profile()->GetFaviconService(Profile::EXPLICIT_ACCESS);
 }
 
@@ skipping 13 matching lines @@
     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;
   }
 }
 
+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(
     TabContentsWrapper* wrapper)
     : wrapper_(wrapper),
       picker_(NULL),
       picker_model_(new WebIntentPickerModel()),
       pending_async_count_(0),
       picker_shown_(false),
       intents_dispatcher_(NULL),
@@ skipping 205 matching lines @@
       picker_model_->UpdateFaviconAt(index, icon_image);
       return;
     }
   }
 
   AsyncOperationFinished();
 }
 
 void WebIntentPickerController::OnCWSIntentServicesAvailable(
     const CWSIntentsRegistry::IntentExtensionList& extensions) {
+  net::URLRequestContextGetter* context =

[Greg Billock, 2012/02/23 22:15:50]

It looks like this can just move down to
SetRequestContext(wrapper_->profile()->GetReqCtx())?

[binji, 2012/02/23 22:33:41]

Done.

+      wrapper_->profile()->GetRequestContext();
+
   for (size_t i = 0; i < extensions.size(); ++i) {
     const CWSIntentsRegistry::IntentExtensionInfo& info = extensions[i];
     picker_model_->AddSuggestedExtension(
         info.name,
         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();

[Greg Billock, 2012/02/23 22:15:50]

So the URLFetcher always delegates the callback to the UI thread? That seems
like something it'd let the caller manage, but OK.

[binji, 2012/02/23 22:33:41]

I wasn't 100% on this, so I took a look. It looks like the delegate is always
called on the same thread as Create is called -- probably just for convenience.

   }
 
   AsyncOperationFinished();
 }
 
+void WebIntentPickerController::OnExtensionIconURLFetchComplete(
+    const string16& extension_id, const content::URLFetcher* source) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  if (source->GetResponseCode() != 200) {
+    AsyncOperationFinished();
+    return;
+  }
+
+  scoped_ptr<std::string> response(new std::string);
+  if (source->GetResponseAsString(response.get())) {

[Greg Billock, 2012/02/23 22:15:50]

How about if (!...) { AsyncOpFinished; return; }

[binji, 2012/02/23 22:33:41]

Done.

+    DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));

[Greg Billock, 2012/02/23 22:15:50]

Can take this out (you have it above now).

[binji, 2012/02/23 22:33:41]

Oh, right. :-)

+
+    // Decode PNG and resize on worker thread.
+    content::BrowserThread::PostBlockingPoolTask(
+        FROM_HERE,
+        base::Bind(&DecodeExtensionIconAndResize,
+                   base::Passed(&response),
+                   base::Bind(

[Greg Billock, 2012/02/23 22:15:50]

I think it'd read easier to extract these two callbacks as named variables.

[binji, 2012/02/23 22:33:41]

Done.

+                       &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(
+        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 22:15:50]

It might be easier to get a WebIntentPickerController ref and just call these
known methods on it. If this isn't an instance method, you could just not make
it static and stick it local to this .cc file and not publish it at all. That
might be the best given the thread situation.

[binji, 2012/02/23 22:33:41]

This was the original way I wrote it, but it didn't seem possible. I need to get
a weak pointer to the WebIntentPickerController to ensure that the callback
doesn't happen after it is destroyed, but that is not allowed on a different
thread than created the WeakPtrFactory.

In fact, I was getting an assert just dereferencing the weak "this" from a
different thread, which is why DecodeExtensionIconAndResize is static.

I suppose it could be file local, but then it should be in an anonymous
namespace at the top of the file, which I think makes it a little harder to
follow. What do you think?

[Greg Billock, 2012/02/23 22:51:30]

You want to put some of these threading constraints in a comment? There's quite
a bit of hidden information about what's possible that'd be good to write down.

I think I was trying to ask about a couple different options in this comment,
but it came out kind of garbled. What I meant was, if it isn't possible to have
this be an instance method (because of the weak ptr), then how about just making
it file-local? (Up by the trampoline)

On 2012/02/23 22:33:41, binji wrote:

[binji, 2012/02/23 23:34:48]

Done.

+  } 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() {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   if (--pending_async_count_ == 0) {
     picker_->OnPendingAsyncCompleted();
   }
 }
 
 void WebIntentPickerController::ClosePicker() {
   if (picker_) {
     picker_->Close();
   }
 }