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..1c7379693f0a5a53f8eb53d2b472d165783ed105 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,29 @@ using ui::NativeTheme;
 
 namespace {
 
+// The observer watches for changes in the image being downloaded.
+class AnswersImageObserverDesktop : public BitmapFetcherService::Observer {
+ 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());
+  if (view_)
+    view_.get()->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;
@@ -127,7 +152,8 @@ OmniboxResultView::OmniboxResultView(OmniboxPopupContentsView* model,
                                      int model_index,
                                      LocationBarView* location_bar_view,
                                      const gfx::FontList& font_list)
-    : edge_item_padding_(LocationBarView::kItemPadding),
+    : request_id_(BitmapFetcherService::REQUEST_ID_INVALID),
+      edge_item_padding_(LocationBarView::kItemPadding),
       item_padding_(LocationBarView::kItemPadding),
       minimum_text_vertical_padding_(kMinimumTextVerticalPadding),
       model_(model),
@@ -139,7 +165,8 @@ OmniboxResultView::OmniboxResultView(OmniboxPopupContentsView* model,
                    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)),
+      weak_ptr_factory_(this) {
   CHECK_GE(model_index, 0);
   if (default_icon_size_ == 0) {
     default_icon_size_ =
@@ -151,9 +178,13 @@ OmniboxResultView::OmniboxResultView(OmniboxPopupContentsView* model,
   keyword_icon_->EnableCanvasFlippingForRTLUI(true);
   keyword_icon_->SetImage(GetKeywordIcon());
   keyword_icon_->SizeToPreferredSize();
+  image_service_ = BitmapFetcherServiceFactory::GetForBrowserContext(
+      location_bar_view_->profile());

[Peter Kasting, 2015/03/03 01:32:36]

Nit: Init this in the initializer list

[dschuyler, 2015/03/03 21:13:56]

Done.

 }
 
 OmniboxResultView::~OmniboxResultView() {
+  if (image_service_)
+    image_service_->CancelRequest(request_id_);
 }
 
 SkColor OmniboxResultView::GetColor(
@@ -175,6 +206,10 @@ void OmniboxResultView::SetMatch(const AutocompleteMatch& match) {
   ResetRenderTexts();
   animation_->Reset();
 
+  answer_image_ = gfx::ImageSkia();
+  if (match_.answer && match_.answer->second_line().image_url().is_valid())
+    GetAnswerIcon();

[Peter Kasting, 2015/03/03 01:32:36]

It seems sort of weird to have caller code that conditionally calls a function,
and have the function immediately do a further conditional check.  Plus, both
sides want to look at match_.answer->second_line().image_url(), so by extracting
the code in GetAnswerIcon() out, we're not really improving any kind of
encapsulation.

I would respond to all this by collapsing together all this code; either call
the function unconditionally here and move the conditionals there, or (probably
better) just inline the contents of that function into here, since this is the
lone callsite and the function body is short.

Then the two separate conditionals can be one statement.

[dschuyler, 2015/03/03 21:13:56]

Done.

+
   AutocompleteMatch* associated_keyword_match = match_.associated_keyword.get();
   if (associated_keyword_match) {
     keyword_icon_->SetImage(GetKeywordIcon());
@@ -222,12 +257,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 +287,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()) {
+      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 +442,11 @@ int OmniboxResultView::GetMatchContentsWidth() const {
   return contents_rendertext_ ? contents_rendertext_->GetContentWidth() : 0;
 }
 
+void OmniboxResultView::SetAnswerImage(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(
@@ -425,6 +476,17 @@ int OmniboxResultView::GetDisplayOffset(
 // static
 int OmniboxResultView::default_icon_size_ = 0;
 
+void OmniboxResultView::GetAnswerIcon() {
+  // The answer images are not bundled with Chrome, so we need to download

[Peter Kasting, 2015/03/03 01:32:36]

Is there a reason the reader would have expected these images to be bundled with
Chrome?  If not, I would probably omit this comment.

[dschuyler, 2015/03/03 21:13:56]

Done.

+  // them.
+  if (image_service_) {
+    image_service_->CancelRequest(request_id_);
+    request_id_ = image_service_->RequestImage(
+        match_.answer->second_line().image_url(),
+        new AnswersImageObserverDesktop(GetWeakPtr()));
+  }
+}
+
 const char* OmniboxResultView::GetClassName() const {
   return "OmniboxResultView";
 }