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 fbddb5da4057d6d07acd754fd30d7f30f0d57be1..16f1ca1e7117c6a894849e1583ae27e74b7a87d8 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,25 @@ using ui::NativeTheme;
 
 namespace {
 
+// The observer watches for changes in the image being downloaded.
+class AnswersImageObserverDesktop : public BitmapFetcherService::Observer {
+ public:
+  explicit AnswersImageObserverDesktop(OmniboxResultView* view) : view_(view) {}
+
+  void OnImageChanged(BitmapFetcherService::RequestId request_id,
+                      const SkBitmap& image) override;
+
+ private:
+  OmniboxResultView* view_;
+  DISALLOW_COPY_AND_ASSIGN(AnswersImageObserverDesktop);
+};
+
+void AnswersImageObserverDesktop::OnImageChanged(
+    BitmapFetcherService::RequestId request_id, const SkBitmap& image) {
+  DCHECK(!image.empty());
+  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;
@@ -127,7 +148,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),
@@ -154,6 +176,8 @@ OmniboxResultView::OmniboxResultView(OmniboxPopupContentsView* model,
 }
 
 OmniboxResultView::~OmniboxResultView() {
+  BitmapFetcherServiceFactory::GetForBrowserContext(
+      location_bar_view_->profile())->CancelRequest(request_id_);
 }
 
 SkColor OmniboxResultView::GetColor(
@@ -222,12 +246,10 @@ 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,

[dschuyler, 2015/02/13 22:25:26]

This reformatting was done by clang-format.  Should I change it back or let
clang-format have its way?

[Peter Kasting, 2015/02/13 22:36:36]

I would just break "int x" to a new line.  I know clang-format packs args at
callsites, but I'm surprised to see it pack args at a definition, especially in
a case like this where almost everything is already one-per-line.

You might consider filing a clang-format bug.

[groby-ooo-7-16, 2015/02/13 22:47:22]

While I'm technically fine with either formatting, the entire rest of the file
follows the "indent params 4 spaces" model, so yes, please undo clang-format.
(git cl format is awesome, but usually, you want "git add -p" afterwards so you
can skip the parts where it's clearly doing the wrong thing.)

[Peter Kasting, 2015/02/13 22:49:39]

All omnibox code, at least, should only be indenting 4 (instead of even) if
indenting even doesn't fit.  I assume that's indeed the case.  If not please fix
-- but don't undo the change from indent-4 to indent-even here as that's a good
change.

[dschuyler, 2015/02/14 00:24:31]

Acknowledged.

[dschuyler, 2015/02/14 00:24:31]

Acknowledged.

[dschuyler, 2015/02/14 00:24:32]

If I'm following this correctly, the last thing said was to leave it.

Acknowledged.

+                                   gfx::RenderText* contents,
+                                   gfx::RenderText* description,
+                                   gfx::Canvas* canvas, int x) const {
   int y = text_bounds_.y();
 
   if (!separator_rendertext_) {
@@ -253,6 +275,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.

[Peter Kasting, 2015/02/14 00:10:14]

Nit: I'd normally omit in-the-middle-of-a-bunch-of-args comments like this, but
it is true that DrawImageInt() has a lot of args, so up to you.

(We really need to convert DrawImageInt() over to taking Rects, or at least
Points and Sizes, instead of four ints.)

+                           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 +430,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 +464,31 @@ int OmniboxResultView::GetDisplayOffset(
 // static
 int OmniboxResultView::default_icon_size_ = 0;
 
+void OmniboxResultView::GetAnswerIcon() {
+  // Check to see if we have already requested this image.
+  const GURL& image_url = match_.answer->second_line().image_url();
+  if (image_url == image_url_)
+    // We have already requested this url.

[Peter Kasting, 2015/02/14 00:10:14]

Nit: A comment here would necessitate {} on the conditional (since the rule is
based around "more than one physical line" rather than "more than one logical
line").  Since the code seems pretty obvious without this, I'd just remove the
comment.

[dschuyler, 2015/02/14 00:24:32]

I removed the image_url_ entirely.  

Done.

+    return;
+
+  // We don't have that image, so we will try to get it.
+  answer_image_ = gfx::ImageSkia();
+
+  // Make a new image download request.

[Peter Kasting, 2015/02/14 00:10:14]

Nit: This block doesn't actually make a new download request (you do that
below), so I'd omit this comment.

[dschuyler, 2015/02/14 00:24:31]

Done.

+  BitmapFetcherService* image_service =
+      BitmapFetcherServiceFactory::GetForBrowserContext(
+          location_bar_view_->profile());
+  DCHECK(image_service);
+
+  // Cancel the prior request.

[Peter Kasting, 2015/02/14 00:10:15]

Nit: Omit this comment as it merely restates the code.

(Overall I'd probably simply combine everything after the early return into a
single block with a single comment above.  Other than brief summaries of longer
blocks, comments should mostly motivate "why" rather than explain "what" unless
the "what" in the code is very unclear or there are hidden gotchas readers need
to understand.)

[dschuyler, 2015/02/14 00:24:31]

Done.

+  image_service->CancelRequest(request_id_);
+
+  // Request the new image.
+  image_url_ = image_url;
+  request_id_ = image_service->RequestImage(
+      image_url, new AnswersImageObserverDesktop(this));
+}
+
 const char* OmniboxResultView::GetClassName() const {
   return "OmniboxResultView";
 }
@@ -540,6 +604,9 @@ void OmniboxResultView::OnPaint(gfx::Canvas* canvas) {
         for (const auto& textfield : match_.answer->second_line().text_fields())
           text += textfield.text();
         description_rendertext_ = CreateRenderText(text);
+        if (match_.answer->second_line().image_url().is_valid()) {

[Peter Kasting, 2015/02/14 00:10:14]

Nit: No {} (omnibox file style)

[dschuyler, 2015/02/14 00:24:32]

Done.

+          GetAnswerIcon();
+        }
       } else if (!match_.description.empty()) {
         description_rendertext_ = CreateClassifiedRenderText(
             match_.description, match_.description_class, true);