Add a class to parse answer json.

components/omnibox/suggestion_answer.h

+// 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.
+
+#ifndef COMPONENTS_OMNIBOX_SUGGESTION_ANSWER_H_
+#define COMPONENTS_OMNIBOX_SUGGESTION_ANSWER_H_
+
+#include <string>
+#include <vector>
+
+#include "url/gurl.h"
+
+namespace base {
+class DictionaryValue;
+}
+
+// Structured representation of the JSON payload of a suggestion with an answer.
+// An answer has exactly two image lines, so called because they are a
+// combination of text and an optional image URL.  Each image line has 1 or more
+// text fields, each of which is required to contain a string and an integer
+// type.  The text fields are contained in a non-empty vector and two optional
+// named properties, referred to as "additional text" and "status text".
+//
+// When represented in the UI, these elements should be styled and laid out
+// according to the specification at https://goto.google.com/ais_api.
+class SuggestionAnswer {
+ public:
+  class TextField {
+   public:
+    // Constructs an empty, invalid field.
+    TextField();
+    ~TextField();
+
+    static void ParseTextField(
+        const base::DictionaryValue* field_json, TextField* text_field);
+
+    bool operator==(const TextField& field) const;

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

Per the style guide, avoid defining == and != for these classes unless you
really need to.  Instead use a function like Equals().  (3 places)

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

Done.

+    bool operator!=(const TextField& field) const;
+
+    bool is_valid() const { return !text_.empty() && type_ >= 0; }
+
+    const std::string& text() const { return text_; }
+    int type() const { return type_; }
+
+    // Removes all data. After this call, the field will be invalid.
+    void Clear();
+
+   private:
+    std::string text_;
+    int type_;
+  };

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

I assume you can't use DISALLOW_COPY_AND_ASSIGN here since you want these in a
vector.  Consider instead having a comment that notes that we allow the compiler
to auto-generate copy/assignment operators and why.

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

It's because copies of answers are made in AutocompleteMatch's copy and
assignment operators and some logic in BaseSearchProvider.  Added a comment
below in SuggestionAnswer.  Let me know if you think it's worth repeating in
each of the inner classes.

+
+  typedef std::vector<TextField> TextFields;
+
+  class ImageLine {
+   public:
+    // Constructs an empty, invalid line.
+    ImageLine();
+    ~ImageLine();
+
+    static void ParseImageLine(
+        const base::DictionaryValue* line_json, ImageLine* image_line);
+
+    bool operator==(const ImageLine& line) const;
+    bool operator!=(const ImageLine& line) const;
+
+    // A line is only valid if the JSON representation was successfully parsed
+    // and all expected constraints were met (primarily, that there was at least
+    // one text field in the repeated text field property).
+    bool is_valid() const { return is_valid_; }
+
+    const std::vector<TextField>& text_fields() const { return text_fields_; }

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

Nit: Use TextFields

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

Done.

+
+    bool has_additional_text() const { return additional_text_.is_valid(); }

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

Nit: I'm a bit on the fence as to whether these has_xxx() methods are really
useful, given that it's almost as short for callers to just call
xxx().is_valid() themselves, and these first two at least are only used in
unittests.  I tend to like shrinking public APIs as much as possible.  That
said, I don't feel strongly, so do whatever you think is most maintainable in
the end.

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

Fair question, but there's a Java API with Has methods that will also map onto
this so I think it makes sense.

+    const TextField& additional_text() const { return additional_text_; }
+
+    bool has_status_text() const { return status_text_.is_valid(); }
+    const TextField& status_text() const { return status_text_; }
+
+    bool has_image_url() const { return image_url_.is_valid(); }
+    const GURL& image_url() const { return image_url_; }
+
+    // Removes all data. After this call, the line will be invalid.
+    void Clear();
+
+   private:
+    TextFields text_fields_;
+    TextField additional_text_;
+    TextField status_text_;
+    GURL image_url_;
+    bool is_valid_;
+  };

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

Nit: DISALLOW_COPY_AND_ASSIGN (2 places)

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

See above comment re: answer copying.

+
+  // Constructs an empty, invalid answer.
+  SuggestionAnswer();
+  ~SuggestionAnswer();
+
+  static void ParseAnswer(
+        const std::string& answer_json, SuggestionAnswer* answer);
+  static void ParseAnswer(
+        const base::DictionaryValue* answer_json, SuggestionAnswer* answer);
+
+  bool operator==(const SuggestionAnswer& answer) const;
+  bool operator!=(const SuggestionAnswer& answer) const;
+
+  // An answer is valid only if both a type has been set and the JSON

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

Nit: Eliminate "both", add commas (since list seems to really have three items)

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

Done.

+  // representation was successfully parsed and all expected constraints were
+  // met (primarily that there were exactly two properly formed image lines).
+  bool is_valid() const {
+    return type_ >= 0 && first_line_.is_valid() && second_line_.is_valid();
+  }
+
+  const ImageLine& first_line() const { return first_line_; }
+  const ImageLine& second_line() const { return second_line_; }
+  int type() const { return type_; }
+
+  // Specifies the type of this answer.  The string will be interpreted as a
+  // base10 integer.  If it can't be parsed as such, the type will be set to -1.
+  // Valid types must be non-negatiave and are defined in
+  // https://goto.google.com/visual_element_configuration.
+  void SetType(const base::string16& type);

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

All but one caller of this has a number to begin with.  It would be simpler to
push string parsing into the lone client of this that needs to do it and make
this take a number, so this API wasn't as easy to misuse (e.g. by code that
accidentally passes the wrong string field).

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

Done.

+
+  // Removes all data. After this call, the answer will be invalid.
+  void Clear();
+
+  // Gets any URLs of any images in Answers results.  Any existing URLs in
+  // |urls| will be removed.
+  void GetImageURLs(std::vector<GURL>* urls) const;

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

Nit: I suggest making a typedef for this

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

Seems to me that in the client classes (SearchSuggestionParser and
BaseSearchProvider) it would be no terser and slightly less clear to refer to
SuggestionAnswer::GURLs rather than vector<GURL>.  But if we don't use it
outside this class then it's just used in one place here.  Let me know if that's
what you had in mind or if you had some other motivation here.

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

Terseness isn't the goal; the goal is to insulate the caller from the actual
type in use.  Basically this is a way to purposefully black-box the type so the
caller doesn't know or care whether it's a vector, list, or whatever.

Also I have an allergy to angle brackets so I'll basically always take any
solution other than container<type>.  :)

It's not really a big deal in the end.  Do what you think is best.

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

Interesting.  I didn't actually know that that was the rationale.  Given how
prevalent the pattern of typedef-ing containers is, I guess it'd be better for
consistency alone.  Done.

+
+ private:
+  ImageLine first_line_;
+  ImageLine second_line_;
+  int type_;
+};
+
+#endif  // COMPONENTS_OMNIBOX_SUGGESTION_ANSWER_H_