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..3d545e166f872e49fa9b535a73885d943ce36a79
--- /dev/null
+++ b/components/omnibox/suggestion_answer.cc
@@ -0,0 +1,209 @@
+// 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";

[Peter Kasting, 2014/10/24 22:38:10]

Nit: I tend to interpret the style guide's "Place a function's variables in the
narrowest scope possible" as also applying to constants that are local to one
function or block (as all but one of these are).

[Justin Donnelly, 2014/10/27 21:41:15]

Makes sense, but these are a bit of a special case.  I wouldn't normally use
constants at all if their value was only used in one place (and this is not
likely to ever change).  The purpose of these is to put the format details
together so that if they're updated or added to it's easy to locate and compare
to the spec doc.

I wouldn't strongly object to just eliminating the constants if you think they
don't make sense here.

[Peter Kasting, 2014/10/27 21:50:09]

I don't care much.  If you think there's a win in placing them here like you
describe, I would suggest writing a comment above these noting the rationale you
detail here.

[Justin Donnelly, 2014/10/28 00:39:34]

Done.

+
+}
+
+// SuggestionAnswer::TextField -------------------------------------------------
+
+SuggestionAnswer::TextField::TextField() : type_(-1) {}
+SuggestionAnswer::TextField::~TextField() {}
+
+// static
+void SuggestionAnswer::TextField::ParseTextField(

[Peter Kasting, 2014/10/24 22:38:10]

This function returns void and the caller checks validity afterwards.

It seems like the API might be cleaner if this function checked validity
internally, returned a bool status, and guaranteed not to change its argument's
contents in the case of an invalid result.  (That last might be optional.)

Similar comments apply through the rest of this file.

[Justin Donnelly, 2014/10/27 21:41:15]

The issue here is that the is_valid() functions are necessary since clients pass
the answers by value.  I originally had bool return values from the Parse
functions but Rachel objected (understandably) to having both a returned bool
and is_valid logic that did the same thing.  Thoughts?

[Peter Kasting, 2014/10/27 21:50:09]

Would passing by pointer muck things up too much?
I probably prefer to have both if it actually makes the callers simpler or more
readable.  If it's not actually any better, then I wouldn't bother.

[Justin Donnelly, 2014/10/28 00:39:34]

Yeah, it creates a lot of slightly more complicated code.  You can actually see
what it looks like in patchset 1 of this CL.  A bunch of new scoped_ptrs are
introduced, plus SuggestResult has to get a custom copy constructor and
assignment operator.
It makes the caller code slightly simpler, but it's a bit of a matter of taste. 
Given Rachel's slightly (I think) stronger preference that yours, I'll leave it
as-is for now unless there's anything she wants to add.

[Peter Kasting, 2014/10/29 17:56:17]

OK, I read through patch set 1 and looked at the delta between it and patch set
2.

I actually think the original design of having this field be optional is more
sensible.  The code delta is pretty minimal, mainly just the new copy/assignment
operators you defined.  There are also multiple places that do something faintly
ugly like:

  answer.reset(other_answer ? new SuggestionAnswer(other_answer) : nullptr);

However, these issues can be worked around, and if we leave this field optional,
we can get rid of the validity functions entirely, and then we have no excuse
not to also clean up the parsing functions here to either return a possibly-null
pointer for failure or to return a bool.  I think the net gain from that cleanup
outweighs the net increased complexity of making the answer optional, especially
since in both cases we're making the code read more like how one should actually
think of it working (i.e. that answers are optional and parsing can fail,
instead of hiding the idea of a non-existent answer in a validity-check
function).

There are a couple ways to make the copying code a little nicer.  One might be
to do something like this:

  class SuggestionAnswer {
    static scoped_ptr<SuggestionAnswer> Copy(SuggestionAnswer* source) {
      return make_scoped_ptr(source ? new SuggestionAnswer(source) : nullptr);
    }
  };

  ...
  answer = SuggestionAnswer::Copy(other_answer);

Another way would be to do something similar, except make Copy() take both
answers as args ("SuggestionAnswer::Copy(other_answer, &answer);").  Another way
would be to create a wrapper class like OptionalAnswer that's simply a
scoped_ptr<SuggestionAnswer> plus copy constructor, assignment operator, and a
few other utility functions; this lets you write something like

  answer = other_answer;

...and avoids the need to define custom copy/assignment code for SuggestResult,
but one might argue that at that point we've lost the simplicity and readability
that I argued above made this path worth taking to begin with, and that the end
code isn't different from just putting the "optional"-ness inside
SuggestionAnswer.

I realize the idea of "no, go back and change everything back again" is going to
be pretty unpalatable.  What do you think of my arguments above?  Have I missed
good reasons to prefer one side over the other?

+    const base::DictionaryValue* field_json, TextField* text_field) {
+  if (!field_json->GetString(kAnswerJsonText, &text_field->text_) ||
+      text_field->text_.empty())
+    return;
+  field_json->GetInteger(kAnswerJsonTextType, &text_field->type_);
+}
+
+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
+void SuggestionAnswer::ImageLine::ParseImageLine(
+    const base::DictionaryValue* line_json, ImageLine* image_line) {
+  image_line->is_valid_ = false;
+
+  const base::DictionaryValue* inner_json;
+  if (!line_json->GetDictionary(kAnswerJsonImageLine, &inner_json))
+    return;
+
+  const base::ListValue* fields_json;
+  if (!inner_json->GetList(kAnswerJsonText, &fields_json) ||
+      fields_json->GetSize() == 0)
+    return;
+
+  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))
+      return;
+    TextField::ParseTextField(field_json, &text_field);
+    if (!text_field.is_valid())
+      return;
+    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))
+      return;
+    TextField::ParseTextField(field_json, &image_line->additional_text_);
+    if (!image_line->additional_text_.is_valid())
+      return;
+  }
+
+  if (inner_json->HasKey(kAnswerJsonStatusText)) {
+    const base::DictionaryValue* field_json;
+    if (!inner_json->GetDictionary(kAnswerJsonStatusText, &field_json))
+      return;
+    TextField::ParseTextField(field_json, &image_line->status_text_);
+    if (!image_line->status_text_.is_valid())
+      return;
+  }
+
+  if (inner_json->HasKey(kAnswerJsonImage)) {
+    base::string16 url_string;
+    if (!inner_json->GetString(kAnswerJsonImageData, &url_string) ||
+        url_string.empty())
+      return;
+    // 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);

[Peter Kasting, 2014/10/24 22:38:10]

Nit: Maybe we should use kHttpsScheme from url/url_constants.h.

You might also want to add to the comment above noting why "//" in this case is
always properly expanded to "https" (instead of having to go fetch some context
or something to decide).

[Justin Donnelly, 2014/10/27 21:41:15]

Done.

+    else
+      image_line->image_url_ = GURL(url_string);

[Peter Kasting, 2014/10/24 22:38:10]

Nit: Shorter:

    image_line->image_url_ = GURL(
        StartsWith(url_string, base::ASCIIToUTF16("//"), false) ?
            (base::ASCIIToUTF16("https:") + url_string) : url_string);

[Justin Donnelly, 2014/10/27 21:41:15]

Done.

+
+    if (!image_line->image_url_.is_valid())
+      return;
+  }
+
+  image_line->is_valid_ = true;
+}
+
+bool SuggestionAnswer::ImageLine::operator==(const ImageLine& line) const {
+  return text_fields_ == line.text_fields_ &&
+         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
+void 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;
+
+  ParseAnswer(dict, answer);
+}
+
+// static
+void 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;
+
+  const base::DictionaryValue* first_line_json;
+  if (!lines_json->GetDictionary(0, &first_line_json))
+    return;
+  ImageLine::ParseImageLine(first_line_json, &answer->first_line_);
+  if (!answer->first_line_.is_valid())
+    return;
+
+  const base::DictionaryValue* second_line_json;
+  if (!lines_json->GetDictionary(1, &second_line_json))
+    return;
+  ImageLine::ParseImageLine(second_line_json, &answer->second_line_);
+}
+
+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 base::string16& type) {
+  if (!base::StringToInt(type, &type_))
+    type_ = -1;
+}
+
+void SuggestionAnswer::Clear() {
+  first_line_.Clear();
+  second_line_.Clear();
+  type_ = -1;
+}
+
+void SuggestionAnswer::GetImageURLs(std::vector<GURL>* urls) const {
+  urls->clear();
+
+  if(!is_valid())
+    return;
+
+  if (first_line_.has_image_url())
+    urls->push_back(first_line_.image_url());
+  if (second_line_.has_image_url())
+    urls->push_back(second_line_.image_url());
+}