[AiS] Fix parsing for Answers Images.

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 a481def345dee8aec352842f70e423d10125faeb..31a2781978df724aa17074ae3e948decebcb1a69 100644
--- a/components/omnibox/search_suggestion_parser.cc
+++ b/components/omnibox/search_suggestion_parser.cc
@@ -489,9 +489,10 @@ void SearchSuggestionParser::GetAnswersImageURLs(
     return;
 
   for (size_t line = 0; line < lines->GetSize(); ++line) {
+    const base::DictionaryValue* value = NULL;
     const base::DictionaryValue* imageLine = NULL;
-    lines->GetDictionary(line, &imageLine);
-    if (!imageLine)
+    lines->GetDictionary(line, &value);

[Mark P, 2014/08/15 22:49:16]

comment: you and put this at the beginning of the if statement below as
if (!lines->GetDictionary(line, &value) || !value ...

ditto in other if below

[groby-ooo-7-16, 2014/08/22 00:43:58]

I'm actually taking all comments here and rewriting this. The whole thing is not
as clear as it could be.

+    if (!value || !value->GetDictionary("il", &imageLine))

[Mark P, 2014/08/15 22:49:16]

consider || !imageLine at the end

[groby-ooo-7-16, 2014/08/22 00:43:58]

imageLine is guaranteed non-NULL if GetDictionary succeeds. (Technically,
guaranteed to be unchanged, but it's initialized to NULL)

That's why I'm not checking the return value of GetDictionary above, either.

       continue;
     const base::DictionaryValue* imageData = NULL;
     imageLine->GetDictionary("i", &imageData);
@@ -499,6 +500,6 @@ void SearchSuggestionParser::GetAnswersImageURLs(
       continue;
     std::string imageUrl;

[Mark P, 2014/08/15 22:49:16]

If you're going to prepend a protocol to this, then it isn't a URL.  Consider
not naming it as such, or at least point this fact out.  Perhaps
image_host_and_path?

(p.s.  why is the format odd?  why does it pass url-like-things without a
protocol?)

[groby-ooo-7-16, 2014/08/22 00:43:58]

It's odd because GWS returns paths without scheme, so they work in the https and
http page version. (HTML lets you spec URLs without scheme, and the browser is
then supposed to use the enclosing page's scheme, IIRC)

host_and_path will do. (It's technically authority_and_path, but that only makes
sense if you read the RFCs on URIs :)

     imageData->GetString("d", &imageUrl);

[Mark P, 2014/08/15 22:49:16]

Please don't ignore the return value.

[groby-ooo-7-16, 2014/08/22 00:43:58]

Done.

-    urls->push_back(GURL(imageUrl));
+    urls->push_back(GURL("https:" + imageUrl));

[Mark P, 2014/08/15 22:49:16]

This probably should be one of the protocol value constants.

[groby-ooo-7-16, 2014/08/22 00:43:58]

Done.

   }
 }