Answers in Suggest icon downloading

chrome/browser/ui/views/omnibox/omnibox_result_view.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // For WinDDK ATL compatibility, these ATL headers must come first.
 #include "build/build_config.h"
 #if defined(OS_WIN)
 #include <atlbase.h>  // NOLINT
 #include <atlwin.h>  // NOLINT
 #endif
 
 #include "chrome/browser/ui/views/omnibox/omnibox_result_view.h"
 
 #include <algorithm>  // NOLINT
 
 #include "base/i18n/bidi_line_iterator.h"
 #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"
 #include "chrome/grit/generated_resources.h"
 #include "components/omnibox/suggestion_answer.h"
 #include "grit/components_scaled_resources.h"
 #include "grit/theme_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/theme_provider.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/color_utils.h"
 #include "ui/gfx/image/image.h"
 #include "ui/gfx/range/range.h"
 #include "ui/gfx/render_text.h"
 #include "ui/gfx/text_utils.h"
 #include "ui/native_theme/native_theme.h"
 
 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;
 const int kMinimumTextVerticalPadding = 3;
 
 // A mapping from OmniboxResultView's ResultViewState/ColorKind types to
 // NativeTheme colors.
 struct TranslationTable {
   ui::NativeTheme::ColorId id;
   OmniboxResultView::ResultViewState state;
@@ skipping 75 matching lines @@
 OmniboxResultView::OmniboxResultView(OmniboxPopupContentsView* model,
                                      int model_index,
                                      LocationBarView* location_bar_view,
                                      const gfx::FontList& font_list)
     : edge_item_padding_(LocationBarView::kItemPadding),
       item_padding_(LocationBarView::kItemPadding),
       minimum_text_vertical_padding_(kMinimumTextVerticalPadding),
       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_ =
         location_bar_view_->GetThemeProvider()->GetImageSkiaNamed(
             AutocompleteMatch::TypeToIcon(
                 AutocompleteMatchType::URL_WHAT_YOU_TYPED))->width();
   }
   keyword_icon_->set_owned_by_client();
   keyword_icon_->EnableCanvasFlippingForRTLUI(true);
   keyword_icon_->SetImage(GetKeywordIcon());
   keyword_icon_->SizeToPreferredSize();
 }
 
 OmniboxResultView::~OmniboxResultView() {
+  if (image_service_)
+    image_service_->CancelRequest(request_id_);
 }
 
 SkColor OmniboxResultView::GetColor(
     ResultViewState state,
     ColorKind kind) const {
   for (size_t i = 0; i < arraysize(kTranslationTable); ++i) {
     if (kTranslationTable[i].state == state &&
         kTranslationTable[i].kind == kind) {
       return GetNativeTheme()->GetSystemColor(kTranslationTable[i].id);
     }
   }
 
   NOTREACHED();
   return SK_ColorRED;
 }
 
 void OmniboxResultView::SetMatch(const AutocompleteMatch& match) {
   match_ = 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());
     if (!keyword_icon_->parent())
       AddChildView(keyword_icon_.get());
   } else if (keyword_icon_->parent()) {
     RemoveChildView(keyword_icon_.get());
   }
 
   Layout();
@@ skipping 27 matching lines @@
 OmniboxResultView::ResultViewState OmniboxResultView::GetState() const {
   if (model_->IsSelectedIndex(model_index_))
     return SELECTED;
   return model_->IsHoveredIndex(model_index_) ? HOVERED : NORMAL;
 }
 
 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_) {
     const base::string16& separator =
         l10n_util::GetStringUTF16(IDS_AUTOCOMPLETE_MATCH_DESCRIPTION_SEPARATOR);
     separator_rendertext_.reset(CreateRenderText(separator).release());
     separator_rendertext_->SetColor(GetColor(GetState(), DIMMED_TEXT));
     separator_width_ = separator_rendertext_->GetContentWidth();
   }
 
   int contents_max_width, description_max_width;
   OmniboxPopupModel::ComputeMatchMaxWidths(
       contents->GetContentWidth(),
       separator_width_,
       description ? description->GetContentWidth() : 0,
       mirroring_context_->remaining_width(x),
       !AutocompleteMatch::IsSearchType(match.type),
       &contents_max_width,
       &description_max_width);
 
   x = DrawRenderText(match, contents, true, canvas, x, y, contents_max_width);
 
   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);
   }
 }
 
 int OmniboxResultView::DrawRenderText(
     const AutocompleteMatch& match,
     gfx::RenderText* render_text,
     bool contents,
     gfx::Canvas* canvas,
@@ skipping 123 matching lines @@
     render_text->ApplyColor(GetColor(GetState(), color_kind), current_range);
   }
   return render_text.Pass();
 }
 
 int OmniboxResultView::GetMatchContentsWidth() const {
   InitContentsRenderTextIfNecessary();
   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(
     const AutocompleteMatch& match,
     bool is_ui_rtl,
     bool is_match_contents_rtl) const {
   if (match.type != AutocompleteMatchType::SEARCH_SUGGEST_TAIL)
     return 0;
 
   const base::string16& input_text =
@@ skipping 159 matching lines @@
     }
     PaintMatch(*keyword_match, keyword_contents_rendertext_.get(),
                keyword_description_rendertext_.get(), canvas, x);
   }
 }
 
 void OmniboxResultView::AnimationProgressed(const gfx::Animation* animation) {
   Layout();
   SchedulePaint();
 }