Implement onOpenResult.

Implement onOpenResult.

onOpenResult event is dispatched when a search result which is provided by an extension is opened.

BUG=440649
Committed: https://crrev.com/bbdd2f0e44081940d1c4111676267e1ac6b654cf
Cr-Commit-Position: refs/heads/master@{#325823}

[yawano, 2015-04-17 06:15:11]

yawano@chromium.org changed reviewers:
+ mgiuca@chromium.org, mpearson@chromium.org, satorux@chromium.org

[yawano, 2015-04-17 06:15:11]

PTAL.

@satorux: files under chrome/browser/chromeos.
@mpearson: histograms.xml.
@mgiuca: the rest of files.

Thank you!

[satorux1, 2015-04-17 06:21:52]

https://codereview.chromium.org/1095793002/diff/1/chrome/browser/chromeos/lau...
File chrome/browser/chromeos/launcher_search_provider/service.cc (right):

https://codereview.chromium.org/1095793002/diff/1/chrome/browser/chromeos/lau...
chrome/browser/chromeos/launcher_search_provider/service.cc:87: void
Service::OnOpenResult(const extensions::Extension* extension,
if it must not be null, how about making it const extensions::Extension&

[yawano, 2015-04-17 06:39:08]

@satorux: Thank you for the review! PTAL.

https://codereview.chromium.org/1095793002/diff/1/chrome/browser/chromeos/lau...
File chrome/browser/chromeos/launcher_search_provider/service.cc (right):

https://codereview.chromium.org/1095793002/diff/1/chrome/browser/chromeos/lau...
chrome/browser/chromeos/launcher_search_provider/service.cc:87: void
Service::OnOpenResult(const extensions::Extension* extension,
In this method, we only need ExtensionId instead of Extension*. I changed the
interface from Extension* to ExtensionId.

[satorux1, 2015-04-17 06:50:21]

c/b/chromeos lgtm

https://codereview.chromium.org/1095793002/diff/1/chrome/browser/chromeos/lau...
File chrome/browser/chromeos/launcher_search_provider/service.cc (right):

https://codereview.chromium.org/1095793002/diff/1/chrome/browser/chromeos/lau...
chrome/browser/chromeos/launcher_search_provider/service.cc:87: void
Service::OnOpenResult(const extensions::Extension* extension,
that's nicer! Law of Demeter

[yawano, 2015-04-17 06:54:19]

On 2015/04/17 06:50:21, satorux1 wrote:
> c/b/chromeos lgtm
> 
>
https://codereview.chromium.org/1095793002/diff/1/chrome/browser/chromeos/lau...
> File chrome/browser/chromeos/launcher_search_provider/service.cc (right):
> 
>
https://codereview.chromium.org/1095793002/diff/1/chrome/browser/chromeos/lau...
> chrome/browser/chromeos/launcher_search_provider/service.cc:87: void
> Service::OnOpenResult(const extensions::Extension* extension,
> that's nicer! Law of Demeter

@satorux: Thank you for the review!

[Mark P, 2015-04-17 16:08:45]

histograms.xml lgtm

[Matt Giuca, 2015-04-20 02:44:53]

https://codereview.chromium.org/1095793002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/launcher_search_provider/service.cc (right):

https://codereview.chromium.org/1095793002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/launcher_search_provider/service.cc:91: if
(!ContainsValue(extension_ids, extension_id))
Is there any reason to expect this to happen during normal operation? I can't
see how it would ever occur that a search result would have an extension ID of a
non-listener extension. Therefore, we could potentially CHECK here instead of
returning silently.

https://codereview.chromium.org/1095793002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/launcher_search_provider/service.h (right):

https://codereview.chromium.org/1095793002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/launcher_search_provider/service.h:62:
std::set<extensions::ExtensionId> GetListenerExtensionIds();
Unrelated to this CL, but I was looking at GetListenerExtensionIds, and found
that it linearly seeks through the entire list of installed extensions. Since
it's used by OnQueryStarted and OnQueryEnded, and now OnOpenResult, I think it
would be good to cache the listener set when opening the Service. Could you do
that in a separate CL?

https://codereview.chromium.org/1095793002/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/search/search_util.h (right):

https://codereview.chromium.org/1095793002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/search_util.h:25: // A result that is provided
by launcher search provider.
can you say "by the custom launcher search provider"

so it's clear to readers who may not be familiar with this area that "launcher
search provider" is the custom API-based search results.

[yawano, 2015-04-20 04:02:36]

@mgiuca: PTAL. Thank you!

https://codereview.chromium.org/1095793002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/launcher_search_provider/service.cc (right):

https://codereview.chromium.org/1095793002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/launcher_search_provider/service.cc:91: if
(!ContainsValue(extension_ids, extension_id))
Yes, this could not happen since extension_id is set at when the result is
generated (the value should be validated at the creation time). Changed this to
DCHECK.

https://codereview.chromium.org/1095793002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/launcher_search_provider/service.h (right):

https://codereview.chromium.org/1095793002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/launcher_search_provider/service.h:62:
std::set<extensions::ExtensionId> GetListenerExtensionIds();
Okay, I'll do it in a separate CL. To cache the list, we also need to listen
extension loads & unloads via ExtensionRegistryObserver to evict the cache. -
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

https://codereview.chromium.org/1095793002/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/search/search_util.h (right):

https://codereview.chromium.org/1095793002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/search_util.h:25: // A result that is provided
by launcher search provider.
On 2015/04/20 02:44:53, Matt Giuca wrote:
> can you say "by the custom launcher search provider"
> 
> so it's clear to readers who may not be familiar with this area that "launcher
> search provider" is the custom API-based search results.

Done.

https://codereview.chromium.org/1095793002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/launcher_search_provider/service.cc (right):

https://codereview.chromium.org/1095793002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/launcher_search_provider/service.cc:111: if
(!ContainsValue(GetListenerExtensionIds(), extension->id()))
I noticed that we haven't checked extension id here. If they can get (or
estimate) query_id, not-allowed extensions could have set search results.

[Matt Giuca, 2015-04-20 04:36:26]

https://codereview.chromium.org/1095793002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/launcher_search_provider/service.cc (right):

https://codereview.chromium.org/1095793002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/launcher_search_provider/service.cc:91: if
(!ContainsValue(extension_ids, extension_id))
I think it should be a CHECK because it would be a security issue if it were not
satisfied.

https://codereview.chromium.org/1095793002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/launcher_search_provider/service.h (right):

https://codereview.chromium.org/1095793002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/launcher_search_provider/service.h:62:
std::set<extensions::ExtensionId> GetListenerExtensionIds();
On 2015/04/20 04:02:36, yawano wrote:
> Okay, I'll do it in a separate CL. To cache the list, we also need to listen
> extension loads & unloads via ExtensionRegistryObserver to evict the cache. -
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

Acknowledged.

[yawano, 2015-04-20 04:46:23]

Thank you for the review! PTAL.

https://codereview.chromium.org/1095793002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/launcher_search_provider/service.cc (right):

https://codereview.chromium.org/1095793002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/launcher_search_provider/service.cc:91: if
(!ContainsValue(extension_ids, extension_id))
On 2015/04/20 04:36:26, Matt Giuca wrote:
> I think it should be a CHECK because it would be a security issue if it were
not
> satisfied.

Done.

[Matt Giuca, 2015-04-20 04:47:47]

lgtm

[yawano, 2015-04-20 04:48:54]

On 2015/04/20 04:47:47, Matt Giuca wrote:
> lgtm

Thank you!

[yawano, 2015-04-20 04:49:00]

The CQ bit was checked by yawano@chromium.org

[yawano, 2015-04-20 04:49:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
satorux@chromium.org, mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/1095793002/#ps80001
(title: "Change to CHECK.")

[commit-bot: I haz the power, 2015-04-20 08:02:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1095793002/80001

[commit-bot: I haz the power, 2015-04-20 10:07:08]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-04-20 10:09:59]

Patchset 5 (id:??) landed as
https://crrev.com/bbdd2f0e44081940d1c4111676267e1ac6b654cf
Cr-Commit-Position: refs/heads/master@{#325823}