Add a class to parse answer json.

components/omnibox/search_suggestion_parser.cc

Index: components/omnibox/search_suggestion_parser.cc
diff --git a/components/omnibox/search_suggestion_parser.cc b/components/omnibox/search_suggestion_parser.cc
index 965662fd0e2cfb0f74453362c6bdf65bd48bb73c..996be303a3f4e0860c35e91a1dd86cb8039a0b4a 100644
--- a/components/omnibox/search_suggestion_parser.cc
+++ b/components/omnibox/search_suggestion_parser.cc
@@ -65,6 +65,7 @@ SearchSuggestionParser::SuggestResult::SuggestResult(
     const base::string16& annotation,
     const base::string16& answer_contents,
     const base::string16& answer_type,
+    const SuggestionAnswer& answer,
     const std::string& suggest_query_params,
     const std::string& deletion_url,
     bool from_keyword_provider,
@@ -83,6 +84,7 @@ SearchSuggestionParser::SuggestResult::SuggestResult(
       suggest_query_params_(suggest_query_params),
       answer_contents_(answer_contents),
       answer_type_(answer_type),
+      answer_(answer),
       should_prefetch_(should_prefetch) {
   match_contents_ = match_contents;
   DCHECK(!match_contents_.empty());
@@ -440,6 +442,7 @@ bool SearchSuggestionParser::ParseSuggestResults(
       base::string16 annotation;
       base::string16 answer_contents;
       base::string16 answer_type;
+      SuggestionAnswer answer;
       std::string suggest_query_params;
 
       if (suggestion_details) {
@@ -456,12 +459,19 @@ bool SearchSuggestionParser::ParseSuggestResults(
           // Extract Answers, if provided.
           const base::DictionaryValue* answer_json = NULL;
           if (suggestion_detail->GetDictionary("ansa", &answer_json)) {
-            match_type = AutocompleteMatchType::SEARCH_SUGGEST_ANSWER;
-            GetAnswersImageURLs(answer_json, &results->answers_image_urls);
-            std::string contents;
+            std::string contents, type;
             base::JSONWriter::Write(answer_json, &contents);
-            answer_contents = base::UTF8ToUTF16(contents);
-            suggestion_detail->GetString("ansb", &answer_type);
+            if (SuggestionAnswer::ParseAnswer(answer_json, &answer)) {
+              match_type = AutocompleteMatchType::SEARCH_SUGGEST_ANSWER;
+              answer_contents = base::UTF8ToUTF16(contents);
+              suggestion_detail->GetString("ansb", &answer_type);
+              answer.SetType(answer_type);
+              GetAnswersImageURLs(answer, &results->answers_image_urls);
+            } else {
+              answer.Clear();

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

Question: What if ParseAnswer() returned void, and error handling was contained
in there? (Yielding either a valid answer, or an empty one)

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

I considered that, but this is such a standard C++ pattern I decided against it.
But what I'm doing is definitely redundant so I'd be willing to drop the return
value if you think it's better.

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

Since the SuggestionAnswer object already needs to carry an is_valid function, I
think dropping the return value (and return by value) are a better choice. 

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

Done.

+              DLOG(ERROR) << "Invalid suggestion answer json: "
+                          << answer_contents;
+            }
           }
         }
       }
@@ -472,8 +482,8 @@ bool SearchSuggestionParser::ParseSuggestResults(
           base::CollapseWhitespace(suggestion, false), match_type,
           base::CollapseWhitespace(match_contents, false),
           match_contents_prefix, annotation, answer_contents, answer_type,
-          suggest_query_params, deletion_url, is_keyword_result, relevance,
-          relevances != NULL, should_prefetch, trimmed_input));
+          answer, suggest_query_params, deletion_url, is_keyword_result,
+          relevance, relevances != NULL, should_prefetch, trimmed_input));
     }
   }
   results->relevances_from_server = relevances != NULL;
@@ -482,32 +492,13 @@ bool SearchSuggestionParser::ParseSuggestResults(
 
 // static
 void SearchSuggestionParser::GetAnswersImageURLs(
-    const base::DictionaryValue* answer_json,
+    const SuggestionAnswer& answer,

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

I think this function should move onto SuggestionAnswer

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

Yeah, duh, should've thought of that.  Done.

     std::vector<GURL>* urls) {
-  DCHECK(answer_json);
-
-  const base::ListValue* lines = NULL;
-  if (!answer_json->GetList("l", &lines) || !lines || lines->GetSize() == 0)
+  if(!answer.is_valid())
     return;
 
-  for (base::ListValue::const_iterator iter = lines->begin();
-       iter != lines->end();
-       ++iter) {
-    const base::DictionaryValue* line = NULL;
-    if (!(*iter)->GetAsDictionary(&line) || !line)
-      continue;
-
-    std::string image_host_and_path;
-    if (!line->GetString("il.i.d", &image_host_and_path) ||
-        image_host_and_path.empty())
-      continue;
-    // 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.
-    GURL image_url(
-        GURL(std::string(url::kHttpsScheme) + ":" + image_host_and_path));
-    if (image_url.is_valid())
-      urls->push_back(image_url);
-  }
+  if (answer.first_line().has_image_url())
+    urls->push_back(answer.first_line().image_url());
+  if (answer.second_line().has_image_url())
+    urls->push_back(answer.second_line().image_url());
 }