[Omnibox] show alternate answers images

This CL shows answer images that are delivered through the alternate json property

Bug: 843370
Change-Id: Ic0a2088b16b5f2d00944719c46c5ff1724d7cc90
Reviewed-on: https://chromium-review.googlesource.com/1066973
Commit-Queue: Dave Schuyler <dschuyler@chromium.org>
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560796}

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

@@ -240,6 +240,7 @@ void OmniboxMatchCellView::OnMatchUpdate(const OmniboxResultView* result_view,
     // An entry with |is_old_style_answer_| may use the image_view_. But it's
     // set when the image arrives (later).
     image_view_->SetImage(gfx::ImageSkia());
+    image_view_->SetSize(gfx::Size());
   } else {
     SkColor color = result_view->GetColor(OmniboxPart::RESULTS_BACKGROUND);
     extensions::image_util::ParseHexColorString(match.image_dominant_color,

components/omnibox/browser/autocomplete_match.cc

@@ -494,7 +494,7 @@ GURL AutocompleteMatch::GURLToStrippedGURL(
     }
   }
 
-  // |replacements| keeps all the substitions we're going to make to
+  // |replacements| keeps all the substitutions we're going to make to
   // from {destination_url} to {stripped_destination_url}.  |need_replacement|
   // is a helper variable that helps us keep track of whether we need
   // to apply the replacement.
@@ -663,7 +663,7 @@ TemplateURL* AutocompleteMatch::GetTemplateURL(
 }
 
 GURL AutocompleteMatch::ImageUrl() const {
-  return answer ? answer->second_line().image_url() : GURL(image_url);
+  return answer ? answer->image_url() : GURL(image_url);
 }
 
 void AutocompleteMatch::RecordAdditionalInfo(const std::string& property,

components/omnibox/browser/suggestion_answer.cc

@@ -8,11 +8,13 @@
 
 #include <memory>
 
+#include "base/feature_list.h"
 #include "base/i18n/rtl.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/trace_event/memory_usage_estimator.h"
 #include "base/values.h"
+#include "components/omnibox/browser/omnibox_field_trial.h"
 #include "net/base/escape.h"
 #include "url/url_constants.h"
 
@@ -207,7 +209,9 @@ size_t SuggestionAnswer::ImageLine::EstimateMemoryUsage() const {
 
 SuggestionAnswer::SuggestionAnswer() : type_(-1) {}
 
-SuggestionAnswer::~SuggestionAnswer() {}
+SuggestionAnswer::SuggestionAnswer(const SuggestionAnswer& answer) = default;
+
+SuggestionAnswer::~SuggestionAnswer() = default;
 
 // static
 std::unique_ptr<SuggestionAnswer> SuggestionAnswer::ParseAnswer(
@@ -216,38 +220,53 @@ std::unique_ptr<SuggestionAnswer> SuggestionAnswer::ParseAnswer(
 
   const base::ListValue* lines_json;
   if (!answer_json->GetList(kAnswerJsonLines, &lines_json) ||
-      lines_json->GetSize() != 2)
+      lines_json->GetSize() != 2) {
     return nullptr;
+  }
 
   const base::DictionaryValue* first_line_json;
   if (!lines_json->GetDictionary(0, &first_line_json) ||
-      !ImageLine::ParseImageLine(first_line_json, &result->first_line_))
+      !ImageLine::ParseImageLine(first_line_json, &result->first_line_)) {
     return nullptr;
+  }
 
   const base::DictionaryValue* second_line_json;
   if (!lines_json->GetDictionary(1, &second_line_json) ||
-      !ImageLine::ParseImageLine(second_line_json, &result->second_line_))
+      !ImageLine::ParseImageLine(second_line_json, &result->second_line_)) {
     return nullptr;
+  }
+
+  std::string image_url;
+  const base::DictionaryValue* optional_image;
+  if (base::FeatureList::IsEnabled(omnibox::kOmniboxNewAnswerLayout) &&
+      answer_json->GetDictionary("i", &optional_image) &&
+      optional_image->GetString("d", &image_url)) {
+    result->image_url_ = GURL(image_url);
+  } else {
+    result->image_url_ = result->second_line_.image_url();
+  }
 
   return result;
 }
 
 bool SuggestionAnswer::Equals(const SuggestionAnswer& answer) const {
-  return type_ == answer.type_ &&
+  return type_ == answer.type_ && image_url_ == answer.image_url_ &&
          first_line_.Equals(answer.first_line_) &&
          second_line_.Equals(answer.second_line_);
 }
 
 void SuggestionAnswer::AddImageURLsTo(std::vector<GURL>* urls) const {
-  if (first_line_.image_url().is_valid())
-    urls->push_back(first_line_.image_url());
-  if (second_line_.image_url().is_valid())
+  // Note: first_line_.image_url() is not used in practice (so it's ignored).
+  if (image_url_.is_valid())
+    urls->push_back(image_url_);
+  else if (second_line_.image_url().is_valid())
     urls->push_back(second_line_.image_url());
 }
 
 size_t SuggestionAnswer::EstimateMemoryUsage() const {
   size_t res = 0;
 
+  res += base::trace_event::EstimateMemoryUsage(image_url_);
   res += base::trace_event::EstimateMemoryUsage(first_line_);
   res += base::trace_event::EstimateMemoryUsage(second_line_);
 

components/omnibox/browser/suggestion_answer.h

@@ -28,7 +28,7 @@ class DictionaryValue;
 // When represented in the UI, these elements should be styled and laid out
 // according to the specification at https://goto.google.com/ais_api.
 //
-// Each of the three classes has either an explicit or implicity copy
+// Each of the three classes has either an explicit or implicit copy
 // constructor to support copying answer values (via SuggestionAnswer::copy) as
 // members of SuggestResult and AutocompleteMatch.
 class SuggestionAnswer {
@@ -93,7 +93,7 @@ class SuggestionAnswer {
     FRIEND_TEST_ALL_PREFIXES(SuggestionAnswerTest, DifferentValuesAreUnequal);
 
     // No DISALLOW_COPY_AND_ASSIGN since we depend on the copy constructor in
-    // SuggestionAnswer::copy and the assigment operator as values in vector.
+    // SuggestionAnswer::copy and the assignment operator as values in vector.
   };
 
   class ImageLine {
@@ -138,7 +138,7 @@ class SuggestionAnswer {
   };
 
   SuggestionAnswer();
-  SuggestionAnswer(const SuggestionAnswer& answer) = default;
+  SuggestionAnswer(const SuggestionAnswer& answer);
   ~SuggestionAnswer();
 
   // Parses |answer_json| and returns a SuggestionAnswer containing the
@@ -155,6 +155,7 @@ class SuggestionAnswer {
     return base::WrapUnique(source ? new SuggestionAnswer(*source) : nullptr);
   }
 
+  const GURL& image_url() const { return image_url_; }
   const ImageLine& first_line() const { return first_line_; }
   const ImageLine& second_line() const { return second_line_; }
 
@@ -176,6 +177,7 @@ class SuggestionAnswer {
   // Forbid assignment.
   SuggestionAnswer& operator=(const SuggestionAnswer&);
 
+  GURL image_url_;
   ImageLine first_line_;
   ImageLine second_line_;
   int type_;

components/omnibox/browser/suggestion_answer_unittest.cc

@@ -9,7 +9,9 @@
 
 #include "base/json/json_reader.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/values.h"
+#include "components/omnibox/browser/omnibox_field_trial.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace {
@@ -261,6 +263,23 @@ TEST(SuggestionAnswerTest, AddImageURLsTo) {
   answer->AddImageURLsTo(&urls);
   ASSERT_EQ(0U, urls.size());
 
+  {
+    base::test::ScopedFeatureList feature_list;
+    feature_list.InitAndEnableFeature(omnibox::kOmniboxNewAnswerLayout);
+    json =
+        "{ \"i\": { \"d\": \"https://gstatic.com/foo.png\", \"t\": 3 },"
+        "  \"l\" : ["
+        "    { \"il\": { \"t\": [{ \"t\": \"some text\", \"tt\": 5 }] } },"
+        "    { \"il\": { \"t\": [{ \"t\": \"other text\", \"tt\": 8 }] } }"
+        "  ]}";
+    answer = ParseAnswer(json);
+    ASSERT_TRUE(answer);
+    answer->AddImageURLsTo(&urls);
+    ASSERT_EQ(1U, urls.size());
+    EXPECT_EQ(GURL("https://gstatic.com/foo.png"), urls[0]);
+    urls.clear();
+  }
+
   json =
       "{ \"l\" : ["
       "  { \"il\": { \"t\": [{ \"t\": \"some text\", \"tt\": 5 }] } },"
@@ -271,6 +290,21 @@ TEST(SuggestionAnswerTest, AddImageURLsTo) {
   answer->AddImageURLsTo(&urls);
   ASSERT_EQ(1U, urls.size());
   EXPECT_EQ(GURL("https://gstatic.com/foo.png"), urls[0]);
+  urls.clear();
+
+  json =
+      "{ \"i\": { \"d\": \"https://gstatic.com/foo.png\", \"t\": 3 },"
+      "  \"l\" : ["
+      "    { \"il\": { \"t\": [{ \"t\": \"some text\", \"tt\": 5 }] } },"
+      "    { \"il\": { \"t\": [{ \"t\": \"other text\", \"tt\": 8 }],"
+      "              \"i\": { \"d\": \"//gstatic.com/bar.png\", \"t\": 3 }}}"
+      "  ]}";
+  answer = ParseAnswer(json);
+  ASSERT_TRUE(answer);
+  answer->AddImageURLsTo(&urls);
+  ASSERT_EQ(1U, urls.size());
+  EXPECT_EQ(GURL("https://gstatic.com/bar.png"), urls[0]);
+  urls.clear();
 
   json =
       "{ \"l\" : ["
@@ -281,7 +315,7 @@ TEST(SuggestionAnswerTest, AddImageURLsTo) {
   answer = ParseAnswer(json);
   ASSERT_TRUE(answer);
   answer->AddImageURLsTo(&urls);
-  ASSERT_EQ(3U, urls.size());
-  EXPECT_EQ(GURL("https://gstatic.com/foo.png"), urls[1]);
-  EXPECT_EQ(GURL("https://gstatic.com/bar.jpg"), urls[2]);
+  ASSERT_EQ(1U, urls.size());
+  // Note: first_line_.image_url() is not used in practice (so it's ignored).
+  EXPECT_EQ(GURL("https://gstatic.com/bar.jpg"), urls[0]);
 }