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(OmniboxResultView* view) : view_(view) {}
+
+  void OnImageChanged(BitmapFetcherService::RequestId request_id,
+                      const SkBitmap& image) override;
+
+ private:
+  OmniboxResultView* view_;
+  DISALLOW_COPY_AND_ASSIGN(AnswersImageObserverDesktop);
+};
+
+void AnswersImageObserverDesktop::OnImageChanged(
+    BitmapFetcherService::RequestId request_id, const SkBitmap& image) {
+  DCHECK(!image.empty());
+  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 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),
+    : 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)) {
   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() {
+  BitmapFetcherServiceFactory::GetForBrowserContext(
+      location_bar_view_->profile())->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);
     }
@@ skipping 48 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,

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

This reformatting was done by clang-format.  Should I change it back or let
clang-format have its way?

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

I would just break "int x" to a new line.  I know clang-format packs args at
callsites, but I'm surprised to see it pack args at a definition, especially in
a case like this where almost everything is already one-per-line.

You might consider filing a clang-format bug.

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

While I'm technically fine with either formatting, the entire rest of the file
follows the "indent params 4 spaces" model, so yes, please undo clang-format.
(git cl format is awesome, but usually, you want "git add -p" afterwards so you
can skip the parts where it's clearly doing the wrong thing.)

[Peter Kasting, 2015/02/13 22:49:39]

All omnibox code, at least, should only be indenting 4 (instead of even) if
indenting even doesn't fit.  I assume that's indeed the case.  If not please fix
-- but don't undo the change from indent-4 to indent-even here as that's a good
change.

[dschuyler, 2015/02/14 00:24:31]

Acknowledged.

[dschuyler, 2015/02/14 00:24:31]

Acknowledged.

[dschuyler, 2015/02/14 00:24:32]

If I'm following this correctly, the last thing said was to leave it.

Acknowledged.

+                                   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.

[Peter Kasting, 2015/02/14 00:10:14]

Nit: I'd normally omit in-the-middle-of-a-bunch-of-args comments like this, but
it is true that DrawImageInt() has a lot of args, so up to you.

(We really need to convert DrawImageInt() over to taking Rects, or at least
Points and Sizes, instead of four ints.)

+                           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() {
+  // Check to see if we have already requested this image.
+  const GURL& image_url = match_.answer->second_line().image_url();
+  if (image_url == image_url_)
+    // We have already requested this url.

[Peter Kasting, 2015/02/14 00:10:14]

Nit: A comment here would necessitate {} on the conditional (since the rule is
based around "more than one physical line" rather than "more than one logical
line").  Since the code seems pretty obvious without this, I'd just remove the
comment.

[dschuyler, 2015/02/14 00:24:32]

I removed the image_url_ entirely.  

Done.

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

[Peter Kasting, 2015/02/14 00:10:14]

Nit: This block doesn't actually make a new download request (you do that
below), so I'd omit this comment.

[dschuyler, 2015/02/14 00:24:31]

Done.

+  BitmapFetcherService* image_service =
+      BitmapFetcherServiceFactory::GetForBrowserContext(
+          location_bar_view_->profile());
+  DCHECK(image_service);
+
+  // Cancel the prior request.

[Peter Kasting, 2015/02/14 00:10:15]

Nit: Omit this comment as it merely restates the code.

(Overall I'd probably simply combine everything after the early return into a
single block with a single comment above.  Other than brief summaries of longer
blocks, comments should mostly motivate "why" rather than explain "what" unless
the "what" in the code is very unclear or there are hidden gotchas readers need
to understand.)

[dschuyler, 2015/02/14 00:24:31]

Done.

+  image_service->CancelRequest(request_id_);
+
+  // Request the new image.
+  image_url_ = image_url;
+  request_id_ = image_service->RequestImage(
+      image_url, new AnswersImageObserverDesktop(this));
+}
+
 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 95 matching lines @@
     int x = GetMirroredXForRect(text_bounds_);
     mirroring_context_->Initialize(x, text_bounds_.width());
     InitContentsRenderTextIfNecessary();
 
     if (!description_rendertext_) {
       if (match_.answer) {
         base::string16 text;
         for (const auto& textfield : match_.answer->second_line().text_fields())
           text += textfield.text();
         description_rendertext_ = CreateRenderText(text);
+        if (match_.answer->second_line().image_url().is_valid()) {

[Peter Kasting, 2015/02/14 00:10:14]

Nit: No {} (omnibox file style)

[dschuyler, 2015/02/14 00:24:32]

Done.

+          GetAnswerIcon();
+        }
       } else if (!match_.description.empty()) {
         description_rendertext_ = CreateClassifiedRenderText(
             match_.description, match_.description_class, true);
       }
     }
     PaintMatch(match_, contents_rendertext_.get(),
                description_rendertext_.get(), canvas, x);
   }
 
   AutocompleteMatch* keyword_match = match_.associated_keyword.get();
@@ skipping 15 matching lines @@
     }
     PaintMatch(*keyword_match, keyword_contents_rendertext_.get(),
                keyword_description_rendertext_.get(), canvas, x);
   }
 }
 
 void OmniboxResultView::AnimationProgressed(const gfx::Animation* animation) {
   Layout();
   SchedulePaint();
 }