Add a class to parse answer json.

components/omnibox/suggestion_answer.cc

Index: components/omnibox/suggestion_answer.cc
diff --git a/components/omnibox/suggestion_answer.cc b/components/omnibox/suggestion_answer.cc
new file mode 100644
index 0000000000000000000000000000000000000000..9f9bcb15c8a44c8183dc1c3a52891843b85517bc
--- /dev/null
+++ b/components/omnibox/suggestion_answer.cc
@@ -0,0 +1,198 @@
+// Copyright 2014 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.
+
+#include "components/omnibox/suggestion_answer.h"
+
+#include "base/json/json_reader.h"
+#include "base/strings/string_util.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/values.h"
+#include "url/url_constants.h"
+
+namespace {
+
+// All of these are defined here (even though they're only used once each) so

[Peter Kasting, 2014/10/30 03:43:29]

Nit: Maybe "even if they're used sparingly below" or similar since not all these
are only used once (e.g. kAnswerJsonText).

[Justin Donnelly, 2014/10/30 19:23:13]

Done.

+// the format details are easy to locate and update or compare to the spec doc.
+static const char* kAnswerJsonLines = "l";
+static const char* kAnswerJsonImageLine = "il";
+static const char* kAnswerJsonText = "t";
+static const char* kAnswerJsonAdditionalText = "at";
+static const char* kAnswerJsonStatusText = "st";
+static const char* kAnswerJsonTextType = "tt";
+static const char* kAnswerJsonImage = "i";
+static const char* kAnswerJsonImageData = "i.d";
+
+}  // namespace
+
+// SuggestionAnswer::TextField -------------------------------------------------
+
+SuggestionAnswer::TextField::TextField() : type_(-1) {}
+SuggestionAnswer::TextField::~TextField() {}
+
+// static
+bool SuggestionAnswer::TextField::ParseTextField(
+    const base::DictionaryValue* field_json, TextField* text_field) {
+  return field_json->GetString(kAnswerJsonText, &text_field->text_) &&
+      !text_field->text_.empty() &&
+      field_json->GetInteger(kAnswerJsonTextType, &text_field->type_);
+}
+
+bool SuggestionAnswer::TextField::Equals(const TextField& field) const {
+  return type_ == field.type_ && text_ == field.text_;
+}
+
+// SuggestionAnswer::ImageLine -------------------------------------------------
+
+SuggestionAnswer::ImageLine::ImageLine() {}
+SuggestionAnswer::ImageLine::ImageLine(const ImageLine& line)
+    : text_fields_(line.text_fields_),
+      additional_text_(line.additional_text_.get() ?

[Peter Kasting, 2014/10/30 03:43:29]

Nit: No get() on scoped_ptrs used as bools (6 places)

[Justin Donnelly, 2014/10/30 19:23:14]

Done.

+                       new TextField(*line.additional_text_) : nullptr),
+      status_text_(line.status_text_.get() ?
+                   new TextField(*line.status_text_) : nullptr) {}
+
+SuggestionAnswer::ImageLine::~ImageLine() {}
+
+// static
+bool SuggestionAnswer::ImageLine::ParseImageLine(
+    const base::DictionaryValue* line_json, ImageLine* image_line) {
+  const base::DictionaryValue* inner_json;
+  if (!line_json->GetDictionary(kAnswerJsonImageLine, &inner_json))
+    return false;
+
+  const base::ListValue* fields_json;
+  if (!inner_json->GetList(kAnswerJsonText, &fields_json) ||
+      fields_json->GetSize() == 0)
+    return false;
+
+  for (size_t i = 0; i < fields_json->GetSize(); ++i) {
+    const base::DictionaryValue* field_json;
+    TextField text_field;
+    if (!fields_json->GetDictionary(i, &field_json) ||
+        !TextField::ParseTextField(field_json, &text_field))
+      return false;
+    image_line->text_fields_.push_back(text_field);
+  }
+
+  if (inner_json->HasKey(kAnswerJsonAdditionalText)) {
+    image_line->additional_text_.reset(new TextField());
+    const base::DictionaryValue* field_json;
+    if (!inner_json->GetDictionary(kAnswerJsonAdditionalText, &field_json) ||
+        !TextField::ParseTextField(field_json,
+                                   image_line->additional_text_.get()))
+      return false;
+  }
+
+  if (inner_json->HasKey(kAnswerJsonStatusText)) {
+    image_line->status_text_.reset(new TextField());
+    const base::DictionaryValue* field_json;
+    if (!inner_json->GetDictionary(kAnswerJsonStatusText, &field_json) ||
+        !TextField::ParseTextField(field_json, image_line->status_text_.get()))
+      return false;
+  }
+
+  if (inner_json->HasKey(kAnswerJsonImage)) {
+    base::string16 url_string;
+    if (!inner_json->GetString(kAnswerJsonImageData, &url_string) ||
+        url_string.empty())
+      return false;
+    // If necessary, concatenate scheme and host/path using only ':' as
+    // separator. This is due to the results delivering strings of the form
+    // "//host/path", which is web-speak for "use the enclosing page's scheme",
+    // but not a valid path of an URL.  The GWS frontend commonly (always?)
+    // redirects to HTTPS so we just default to that here.
+    image_line->image_url_ = GURL(
+        StartsWith(url_string, base::ASCIIToUTF16("//"), false) ?
+            (base::ASCIIToUTF16(url::kHttpsScheme) + base::ASCIIToUTF16(":") +
+                url_string) :
+            url_string);
+
+    if (!image_line->image_url_.is_valid())
+      return false;
+  }
+
+  return true;
+}
+
+bool SuggestionAnswer::ImageLine::Equals(const ImageLine& line) const {
+  if (text_fields_.size() != line.text_fields_.size())
+    return false;
+  for (size_t i = 0; i < text_fields_.size(); ++i) {
+    if (!text_fields_[i].Equals(line.text_fields_[i]))
+      return false;
+  }
+
+  if (additional_text_.get()) {

[Peter Kasting, 2014/10/30 03:43:29]

This conditional isn't quite correct, because it will succeed if
(!additional_text_ && line.additional_text_) (the outer conditional will fail
and we won't check |line|).  Same with the next conditional block.

(You should probably ensure your tests cover this.)

[Justin Donnelly, 2014/10/30 19:23:14]

Done and done.

+    if (!line.additional_text_.get() ||
+        additional_text_->Equals(*line.additional_text_))
+      return false;
+  }
+
+  if (status_text_.get()) {
+    if (!line.status_text_.get() || status_text_->Equals(*line.status_text_))
+      return false;
+  }
+
+  return image_url_ == line.image_url_;
+}
+
+// SuggestionAnswer ------------------------------------------------------------
+
+SuggestionAnswer::SuggestionAnswer() : type_(-1) {}
+SuggestionAnswer::SuggestionAnswer(const SuggestionAnswer& answer)
+    : first_line_(answer.first_line_),
+      second_line_(answer.second_line_),
+      type_(answer.type_) {}
+
+SuggestionAnswer::~SuggestionAnswer() {}
+
+// static
+bool SuggestionAnswer::ParseAnswer(

[Peter Kasting, 2014/10/30 03:43:29]

Hmm.  I think it would be slightly nicer if this function and the
similarly-named one below returned a scoped_ptr<SuggestionAnswer> directly
instead of having a bool and an outparam.

The one non-test caller could accommodate this with only a slight code change
(splitting the large conditional that ends by calling ParseAnswer() into two
pieces), and it would make some of the test code simpler, e.g.:

  scoped_ptr<SuggestionAnswer> answer;
  ASSERT_FALSE(SuggestionAnswer::ParseAnswer(json, &answer));

->

  ASSERT_FALSE(SuggestionAnswer::ParseAnswer(json));

Plus this reads a little more clearly to me -- returning a pointer makes it
obvious that NULL means failure, whereas with a bool one might wonder if the
function could return true/false in different cases than the outparam was set to
null/non-null.

[Justin Donnelly, 2014/10/30 19:23:13]

Done.

I agree that this is better.  To be honest, designing and building this class
and figuring out how to properly handle these scoped_ptrs has caused me some
pretty serious brain aches.  But now that I feel like I'm starting to get a
handle on it I'm excited by the idea that one can build C++ that is both
reliable and properly OO.  And that's the first time I've ever used the words
"excited" and "C++" in the same sentence.

[groby-ooo-7-16, 2014/10/30 20:28:29]

Uh-oh. Please step back from the computer - excitement and C++ don't work well
together ;)

+    const std::string& answer_json, scoped_ptr<SuggestionAnswer>* answer) {
+  scoped_ptr<base::Value> value(base::JSONReader::Read(answer_json));
+  base::DictionaryValue* dict;
+  if (!value.get() || !value->GetAsDictionary(&dict))
+    return false;
+
+  return ParseAnswer(dict, answer);
+}
+
+// static
+bool SuggestionAnswer::ParseAnswer(
+    const base::DictionaryValue* answer_json,
+    scoped_ptr<SuggestionAnswer>* answer) {
+  scoped_ptr<SuggestionAnswer> result(new SuggestionAnswer());
+
+  const base::ListValue* lines_json;
+  if (!answer_json->GetList(kAnswerJsonLines, &lines_json) ||
+      lines_json->GetSize() != 2)
+    return false;
+
+  const base::DictionaryValue* first_line_json;
+  if (!lines_json->GetDictionary(0, &first_line_json) ||
+      !ImageLine::ParseImageLine(first_line_json, &result->first_line_))
+    return false;
+
+  const base::DictionaryValue* second_line_json;
+  if (!lines_json->GetDictionary(1, &second_line_json) ||
+      !ImageLine::ParseImageLine(second_line_json, &result->second_line_))
+    return false;
+
+  (*answer) = result.Pass();
+  return true;
+}
+
+bool SuggestionAnswer::Equals(const SuggestionAnswer& answer) const {
+  return type_ == answer.type_ &&
+         first_line_.Equals(answer.first_line_) &&
+         second_line_.Equals(answer.second_line_);
+}
+
+void SuggestionAnswer::GetImageURLs(std::vector<GURL>* urls) const {
+  urls->clear();
+  if (first_line_.image_url().is_valid())
+    urls->push_back(first_line_.image_url());
+  if (second_line_.image_url().is_valid())
+    urls->push_back(second_line_.image_url());
+}