[AiS] Fix parsing for Answers Images.

components/omnibox/search_suggestion_parser.cc

 // 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/search_suggestion_parser.h"
 
 #include "base/i18n/icu_string_conversions.h"
 #include "base/json/json_string_value_serializer.h"
 #include "base/json/json_writer.h"
 #include "base/logging.h"
@@ skipping 471 matching lines @@
 void SearchSuggestionParser::GetAnswersImageURLs(
     const base::DictionaryValue* answer_json,
     std::vector<GURL>* urls) {
   DCHECK(answer_json);
   const base::ListValue* lines = NULL;
   answer_json->GetList("l", &lines);
   if (!lines || lines->GetSize() == 0)
     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);
     if (!imageData)
       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.

   }
 }