Add a class to parse answer json.

components/omnibox/suggestion_answer.cc

+// 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_number_conversions.h"
+#include "base/strings/string_util.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/values.h"
+
+namespace {
+
+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";
+
+}
+
+// SuggestionAnswer::TextField -------------------------------------------------
+
+SuggestionAnswer::TextField::TextField() : type_(-1) {}
+SuggestionAnswer::TextField::~TextField() {}
+
+// static
+bool SuggestionAnswer::TextField::ParseTextField(
+    const base::DictionaryValue* field_json, TextField* text_field) {
+  if (!field_json->GetString(kAnswerJsonText, &text_field->text_) ||
+      text_field->text_.empty())
+    return false;
+  if (!field_json->GetInteger(kAnswerJsonTextType, &text_field->type_))
+    return false;
+  return true;
+}
+
+bool SuggestionAnswer::TextField::operator==(const TextField& field) const {
+  return type_ == field.type_ && text_ == field.text_;
+}
+
+bool SuggestionAnswer::TextField::operator!=(const TextField& field) const {
+  return !operator==(field);
+}
+
+void SuggestionAnswer::TextField::Clear() {
+  text_.clear();
+  type_ = -1;
+}
+
+// SuggestionAnswer::ImageLine -------------------------------------------------
+
+SuggestionAnswer::ImageLine::ImageLine() : is_valid_(false) {}
+SuggestionAnswer::ImageLine::~ImageLine() {}
+
+// static
+bool SuggestionAnswer::ImageLine::ParseImageLine(
+    const base::DictionaryValue* line_json, ImageLine* image_line) {

[groby-ooo-7-16, 2014/10/23 00:46:19]

Why not just return ImageLine by value? (for all ParseXYZ funcs)

Less parameters, less pointers.

[Justin Donnelly, 2014/10/23 17:59:37]

Fair question.  Again, just because of the ubiquity of this pattern.  Same
question applies to your GetAnswersImageURLs, right?  That could (and maybe
should) just return a vector of strings by value (we know it's only 0, 1, or 2
URLs).  Again, I'm definitely open to what you're suggesting.  Maybe we should
discuss offline.

[groby-ooo-7-16, 2014/10/23 21:59:54]

It's ubiquitous and ugly :) You're right, GetAnswersImageURLs could probably do
the same thing. 

The reason I specifically picked on this function is that it returns an object
that can be both valid or invalid, _and_ a status that expresses the same thing.

[Justin Donnelly, 2014/10/24 14:58:08]

I've left this as-is for now since they "two different things that express the
same thing" problem is gone.  Let me know if you still want to consider
returning by value.

+  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)) {
+    const base::DictionaryValue* field_json;
+    if (!inner_json->GetDictionary(kAnswerJsonAdditionalText, &field_json) ||
+        !TextField::ParseTextField(field_json, &image_line->additional_text_))
+      return false;
+  }
+
+  if (inner_json->HasKey(kAnswerJsonStatusText)) {
+    const base::DictionaryValue* field_json;
+    if (!inner_json->GetDictionary(kAnswerJsonStatusText, &field_json) ||
+        !TextField::ParseTextField(field_json, &image_line->status_text_))
+      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.
+    if (StartsWith(url_string, base::ASCIIToUTF16("//"), false))
+      image_line->image_url_ = GURL(base::ASCIIToUTF16("https:") + url_string);
+    else
+      image_line->image_url_ = GURL(url_string);
+
+    if (!image_line->image_url_.is_valid())

[groby-ooo-7-16, 2014/10/23 00:46:19]

I've been thinking of adding UMA metrics for parse failures. I'd rather find out
about malformed replies before crbugs are filed :)

Thoughts?

[Justin Donnelly, 2014/10/23 17:59:37]

Given how you mentioned that Finch team is about to roll out alerts on UMA
metrics, this is a great idea!  But you mind if I add it in a subsequent CL? 
I'd like to keep the size of this one under control.

+      return false;
+  }
+
+  image_line->is_valid_ = true;

[groby-ooo-7-16, 2014/10/23 00:46:19]

Probably want to force that to invalid at the top. Or assert it's passed in
invalid

[Justin Donnelly, 2014/10/23 17:59:37]

Done.

+
+  return true;
+}
+
+bool SuggestionAnswer::ImageLine::operator==(const ImageLine& line) const {
+  if (text_fields_.size() != line.text_fields_.size())

[groby-ooo-7-16, 2014/10/23 00:46:19]

std::vector has equality comparison, so you should be able to use that.

[Justin Donnelly, 2014/10/23 17:59:37]

Done.

+    return false;
+  for (size_t i = 0; i < text_fields_.size(); ++i) {
+    if (text_fields_[i] != line.text_fields_[i])
+      return false;
+  }
+  return additional_text_ == line.additional_text_ &&
+         status_text_ == line.status_text_ &&
+         image_url_ == line.image_url_;
+}
+
+bool SuggestionAnswer::ImageLine::operator!=(const ImageLine& line) const {
+  return !operator==(line);
+}
+
+void SuggestionAnswer::ImageLine::Clear() {
+  text_fields_.clear();
+  additional_text_.Clear();
+  status_text_.Clear();
+  image_url_ = GURL();
+  is_valid_ = false;
+}
+
+// SuggestionAnswer ------------------------------------------------------------
+
+SuggestionAnswer::SuggestionAnswer() : type_(-1) {}
+SuggestionAnswer::~SuggestionAnswer() {}
+
+// static
+bool SuggestionAnswer::ParseAnswer(
+    const std::string& answer_json, 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, SuggestionAnswer* answer) {
+  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, &answer->first_line_))
+    return false;
+  const base::DictionaryValue* second_line_json;
+  if (!lines_json->GetDictionary(1, &second_line_json) ||
+      !ImageLine::ParseImageLine(second_line_json, &answer->second_line_))
+    return false;
+
+  return true;
+}
+
+bool SuggestionAnswer::operator==(const SuggestionAnswer& answer) const {
+  return type_ == answer.type_ &&
+         first_line_ == answer.first_line_ &&
+         second_line_ == answer.second_line_;
+}
+
+bool SuggestionAnswer::operator!=(const SuggestionAnswer& answer) const {
+  return !operator==(answer);
+}
+
+void SuggestionAnswer::SetType(const std::string& type) {

[groby-ooo-7-16, 2014/10/23 00:46:19]

Do we really need string & string16 for this?

[Justin Donnelly, 2014/10/23 17:59:37]

Done.

+  if (!base::StringToInt(type, &type_))
+    type_ = -1;
+}
+
+void SuggestionAnswer::SetType(const base::string16& type) {
+  if (!base::StringToInt(type, &type_))
+    type_ = -1;
+}
+
+void SuggestionAnswer::Clear() {
+  first_line_.Clear();
+  second_line_.Clear();
+  type_ = -1;
+}