Answers in Suggest icon downloading

chrome/browser/ui/views/omnibox/omnibox_result_view.cc

Index: chrome/browser/ui/views/omnibox/omnibox_result_view.cc
diff --git a/chrome/browser/ui/views/omnibox/omnibox_result_view.cc b/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
index b241434426da794fbf5458f05edcb91fc737eb1d..bbec8802a10bf9212fa9de6c5c5435964880cec9 100644
--- a/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
+++ b/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
@@ -17,6 +17,8 @@
 #include "base/memory/scoped_vector.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
+#include "chrome/browser/bitmap_fetcher/bitmap_fetcher_service_factory.h"
+#include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/omnibox/omnibox_popup_model.h"
 #include "chrome/browser/ui/views/location_bar/location_bar_view.h"
 #include "chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.h"
@@ -38,6 +40,34 @@ using ui::NativeTheme;
 
 namespace {
 
+// Calls back to the OmniboxResultView when the requested image is downloaded.
+// This is a separate class instead of being implemented on OmniboxResultView
+// because BitmapFetcherService currently takes ownership of this object.
+// TODO(dschuyler): Make BitmapFetcherService use the more typical non-owning
+// ObserverList pattern and have OmniboxResultView implement the Observer call
+// directly.
+class AnswersImageObserverDesktop : public BitmapFetcherService::Observer {

[Peter Kasting, 2015/03/05 23:11:16]

Nit: How come this has such a long name (and plural "Answers" when it only
observes for one image)?  AnswerImageObserver might be a better name.

[dschuyler, 2015/03/05 23:53:07]

Done.

+ public:
+  explicit AnswersImageObserverDesktop(
+      const base::WeakPtr<OmniboxResultView>& view)
+      : view_(view) {}
+
+  void OnImageChanged(BitmapFetcherService::RequestId request_id,
+                      const SkBitmap& image) override;
+
+ private:
+  const base::WeakPtr<OmniboxResultView> view_;
+  DISALLOW_COPY_AND_ASSIGN(AnswersImageObserverDesktop);
+};
+
+void AnswersImageObserverDesktop::OnImageChanged(
+    BitmapFetcherService::RequestId request_id,
+    const SkBitmap& image) {
+  DCHECK(!image.empty());

[Peter Kasting, 2015/03/05 23:11:16]

It sounds like this DCHECK is safe per the code in BitmapFetcherService, but the
comment in bitmap_fetcher_service.h says otherwise.  Can you update that comment
to be correct?  (I assume the current behavior of the code is OK, unless Rachel
objects.)

[dschuyler, 2015/03/05 23:53:07]

I added a comment for me to follow up on whether to change the code or the
comment.

Done.

+  if (view_)
+    view_->SetAnswerImage(gfx::ImageSkia::CreateFrom1xBitmap(image));
+}
+
 // The minimum distance between the top and bottom of the {icon|text} and the
 // top or bottom of the row.
 const int kMinimumIconVerticalPadding = 2;
@@ -133,13 +163,17 @@ OmniboxResultView::OmniboxResultView(OmniboxPopupContentsView* model,
       model_(model),
       model_index_(model_index),
       location_bar_view_(location_bar_view),
+      image_service_(BitmapFetcherServiceFactory::GetForBrowserContext(
+          location_bar_view_->profile())),
       font_list_(font_list),
       font_height_(
           std::max(font_list.GetHeight(),
                    font_list.DeriveWithStyle(gfx::Font::BOLD).GetHeight())),
       mirroring_context_(new MirroringContext()),
       keyword_icon_(new views::ImageView()),
-      animation_(new gfx::SlideAnimation(this)) {
+      animation_(new gfx::SlideAnimation(this)),
+      request_id_(BitmapFetcherService::REQUEST_ID_INVALID),
+      weak_ptr_factory_(this) {
   CHECK_GE(model_index, 0);
   if (default_icon_size_ == 0) {
     default_icon_size_ =
@@ -154,6 +188,8 @@ OmniboxResultView::OmniboxResultView(OmniboxPopupContentsView* model,
 }
 
 OmniboxResultView::~OmniboxResultView() {
+  if (image_service_)
+    image_service_->CancelRequest(request_id_);
 }
 
 SkColor OmniboxResultView::GetColor(
@@ -175,6 +211,15 @@ void OmniboxResultView::SetMatch(const AutocompleteMatch& match) {
   ResetRenderTexts();
   animation_->Reset();
 
+  answer_image_ = gfx::ImageSkia();
+  if (image_service_) {
+    image_service_->CancelRequest(request_id_);
+    if (match_.answer)

[Peter Kasting, 2015/03/05 23:11:16]

Nit: Need {} since body takes >1 physical line

[dschuyler, 2015/03/05 23:53:07]

Done.

+      request_id_ = image_service_->RequestImage(
+          match_.answer->second_line().image_url(),
+          new AnswersImageObserverDesktop(weak_ptr_factory_.GetWeakPtr()));
+}
+
   AutocompleteMatch* associated_keyword_match = match_.associated_keyword.get();
   if (associated_keyword_match) {
     keyword_icon_->SetImage(GetKeywordIcon());
@@ -222,12 +267,11 @@ int OmniboxResultView::GetTextHeight() const {
   return font_height_;
 }
 
-void OmniboxResultView::PaintMatch(
-    const AutocompleteMatch& match,
-    gfx::RenderText* contents,
-    gfx::RenderText* description,
-    gfx::Canvas* canvas,
-    int x) const {
+void OmniboxResultView::PaintMatch(const AutocompleteMatch& match,
+                                   gfx::RenderText* contents,
+                                   gfx::RenderText* description,
+                                   gfx::Canvas* canvas,
+                                   int x) const {
   int y = text_bounds_.y();
 
   if (!separator_rendertext_) {
@@ -253,6 +297,18 @@ void OmniboxResultView::PaintMatch(
   if (description_max_width != 0) {
     x = DrawRenderText(match, separator_rendertext_.get(), false, canvas, x, y,
                        separator_width_);
+
+    if (!answer_image_.size().IsEmpty()) {

[Peter Kasting, 2015/03/05 23:11:16]

I wasn't thinking through this well enough.  A simple !answer_image_.IsNull()
check would be preferable.  The DCHECK in
AnswersImageObserverDesktop::OnImageChanged() guarantees that if this is
non-null, it's non-empty as well.

[dschuyler, 2015/03/05 23:53:07]

Done.

+      canvas->DrawImageInt(answer_image_,
+                           // Source x, y, w, h.
+                           0, 0, answer_image_.width(), answer_image_.height(),
+                           // Destination x, y, w, h.
+                           GetMirroredXInView(x),
+                           y + kMinimumIconVerticalPadding, default_icon_size_,
+                           default_icon_size_, true);
+      x += default_icon_size_ + LocationBarView::kIconInternalPadding;
+    }
+
     DrawRenderText(match, description, false, canvas, x, y,
                    description_max_width);
   }
@@ -396,6 +452,11 @@ int OmniboxResultView::GetMatchContentsWidth() const {
   return contents_rendertext_ ? contents_rendertext_->GetContentWidth() : 0;
 }
 
+void OmniboxResultView::SetAnswerImage(const gfx::ImageSkia& image) {
+  answer_image_ = image;
+  SchedulePaint();
+}
+
 // TODO(skanuj): This is probably identical across all OmniboxResultView rows in
 // the omnibox dropdown. Consider sharing the result.
 int OmniboxResultView::GetDisplayOffset(