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..8f158d1d66521e21857212fd783e06d0392ea2b0 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,33 @@ using ui::NativeTheme;
 
 namespace {
 
+// The observer watches for changes in the image being downloaded.
+class AnswersImageObserverDesktop : public BitmapFetcherService::Observer {
+ public:
+  explicit AnswersImageObserverDesktop(
+    gfx::ImageSkia* image, views::View* view):
+      image_(image),
+      view_(view)
+  {}

[Peter Kasting, 2015/02/13 03:01:07]

Please read the Google style guide on how to format constructors

Nit: I prefer to avoid inlining method definitions into class declarations, even
for classes in .cc files, as it's a request to the compiler to inline things
(which we don't really care about doing) and IMO it's a bit harder to read.

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

Done.

+
+  void OnImageChanged(
+      BitmapFetcherService::RequestId request_id,
+      const SkBitmap& answers_image) override {
+    DCHECK(!answers_image.empty());
+
+    *image_ = gfx::ImageSkia::CreateFrom1xBitmap(answers_image);

[groby-ooo-7-16, 2015/02/13 02:30:56]

Please don't communicate via shared memory. Pass the image into a callback on
the view. 

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

Done.

+
+    // Setup a redraw event so the icon will be drawn sooner.
+    // Without this it will look like the icon is taking longer to download
+    // than it is truly taking.

[Peter Kasting, 2015/02/13 03:01:07]

It seems like if you didn't do this we could potentially _never_ draw the icon
on the view (assuming nothing else was happening).  I think the comment is thus
a bit misleading -- I'd just remove it entirely.

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

Empirically, something else eventually causes a paint and the icon shows up. 
The comment was accurate to the user experience of the issue, but technically
you're correct of course.  If nothing else triggered a paint, the icon would
never show.  
 
Done.

+    view_->SchedulePaint();
+  }
+
+ private:
+  gfx::ImageSkia* image_;
+  views::View* view_;

[groby-ooo-7-16, 2015/02/13 02:30:56]

What exactly is the ownership situation? Is view_ guaranteed to outlive the
request? (I'd assume it currently isn't - you probably want to cancel in-flight
requests when the view is deleted

[Peter Kasting, 2015/02/13 03:01:07]

In that sense, why not just make the view a BitmapFetcherService::Observer
directly?  That avoids all the ownership and callback issues, I think.

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

Because the ImageFetcher takes ownership of the observer :(

[Peter Kasting, 2015/02/13 21:59:32]

Can we Just Fix That?  That design is atypical of observers in the rest of our
codebase, so it's liable to bite someone.  The ImageFetcher should just have an
ObserverList that consumers get registered on.

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

Thanks for pointing out this over-site.  I intended to cancel it and forgot. 
There is now a cancel in the view dtor.
Done.

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

Acknowledged.

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

Acknowledged.

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

Filed http://crbug.com/458681 to fix this. (base::Bind will be a better fit than
ObserverList, I think - usually, callers want to be notified about one specific
request only)

+};

[Peter Kasting, 2015/02/13 03:01:07]

DISALLOW_COPY_AND_ASSIGN

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

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 +156,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),
@@ -227,7 +257,7 @@ void OmniboxResultView::PaintMatch(
     gfx::RenderText* contents,
     gfx::RenderText* description,
     gfx::Canvas* canvas,
-    int x) const {
+    int x) {
   int y = text_bounds_.y();
 
   if (!separator_rendertext_) {
@@ -253,6 +283,27 @@ void OmniboxResultView::PaintMatch(
   if (description_max_width != 0) {
     x = DrawRenderText(match, separator_rendertext_.get(), false, canvas, x, y,
                        separator_width_);
+
+    if (match.answer && match_.answer->second_line().image_url().is_valid()) {
+      // Draw the answer/description image.
+      gfx::ImageSkia image = GetAnswerIcon();
+      if (image.width())

[Peter Kasting, 2015/02/13 03:01:07]

This should probably be !image.size().IsEmpty()

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

Done.

+        canvas->DrawImageInt(
+            image,
+            // Source x, y, w, h.
+            0,
+            0,
+            image.width(),
+            image.height(),
+            // Destination x, y, w, h.
+            GetMirroredXInView(x),
+            y + kMinimumIconVerticalPadding,
+            default_icon_size_,
+            default_icon_size_,
+            true);
+      x += default_icon_size_ + LocationBarView::kIconInternalPadding;

[Peter Kasting, 2015/02/13 03:01:07]

How come we draw the |image| size, but then we step |x| by the
|default_icon_size_|?  What if the two don't match?

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

Maybe I'm confused, but I think that the destination width/height are both
default_icon_size_.  Err, to be more clear, this is a scaling image draw.  The
size of the source image shouldn't matter if I understand correctly.

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

I think you're right.  I read the code explicitly looking to see if that was the
case and still missed it.  Go me.

+    }
+
     DrawRenderText(match, description, false, canvas, x, y,
                    description_max_width);
   }
@@ -425,6 +476,47 @@ int OmniboxResultView::GetDisplayOffset(
 // static
 int OmniboxResultView::default_icon_size_ = 0;
 
+gfx::ImageSkia OmniboxResultView::GetAnswerIcon() {

[Peter Kasting, 2015/02/13 03:01:07]

There's no real need to return |answer_image_| in every return statement.  If
we're always returning the same thing, we can return void and the caller can use
|answer_image_| directly.

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

Done.

+  // Check to see if we have already requested this image.
+  const GURL& image_url = match_.answer->second_line().image_url();
+  if (image_url.is_valid() && image_url == image_url_)

[groby-ooo-7-16, 2015/02/13 02:30:56]

We don't care about this condition. ImageFetcher IIRC will handle this for us.
(And even cache the last <n> images)

That follows the exact same path as if a download was completed, just instantly.

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

I removed the "image_url.is_valid() && " part.  I feel like I should still be
checking the url to avoid spamming the allocation/free of
AnswersImageObserverDesktop objects.  (Am I wrong on that?)

Done.

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

Depends on how frequent this is.  I probably would take the simpler code and
lower memory cost (of not having a GURL member) over the allocation/free pair,
but of course we could get both if we changed how the image fetcher handled
observers as I'm hoping to do :)

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

You shouldn't call GetAnswerIcon unless there's a new set of results that has an
answer - that doesn't really happen that often. So I wouldn't worry about
allocation/deallocation. 

(Long term, we should follow Peter's suggestion and have the image fetcher not
take ownership, so we'll avoid any alloc/dealloc).

In general, unless things happen in a hot loop, code clarity should win over
small optimizations.

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

[groby-ooo-7-16, 2015/02/13 02:30:56]

Can |profile| _ever_ be NULL here?

[Peter Kasting, 2015/02/13 03:01:07]

I don't think so.

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

Acknowledged.

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

Done.

+  if (!profile)
+    return answer_image_;
+  content::BrowserContext* browser_context = profile;

[groby-ooo-7-16, 2015/02/13 02:30:56]

No need for the temporary, just pass the profile directly into
GetForBrowserContext.

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

Done.

+  if (!browser_context)
+    return answer_image_;
+  BitmapFetcherService* image_service =
+      BitmapFetcherServiceFactory::GetForBrowserContext(browser_context);
+  if (!image_service)

[groby-ooo-7-16, 2015/02/13 02:30:56]

Just DCHECK it's not NULL. It never should be, so we don't have to handle it.

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

Done.

+    return answer_image_;
+
+  if (request_id_ != BitmapFetcherService::REQUEST_ID_INVALID) {

[groby-ooo-7-16, 2015/02/13 02:30:56]

Just call CancelRequest, without all the testing. It gracefully handles
cancelling non-existent requests

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

Done.

+    // Cancel the prior request.
+    image_service->CancelRequest(request_id_);
+    // Request the new url instead of the prior one.
+    request_id_ = BitmapFetcherService::REQUEST_ID_INVALID;
+  }
+
+  // Request the new image.
+  AnswersImageObserverDesktop* observer =
+      new AnswersImageObserverDesktop(&answer_image_, this);
+  if (observer) {

[groby-ooo-7-16, 2015/02/13 02:30:56]

Skip the test - new can't fail. (If it does, crashing is just fine)

[Peter Kasting, 2015/02/13 03:01:07]

To be more precise: We have a memory allocator that force-crashes the browser on
OOM, so you'll never see a failed new() call unless you do a bunch of work to
special-case your allocation to allow failure.

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

Done.

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

Acknowledged.

+    image_url_ = image_url;
+    request_id_ = image_service->RequestImage(image_url, observer);
+  }
+
+  // Return what we have (it may already be valid if the image was cached).
+  return answer_image_;

[groby-ooo-7-16, 2015/02/13 02:30:56]

No need to return an image if you always request - it'll always travel through
OnImageChanged.

[Peter Kasting, 2015/02/13 03:01:07]

If OnImageChanged() is called synchronously in the case where we have the image
available, that's fine.  If it's called asynchronously, returning the image here
can make it paint sooner.

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

It is called synchronously when data is retrieved from the cache, yes. 

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

Acknowledged.

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

Done.

+}
+
 const char* OmniboxResultView::GetClassName() const {
   return "OmniboxResultView";
 }