app_list: Add web store search.

app_list: Add web store search.

- Split SafeJsonParser out of WebstoreDataFetcher to share the code;
- Add a WebstoreSearchFetcher to retrieve results from web store server;
- Add a WebstoreResultIconSource that fetches icon when painted on screen;
- Add a WebStoreProvider that generates WebstoreResult for results fetched
  from server or a placeholder "Search in webstore" result;

BUG=173430
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=201422

[xiyuan, 2013-05-17 22:08:27]

James, please do the main review.
Antony could focus on extensions/* changes.

Thanks.

[James Cook, 2013-05-17 22:21:51]

On 2013/05/17 22:08:27, xiyuan wrote:
> James, please do the main review.
> Antony could focus on extensions/* changes.
> 
> Thanks.

I may not get to this review today - I'll get to it as soon as I can.

[xiyuan, 2013-05-17 22:24:26]

On 2013/05/17 22:21:51, James Cook (Chromium) wrote:
> On 2013/05/17 22:08:27, xiyuan wrote:
> > James, please do the main review.
> > Antony could focus on extensions/* changes.
> > 
> > Thanks.
> 
> I may not get to this review today - I'll get to it as soon as I can.

No worries. Take your time.

[koz (OOO until 15th September), 2013-05-20 01:14:16]

drive by

https://codereview.chromium.org/15342003/diff/1/chrome/browser/ui/app_list/se...
File chrome/browser/ui/app_list/search/webstore_provider.cc (right):

https://codereview.chromium.org/15342003/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/webstore_provider.cc:47:
Add(scoped_ptr<ChromeSearchResult>(
I think this would be clearer with a comment:

"Add a placeholder result which when clicked will run the user's query in a
browser. This placeholder is removed when the search results arrive."

https://codereview.chromium.org/15342003/diff/1/chrome/browser/ui/app_list/se...
File chrome/browser/ui/app_list/search/webstore_result.cc (right):

https://codereview.chromium.org/15342003/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/webstore_result.cc:39:
icon_changed_factory_.GetWeakPtr()),
nit: I think it's more common to just name this variable weak_factory_.

https://codereview.chromium.org/15342003/diff/1/chrome/browser/ui/app_list/se...
File chrome/browser/ui/app_list/search/webstore_search_fetcher.cc (right):

https://codereview.chromium.org/15342003/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/webstore_search_fetcher.cc:24:
json_parser_factory_(this) {
nit: same comment as above.

[James Cook, 2013-05-20 14:39:33]

Overall looks good. Some comments.

https://codereview.chromium.org/15342003/diff/1/chrome/browser/safe_json_pars...
File chrome/browser/safe_json_parser.cc (right):

https://codereview.chromium.org/15342003/diff/1/chrome/browser/safe_json_pars...
chrome/browser/safe_json_parser.cc:24:
CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
Could the thread assertions be DCHECKs?

https://codereview.chromium.org/15342003/diff/1/chrome/browser/safe_json_pars...
chrome/browser/safe_json_parser.cc:64: void
SafeJsonParser::ReportResultOnUIThread() {
Actually, who deletes this object? The destructor is private.

https://codereview.chromium.org/15342003/diff/1/chrome/browser/safe_json_pars...
File chrome/browser/safe_json_parser.h (right):

https://codereview.chromium.org/15342003/diff/1/chrome/browser/safe_json_pars...
chrome/browser/safe_json_parser.h:30: SafeJsonParser(const std::string&
unsafe_json,
nit: Maybe comment on memory lifetime - I presume it deletes itself after either
success or failure?  Before or after calling the callback?

https://codereview.chromium.org/15342003/diff/1/chrome/browser/ui/app_list/se...
File chrome/browser/ui/app_list/search/search_controller.cc (right):

https://codereview.chromium.org/15342003/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/search_controller.cc:40:
AddProvider(Mixer::MAIN_GROUP, scoped_ptr<SearchProvider>(
I really like this approach of having a generic list of providers. Nice.

https://codereview.chromium.org/15342003/diff/1/chrome/browser/ui/app_list/se...
File chrome/browser/ui/app_list/search/search_webstore_result.cc (right):

https://codereview.chromium.org/15342003/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/search_webstore_result.cc:46:
params.disposition = ui::DispositionFromEventFlags(event_flags);
Random historical note: I think my very first Chrome patch touched
DispositionFromEventFlags. :-)

https://codereview.chromium.org/15342003/diff/1/chrome/browser/ui/app_list/se...
File chrome/browser/ui/app_list/search/webstore_provider.cc (right):

https://codereview.chromium.org/15342003/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/webstore_provider.cc:43: const std::string
query8 = UTF16ToUTF8(query);
nit: query8 -> query_utf8 or similar

At first I thought... Where are the other 7 queries?

https://codereview.chromium.org/15342003/diff/1/chrome/browser/ui/app_list/se...
File chrome/browser/ui/app_list/search/webstore_result_icon_source.cc (right):

https://codereview.chromium.org/15342003/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/webstore_result_icon_source.cc:87: std::string
unsafe_icon_data;
It's nice that you label unsafe data this way.

https://codereview.chromium.org/15342003/diff/1/chrome/browser/ui/app_list/se...
File chrome/browser/ui/app_list/search/webstore_result_icon_source.h (right):

https://codereview.chromium.org/15342003/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/webstore_result_icon_source.h:43:
gfx::ImageSkiaRep CreateBadgedIcon(ui::ScaleFactor scale_factor);
function comment?

https://codereview.chromium.org/15342003/diff/1/chrome/browser/ui/app_list/se...
File chrome/browser/ui/app_list/search/webstore_search_fetcher.cc (right):

https://codereview.chromium.org/15342003/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/webstore_search_fetcher.cc:71:
OnJsonParseError(kBadResponse);
Is kBadResponse used? OnJsonParseError seems to ignore the string passed to it.

https://codereview.chromium.org/15342003/diff/1/chrome/browser/ui/app_list/se...
File chrome/browser/ui/app_list/search/webstore_search_fetcher.h (right):

https://codereview.chromium.org/15342003/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/webstore_search_fetcher.h:39: void Start(const
std::string& query, const std::string& hl);
What is "hl"?

https://codereview.chromium.org/15342003/diff/1/chrome/common/extensions/exte...
File chrome/common/extensions/extension_constants.cc (right):

https://codereview.chromium.org/15342003/diff/1/chrome/common/extensions/exte...
chrome/common/extensions/extension_constants.cc:54: url =
net::AppendQueryParameter(url, "q", query);
Does this query need to be escaped?  I presume that must happen in the calling
code. Maybe the function comment could mention who has to do the escaping?

https://codereview.chromium.org/15342003/diff/1/chrome/common/extensions/exte...
File chrome/common/extensions/extension_constants.h (right):

https://codereview.chromium.org/15342003/diff/1/chrome/common/extensions/exte...
chrome/common/extensions/extension_constants.h:37: // Returns the URL used to
get webstore search results in json format. The URL
nit: json -> JSON

https://codereview.chromium.org/15342003/diff/1/chrome/common/extensions/exte...
chrome/common/extensions/extension_constants.h:42: const std::string& hl);
Document "hl"

[xiyuan, 2013-05-20 16:56:30]

https://codereview.chromium.org/15342003/diff/1/chrome/browser/safe_json_pars...
File chrome/browser/safe_json_parser.cc (right):

https://codereview.chromium.org/15342003/diff/1/chrome/browser/safe_json_pars...
chrome/browser/safe_json_parser.cc:24:
CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2013/05/20 14:39:34, James Cook (Chromium) wrote:
> Could the thread assertions be DCHECKs?

Done. Thread assertions are changed to use DCHECKs.

https://codereview.chromium.org/15342003/diff/1/chrome/browser/safe_json_pars...
chrome/browser/safe_json_parser.cc:64: void
SafeJsonParser::ReportResultOnUIThread() {
On 2013/05/20 14:39:34, James Cook (Chromium) wrote:
> Actually, who deletes this object? The destructor is private.

SafeJsonParser derives from UtilityProcessHostClient, which derives a
RefCountedThreadSafe object. So as long as we have pending PostTasks, the object
is alive. And it will be deleted when all tasks are finished, i.e. after
ReportResultOnUIThread returns.

https://codereview.chromium.org/15342003/diff/1/chrome/browser/safe_json_pars...
File chrome/browser/safe_json_parser.h (right):

https://codereview.chromium.org/15342003/diff/1/chrome/browser/safe_json_pars...
chrome/browser/safe_json_parser.h:30: SafeJsonParser(const std::string&
unsafe_json,
On 2013/05/20 14:39:34, James Cook (Chromium) wrote:
> nit: Maybe comment on memory lifetime - I presume it deletes itself after
either
> success or failure?  Before or after calling the callback?

Updated the class comment to document the lifetime.

https://codereview.chromium.org/15342003/diff/1/chrome/browser/ui/app_list/se...
File chrome/browser/ui/app_list/search/webstore_provider.cc (right):

https://codereview.chromium.org/15342003/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/webstore_provider.cc:43: const std::string
query8 = UTF16ToUTF8(query);
On 2013/05/20 14:39:34, James Cook (Chromium) wrote:
> nit: query8 -> query_utf8 or similar
> 
> At first I thought... Where are the other 7 queries?

Renamed query8 -> query_utf8. Too lazy to type. :p

https://codereview.chromium.org/15342003/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/webstore_provider.cc:47:
Add(scoped_ptr<ChromeSearchResult>(
On 2013/05/20 01:14:16, koz wrote:
> I think this would be clearer with a comment:
> 
> "Add a placeholder result which when clicked will run the user's query in a
> browser. This placeholder is removed when the search results arrive."

Done.

https://codereview.chromium.org/15342003/diff/1/chrome/browser/ui/app_list/se...
File chrome/browser/ui/app_list/search/webstore_result.cc (right):

https://codereview.chromium.org/15342003/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/webstore_result.cc:39:
icon_changed_factory_.GetWeakPtr()),
On 2013/05/20 01:14:16, koz wrote:
> nit: I think it's more common to just name this variable weak_factory_.

Done.

https://codereview.chromium.org/15342003/diff/1/chrome/browser/ui/app_list/se...
File chrome/browser/ui/app_list/search/webstore_result_icon_source.h (right):

https://codereview.chromium.org/15342003/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/webstore_result_icon_source.h:43:
gfx::ImageSkiaRep CreateBadgedIcon(ui::ScaleFactor scale_factor);
On 2013/05/20 14:39:34, James Cook (Chromium) wrote:
> function comment?

Done.

https://codereview.chromium.org/15342003/diff/1/chrome/browser/ui/app_list/se...
File chrome/browser/ui/app_list/search/webstore_search_fetcher.cc (right):

https://codereview.chromium.org/15342003/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/webstore_search_fetcher.cc:24:
json_parser_factory_(this) {
On 2013/05/20 01:14:16, koz wrote:
> nit: same comment as above.

Done.

https://codereview.chromium.org/15342003/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/webstore_search_fetcher.cc:71:
OnJsonParseError(kBadResponse);
On 2013/05/20 14:39:34, James Cook (Chromium) wrote:
> Is kBadResponse used? OnJsonParseError seems to ignore the string passed to
it.

OnJsonParseError is a callback for SafeJsonParser. And SafeJsonParser code is
split out from WebstoreDataFetcher under extensions dir. The code there has a
convention to pass back a human readable error string. I have not thought
clearly whether it is good or not and just followed it.

https://codereview.chromium.org/15342003/diff/1/chrome/browser/ui/app_list/se...
File chrome/browser/ui/app_list/search/webstore_search_fetcher.h (right):

https://codereview.chromium.org/15342003/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/webstore_search_fetcher.h:39: void Start(const
std::string& query, const std::string& hl);
On 2013/05/20 14:39:34, James Cook (Chromium) wrote:
> What is "hl"?

hl = host language. Added a comment to make it clear.

https://codereview.chromium.org/15342003/diff/1/chrome/common/extensions/exte...
File chrome/common/extensions/extension_constants.cc (right):

https://codereview.chromium.org/15342003/diff/1/chrome/common/extensions/exte...
chrome/common/extensions/extension_constants.cc:54: url =
net::AppendQueryParameter(url, "q", query);
On 2013/05/20 14:39:34, James Cook (Chromium) wrote:
> Does this query need to be escaped?  I presume that must happen in the calling
> code. Maybe the function comment could mention who has to do the escaping?

Nope, net::AppendQueryParameter does the escape. The caller does not need to do
anything. Function comment updated to make it clear to the future reader.

https://codereview.chromium.org/15342003/diff/1/chrome/common/extensions/exte...
File chrome/common/extensions/extension_constants.h (right):

https://codereview.chromium.org/15342003/diff/1/chrome/common/extensions/exte...
chrome/common/extensions/extension_constants.h:37: // Returns the URL used to
get webstore search results in json format. The URL
On 2013/05/20 14:39:34, James Cook (Chromium) wrote:
> nit: json -> JSON

Done.

https://codereview.chromium.org/15342003/diff/1/chrome/common/extensions/exte...
chrome/common/extensions/extension_constants.h:42: const std::string& hl);
On 2013/05/20 14:39:34, James Cook (Chromium) wrote:
> Document "hl"

Done.

[James Cook, 2013-05-20 18:47:09]

LGTM. Nice patch.

[xiyuan, 2013-05-21 01:26:07]

asargent@/koz@, could one of you review and bless the change?

Thanks.

[koz (OOO until 15th September), 2013-05-21 01:36:33]

extensions/* lgtm

[commit-bot: I haz the power, 2013-05-21 04:31:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xiyuan@chromium.org/15342003/25001

[commit-bot: I haz the power, 2013-05-21 04:41:14]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[xiyuan, 2013-05-21 04:57:49]

+sky for owners for
    chrome/browser/safe_json_parser.cc
    chrome/browser/safe_json_parser.h

The code is split from extensions/webstore_data_fetcher.cc to be shared with
webstore_search_fetcher.cc.

And I see we have a chrome/browser/common dir now, maybe the files should go
there?

[sky, 2013-05-21 14:57:00]

chrome/common is if you need to access from both renderer and browser. That
doesn't appear to be the case here.

LGTM

https://codereview.chromium.org/15342003/diff/25001/chrome/browser/safe_json_...
File chrome/browser/safe_json_parser.cc (right):

https://codereview.chromium.org/15342003/diff/25001/chrome/browser/safe_json_...
chrome/browser/safe_json_parser.cc:21: SafeJsonParser::~SafeJsonParser() {}
nit: make position match header.

https://codereview.chromium.org/15342003/diff/25001/chrome/browser/safe_json_...
File chrome/browser/safe_json_parser.h (right):

https://codereview.chromium.org/15342003/diff/25001/chrome/browser/safe_json_...
chrome/browser/safe_json_parser.h:24: // SafeJsonParser parses a given JSON
safely via utility process. The object is
via 'a' utility...

[xiyuan, 2013-05-21 16:23:39]

https://codereview.chromium.org/15342003/diff/25001/chrome/browser/safe_json_...
File chrome/browser/safe_json_parser.cc (right):

https://codereview.chromium.org/15342003/diff/25001/chrome/browser/safe_json_...
chrome/browser/safe_json_parser.cc:21: SafeJsonParser::~SafeJsonParser() {}
On 2013/05/21 14:57:00, sky wrote:
> nit: make position match header.

Done.

https://codereview.chromium.org/15342003/diff/25001/chrome/browser/safe_json_...
File chrome/browser/safe_json_parser.h (right):

https://codereview.chromium.org/15342003/diff/25001/chrome/browser/safe_json_...
chrome/browser/safe_json_parser.h:24: // SafeJsonParser parses a given JSON
safely via utility process. The object is
On 2013/05/21 14:57:00, sky wrote:
> via 'a' utility...

Done.

[commit-bot: I haz the power, 2013-05-21 16:32:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xiyuan@chromium.org/15342003/38001

[commit-bot: I haz the power, 2013-05-21 19:04:14]

Retried try job too often on win_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-05-21 22:24:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xiyuan@chromium.org/15342003/38001

[commit-bot: I haz the power, 2013-05-22 01:53:08]

Change committed as 201422