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 "base/memory/scoped_ptr.h"
+#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:
+    TextField();
+    ~TextField();
+
+    static bool ParseTextField(
+        const base::DictionaryValue* field_json, TextField* text_field);
+
+    const std::string& text() const { return text_; }
+    int type() const { return type_; }
+
+    bool Equals(const TextField& field) const;
+
+   private:
+    std::string text_;
+    int type_;
+  };

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

You still need all of these classes to be copyable, right?  Perhaps an addition
to the comment above noting that all the classes need to be copyable and why
might be good?

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

Done.

+
+  typedef std::vector<TextField> TextFields;
+
+  class ImageLine {
+   public:
+    ImageLine();
+    ImageLine(const ImageLine& line);

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

Nit: Explicit

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

Done.

+    ~ImageLine();
+
+    static bool ParseImageLine(
+        const base::DictionaryValue* line_json, ImageLine* image_line);
+
+    const TextFields& text_fields() const { return text_fields_; }
+    const TextField* additional_text() const { return additional_text_.get(); }
+    const TextField* status_text() const { return status_text_.get(); }
+    const GURL& image_url() const { return image_url_; }
+
+    bool Equals(const ImageLine& line) const;
+
+   private:
+    TextFields text_fields_;
+    scoped_ptr<TextField> additional_text_;
+    scoped_ptr<TextField> status_text_;
+    GURL image_url_;
+  };
+
+  SuggestionAnswer();
+  SuggestionAnswer(const SuggestionAnswer& answer);
+  ~SuggestionAnswer();
+
+  // Parses |answer_json| and populates |answer| with the contents if and only
+  // if the data is well formed and populated with all required elements.
+  static bool ParseAnswer(
+        const std::string& answer_json, scoped_ptr<SuggestionAnswer>* answer);

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

Is this wrapper just a test-only helper?  If so, I think it should be defined in
the test file instead of here.

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

Done.

+  static bool ParseAnswer(
+        const base::DictionaryValue* answer_json,
+        scoped_ptr<SuggestionAnswer>* answer);
+
+  static scoped_ptr<SuggestionAnswer> Copy(const SuggestionAnswer* source) {

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

Is it worth noting a TODO that we could eliminate this sort of thing if callers
could use something like std::optional<T>?  (It sounds from the thread like
that's likely to move forward.)

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

Done.

+    return make_scoped_ptr(source ? new SuggestionAnswer(*source) : nullptr);

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

Nit: Either call this copy(), or don't inline it.

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

Done.

+  }
+
+  const ImageLine& first_line() const { return first_line_; }
+  const ImageLine& second_line() const { return second_line_; }
+
+  // Answer type accessors.  Valid types are non-negative and defined at
+  // https://goto.google.com/visual_element_configuration.
+  int type() const { return type_; }

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

Should we have an enum that we keep synced to said document?  Or is the type
basically an opaque value as far as Chrome code is concerned?

[groby-ooo-7-16, 2014/10/30 16:21:14]

It should be opaque to Chrome.  We _might_ need to surface some of the types at
some point, but maintaining a full list is not necessary.

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

Acknowledged.

+  void set_type(int type) { type_ = type; }
+
+  bool Equals(const SuggestionAnswer& answer) const;
+
+  typedef std::vector<GURL> URLs;

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

Nit: typedefs and enums go near the top of the section (in this case, I'd put
both this class' typedefs just below the member class declarations).

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

Without forward declaration, TextFields has to go between TextField and
ImageLine.  I forward declared TextField and put the typedefs at the very top. 
Other suggestions are welcome.

+
+  // Gets any URLs of any images in Answers results.  Any existing URLs in
+  // |urls| will be removed.
+  void GetImageURLs(URLs* urls) const;
+
+ private:
+  ImageLine first_line_;
+  ImageLine second_line_;
+  int type_;
+};
+
+#endif  // COMPONENTS_OMNIBOX_SUGGESTION_ANSWER_H_