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..39c9a72c59ce41db1469c89beb6242192bdb11da 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.

[Peter Kasting, 2015/03/05 00:33:43]

Nit: How about something like:

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.

[dschuyler, 2015/03/05 18:31:03]

Done.

+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_;

[groby-ooo-7-16, 2015/03/05 15:44:54]

I'm still not entirely convinced this needs to be a WeakPtr, but willing to
decouple that discussion from this CL.

[dschuyler, 2015/03/05 22:19:03]

Let's look at that with the change about who owns the image observer.

+  DISALLOW_COPY_AND_ASSIGN(AnswersImageObserverDesktop);
+};
+
+void AnswersImageObserverDesktop::OnImageChanged(
+    BitmapFetcherService::RequestId request_id,
+    const SkBitmap& image) {
+  DCHECK(!image.empty());

[Peter Kasting, 2015/03/05 00:33:43]

Just to make sure, if the received image was corrupt or empty, the
BitmapFetcherService would detect that and not call us?

[groby-ooo-7-16, 2015/03/05 15:44:54]

Nice catch - it actually will always call. See the comment on OnImageChanged:
"Called with an empty image if the fetch failed or the request ended for any
reason."

At this point, we won't get corrupt images. The helper process is there to
prevent those from ever making it into Chrome proper.

[dschuyler, 2015/03/05 18:31:04]

I missed the comment, I looked at the code.  which should the caller do - check
or not check the image?

void BitmapFetcherRequest::NotifyImageChanged(const SkBitmap& bitmap) {
  if (!bitmap.empty())
    observer_->OnImageChanged(request_id_, bitmap);
}

[dschuyler, 2015/03/05 18:31:04]

The caller currently does check the image.  So the check should never fire
unless something changes on that end. 

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 +158,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 +183,8 @@ OmniboxResultView::OmniboxResultView(OmniboxPopupContentsView* model,
 }
 
 OmniboxResultView::~OmniboxResultView() {
+  if (image_service_)
+    image_service_->CancelRequest(request_id_);
 }
 
 SkColor OmniboxResultView::GetColor(
@@ -175,6 +206,15 @@ void OmniboxResultView::SetMatch(const AutocompleteMatch& match) {
   ResetRenderTexts();
   animation_->Reset();
 
+  answer_image_ = gfx::ImageSkia();
+  if (match_.answer && match_.answer->second_line().image_url().is_valid() &&

[groby-ooo-7-16, 2015/03/05 15:44:54]

There's no need to check URL validity - the fetcher does that for you (and
returns an empty image if it's invalid).

I also wonder if there's a point in having a match.GetAnswerImageUrl() function.
It'd clean this up significantly:

image_service_->CancelRequest(request_id_);
request_id = image_service_->RequestImage(
    match_.GetAnswerImageURL(),
    new AnswersImageObserverDesktop(weak_ptr_factory_.GetWeakPtr()));


But even if there isn't, there's no need to check the URLs validity.
BitmapFetcherService does that for you

[dschuyler, 2015/03/05 22:19:03]

The accessor for the image url sounds cool, though I'd like to get this feature
in :)  May I followup with that in another CL?

Done.

+      image_service_) {
+    image_service_->CancelRequest(request_id_);

[groby-ooo-7-16, 2015/03/05 15:44:54]

I'd assume you'd want to cancel a pending request even if the new match doesn't
have an image - otherwise, a pending image request might come in late and do
interesting things :)

[dschuyler, 2015/03/05 22:19:03]

Ah, good one :)

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 +262,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 +292,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()) {

[groby-ooo-7-16, 2015/03/05 15:44:54]

Isn't that covered by answer_image_.IsNull()?

[Peter Kasting, 2015/03/05 18:28:36]

I don't think so, though I'm not sure.  It depends whether an empty image is
always collapsed to null storage, or if we can have non-null storage that's for
a zero-width/zero-height image.  The Skia/gfx folks might know better?

I suggested this route because it seemed safer than the IsNull() route.

[dschuyler, 2015/03/05 22:19:03]

Is it ok to leave in the empty check for now?

+      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 +447,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(