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..26a1d8b61ef07e20eb909ebc1314d4db64440bb5 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));

[Peter Kasting, 2015/03/03 00:04:01]

Nit: No .get()

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

Done.

+}
+
 // 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,9 @@ OmniboxResultView::OmniboxResultView(OmniboxPopupContentsView* model,
                                      int model_index,
                                      LocationBarView* location_bar_view,
                                      const gfx::FontList& font_list)
-    : edge_item_padding_(LocationBarView::kItemPadding),
+    : image_service_(nullptr),
+      request_id_(BitmapFetcherService::REQUEST_ID_INVALID),
+      edge_item_padding_(LocationBarView::kItemPadding),
       item_padding_(LocationBarView::kItemPadding),
       minimum_text_vertical_padding_(kMinimumTextVerticalPadding),
       model_(model),
@@ -139,7 +166,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_ =
@@ -154,6 +182,8 @@ OmniboxResultView::OmniboxResultView(OmniboxPopupContentsView* model,
 }
 
 OmniboxResultView::~OmniboxResultView() {
+  if (HasImageService())

[Peter Kasting, 2015/03/03 00:04:01]

This will create the image service if it doesn't exist, which you don't want. 
Seems like you should just test |iamge_service_| directly.

[dschuyler, 2015/03/03 00:16:04]

Yep.  I'll change that (but I want to ask questions about the comments below
first).

+    image_service_->CancelRequest(request_id_);
 }
 
 SkColor OmniboxResultView::GetColor(
@@ -175,6 +205,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();
+
   AutocompleteMatch* associated_keyword_match = match_.associated_keyword.get();
   if (associated_keyword_match) {
     keyword_icon_->SetImage(GetKeywordIcon());
@@ -222,12 +256,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 +286,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 +441,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 +475,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
+  // them.
+  if (HasImageService()) {

[Peter Kasting, 2015/03/03 00:04:01]

This now becomes the lone caller of this function.

I would probably collapse not only the HasImageService() code but also the
conditionals at the callsite of this function all into this and call it
something like StartFetchingAnswerImageIfNecessary().

You might also consider initializing |image_service_| in the constructor if
doing so is cheap, since in that case lazy-init mostly just makes the code more
complex.

[dschuyler, 2015/03/03 00:16:04]

It was initializing in the constructor and that seemed to cause some unit tests
to fail with 
@@@STEP_LOG_LINE@AppListControllerBrowserTest.CreateNewWindow@[3248:420:0302/125006:FATAL:omnibox_result_view.cc(183)]
Check failed: image_service_.@@@

So I was looking for a way to deal with that.  Though, it's also possible that I
may be misunderstanding the error in the unit test or maybe there's a better way
to deal with it.

Here's a link to the stdio from the failing tests:
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...

I think I found 'a solution'.  What would be a 'better solution'?

[Peter Kasting, 2015/03/03 00:18:28]

That doesn't sound like the problem was in initializing this in the constructor,
it sounds like the problem was that later code CHECKed that the initialization
always succeeded, whereas, based on your comments below, this can intentionally
return a null service in tests.

I would assume, then, that the solution would be to init in the constructor, but
null-check at the time of use.

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

Done.

+    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";
 }
@@ -575,3 +636,13 @@ void OmniboxResultView::AnimationProgressed(const gfx::Animation* animation) {
   Layout();
   SchedulePaint();
 }
+
+bool OmniboxResultView::HasImageService()
+{
+  if (image_service_ == nullptr) {
+    image_service_ = BitmapFetcherServiceFactory::GetForBrowserContext(
+        location_bar_view_->profile());

[Peter Kasting, 2015/03/03 00:04:01]

Can this call ever actually return null?  Maybe when testing?  If not, we should
write the code unconditionally.

[dschuyler, 2015/03/03 00:16:04]

GetForBrowserContext can return null in unit tests.

+  }
+  return image_service_ != nullptr;
+}
+