Support specified JSON as argument to setSuggestions().

Support specified JSON as argument to setSuggestions().

Maintain backwards compatibility with the array of strings.

TEST=interactive_ui_tests --gtest_filter=InstantTest.*
BUG=None

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=71450

[sky, 2011-01-13 22:37:46]

http://codereview.chromium.org/6310003/diff/1/chrome/renderer/searchbox_exten...
File chrome/renderer/searchbox_extension.cc (right):

http://codereview.chromium.org/6310003/diff/1/chrome/renderer/searchbox_exten...
chrome/renderer/searchbox_extension.cc:290: // Require at least one arg.
Should we treat an empty array as no suggestions?

http://codereview.chromium.org/6310003/diff/1/chrome/renderer/searchbox_exten...
chrome/renderer/searchbox_extension.cc:323: suggestion_value.As<v8::Array>();
How come you use As<Array> here?

http://codereview.chromium.org/6310003/diff/1/chrome/renderer/searchbox_exten...
chrome/renderer/searchbox_extension.cc:330: if (!suggestion.length()) continue;
It would be nice to add LOGing to all fail cases. That way, if someone on the
search side ends up debugging they can see where the error is.

http://codereview.chromium.org/6310003/diff/1/chrome/test/data/instant/search...
File chrome/test/data/instant/search.html (right):

http://codereview.chromium.org/6310003/diff/1/chrome/test/data/instant/search...
chrome/test/data/instant/search.html:18: window.setSuggestionsArgument =
["abcdef"];
Doesn't this need to be map with suggestions: containing an array of maps?

[tonyg, 2011-01-13 23:07:50]

http://codereview.chromium.org/6310003/diff/1/chrome/renderer/searchbox_exten...
File chrome/renderer/searchbox_extension.cc (right):

http://codereview.chromium.org/6310003/diff/1/chrome/renderer/searchbox_exten...
chrome/renderer/searchbox_extension.cc:290: // Require at least one arg.
On 2011/01/13 22:37:46, sky wrote:
> Should we treat an empty array as no suggestions?

Seems reasonable. Done.

http://codereview.chromium.org/6310003/diff/1/chrome/renderer/searchbox_exten...
chrome/renderer/searchbox_extension.cc:323: suggestion_value.As<v8::Array>();
On 2011/01/13 22:37:46, sky wrote:
> How come you use As<Array> here?

Copy/paste error. Fixed.

http://codereview.chromium.org/6310003/diff/1/chrome/renderer/searchbox_exten...
chrome/renderer/searchbox_extension.cc:330: if (!suggestion.length()) continue;
On 2011/01/13 22:37:46, sky wrote:
> It would be nice to add LOGing to all fail cases. That way, if someone on the
> search side ends up debugging they can see where the error is.

Seems like noise to me. Is there a way to LOG only for Debug builds or were you
suggestions something like LOG(WARNING)?

http://codereview.chromium.org/6310003/diff/1/chrome/test/data/instant/search...
File chrome/test/data/instant/search.html (right):

http://codereview.chromium.org/6310003/diff/1/chrome/test/data/instant/search...
chrome/test/data/instant/search.html:18: window.setSuggestionsArgument =
["abcdef"];
On 2011/01/13 22:37:46, sky wrote:
> Doesn't this need to be map with suggestions: containing an array of maps?

An array of strings is the old format (which continues to be supported). But it
does seem better to have the new one here. Fixed.

[sky, 2011-01-13 23:25:13]

LGTM

[tonyg, 2011-01-13 23:30:58]

Thanks, I'll wait for the try jobs then land.

On Thu, Jan 13, 2011 at 3:25 PM,  <sky@chromium.org> wrote:
> LGTM
>
> http://codereview.chromium.org/6310003/
>