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.
+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;
 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 69 matching lines @@
   int center_;
   int right_;
 
   DISALLOW_COPY_AND_ASSIGN(MirroringContext);
 };
 
 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),
       model_index_(model_index),
       location_bar_view_(location_bar_view),
       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)),
+      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 (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(
     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())
+    GetAnswerIcon();
+
   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()) {
+      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 =
       base::UTF8ToUTF16(match.GetAdditionalInfo(kACMatchPropertyInputText));
   int contents_start_index = 0;
   base::StringToInt(match.GetAdditionalInfo(kACMatchPropertyContentsStartIndex),
                     &contents_start_index);
 
   scoped_ptr<gfx::RenderText> input_render_text(CreateRenderText(input_text));
   const gfx::Range& glyph_bounds =
       input_render_text->GetGlyphBounds(contents_start_index);
   const int start_padding = is_match_contents_rtl ?
       std::max(glyph_bounds.start(), glyph_bounds.end()) :
       std::min(glyph_bounds.start(), glyph_bounds.end());
 
   return is_ui_rtl ?
       (input_render_text->GetContentWidth() - start_padding) : start_padding;
 }
 
 // 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";
 }
 
 gfx::ImageSkia OmniboxResultView::GetIcon() const {
   const gfx::Image image = model_->GetIconIfExtensionMatch(model_index_);
   if (!image.IsEmpty())
     return image.AsImageSkia();
 
   int icon = model_->IsStarredMatch(match_) ?
@@ skipping 130 matching lines @@
     }
     PaintMatch(*keyword_match, keyword_contents_rendertext_.get(),
                keyword_description_rendertext_.get(), canvas, x);
   }
 }
 
 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;
+}
+