Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1043373002:
Implement onQueryEnded. (Closed)
|
Created:
5 years, 8 months ago by yawano Modified:
5 years, 8 months ago CC:
chromium-reviews, tapted, Matt Giuca, tfarina, oshima+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement onQueryEnded.
BUG=440649
Committed: https://crrev.com/00605fc02bab95e68aeec627ce4d786d77e84e20
Cr-Commit-Position: refs/heads/master@{#324010}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Stop timer. #Patch Set 3 : Rebase. #
Messages
Total messages: 19 (4 generated)
yawano@chromium.org changed reviewers: + mgiuca@chromium.org, satorux@chromium.org
PTAL. @satorux: - chrome/browser/chromeos/launcher_search_provider/service.h - chrome/browser/chromeos/launcher_search_provider/service.cc @mgiuca: - chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.cc Thank you!
This CL depends on http://crrev.com/1011353003.
mgiuca@chromium.org changed reviewers: + calamity@chromium.org
Hi Yuki, I am about to go on vacation. If you want this to land before I get back, Chris can give an LG-TM for the app_list part once he is satisfied. https://codereview.chromium.org/1043373002/diff/1/chrome/browser/chromeos/lau... File chrome/browser/chromeos/launcher_search_provider/service.cc (right): https://codereview.chromium.org/1043373002/diff/1/chrome/browser/chromeos/lau... chrome/browser/chromeos/launcher_search_provider/service.cc:67: make_scoped_ptr(new extensions::Event( This can probably be abstracted into a private method DispatchEventToAllExtensions. (Share code with OnQueryStarted.) https://codereview.chromium.org/1043373002/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.cc (right): https://codereview.chromium.org/1043373002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.cc:38: // that no query is running at service side. If we are delaying a query, and we get a Stop() signal, won't this do nothing and we'll continue without sending the OnQueryEnded signal? (i.e., shouldn't we stop the timer in this case?) https://codereview.chromium.org/1043373002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.cc:45: base::TimeDelta::FromMilliseconds(kLauncherSearchProviderQueryDelayInMs); Why did this get reformatted?
@mgiuca: Thank you. Have a nice vacation! @calamity: Could you take a look at files under chrome/browser/ui/app_list? Thank you! https://codereview.chromium.org/1043373002/diff/1/chrome/browser/chromeos/lau... File chrome/browser/chromeos/launcher_search_provider/service.cc (right): https://codereview.chromium.org/1043373002/diff/1/chrome/browser/chromeos/lau... chrome/browser/chromeos/launcher_search_provider/service.cc:67: make_scoped_ptr(new extensions::Event( Since DispatchEventToExtension takes scoped_ptr, it seems hard to extract this loop as a method. Event creation method(api_launcher_search_provider::OnQueryEnded::Create) also returns scoped_ptr. Is it possible to extract this loop? https://codereview.chromium.org/1043373002/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.cc (right): https://codereview.chromium.org/1043373002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.cc:38: // that no query is running at service side. Since kLauncherSearchProviderQueryDelayInMs is smaller than the query time limit (1500ms), it cannot happen. Should we consider a case that the delay is longer than the time limit? https://codereview.chromium.org/1043373002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.cc:45: base::TimeDelta::FromMilliseconds(kLauncherSearchProviderQueryDelayInMs); Sorry, there is no intention to reformat this line. I think git cl format has reformatted this line. The format in the previous patch might be wrongly formatted.
https://codereview.chromium.org/1043373002/diff/1/chrome/browser/chromeos/lau... File chrome/browser/chromeos/launcher_search_provider/service.cc (right): https://codereview.chromium.org/1043373002/diff/1/chrome/browser/chromeos/lau... chrome/browser/chromeos/launcher_search_provider/service.cc:67: make_scoped_ptr(new extensions::Event( Yes, you can return scoped_ptrs from functions just fine. You can also take a scoped_ptr as an argument, but passing it is a bit tricky. You have to call the .Pass() method on the argument, which will pass the scoped_ptr into the function, but set it to null in the caller (so you can't use it again in the caller once you've passed it into the function). https://codereview.chromium.org/1043373002/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.cc (right): https://codereview.chromium.org/1043373002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.cc:38: // that no query is running at service side. Since that is a very separate system to this, yes, we should consider that case. (You should assume that the app_list code can call Stop() whenever it feels like it, and handle that case accordingly.) https://codereview.chromium.org/1043373002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.cc:45: base::TimeDelta::FromMilliseconds(kLauncherSearchProviderQueryDelayInMs); On 2015/04/01 11:26:20, yawano wrote: > Sorry, there is no intention to reformat this line. I think git cl format has > reformatted this line. The format in the previous patch might be wrongly > formatted. Acknowledged.
https://codereview.chromium.org/1043373002/diff/1/chrome/browser/chromeos/lau... File chrome/browser/chromeos/launcher_search_provider/service.cc (right): https://codereview.chromium.org/1043373002/diff/1/chrome/browser/chromeos/lau... chrome/browser/chromeos/launcher_search_provider/service.cc:67: make_scoped_ptr(new extensions::Event( As far as I understand correctly, I think DispatchEventToListenerExtensions will become as the following. But the problem is that we may call event.Pass() (at *1) more than twice if there are multiple listener extensions. DispatchEventToListenerExtensions(const std::string& event_name, scoped_ptr<base::ListValue> event) { extensions::EventRouter* event_router = ... std::set<ExtensionId> extension_ids = ... for (const ExtensionId extension_id : extension_ids) { event_router->DispatchEventToExtension( extension_id, make_scoped_ptr(new extensions::Event( event_name, event.Pass() // *1 )); ); } }
c/b/chromeos LGTM https://codereview.chromium.org/1043373002/diff/1/chrome/browser/chromeos/lau... File chrome/browser/chromeos/launcher_search_provider/service.cc (right): https://codereview.chromium.org/1043373002/diff/1/chrome/browser/chromeos/lau... chrome/browser/chromeos/launcher_search_provider/service.cc:67: make_scoped_ptr(new extensions::Event( On 2015/04/02 02:34:37, yawano wrote: > As far as I understand correctly, I think DispatchEventToListenerExtensions will > become as the following. But the problem is that we may call event.Pass() (at > *1) more than twice if there are multiple listener extensions. > > DispatchEventToListenerExtensions(const std::string& event_name, > scoped_ptr<base::ListValue> event) { > extensions::EventRouter* event_router = ... > std::set<ExtensionId> extension_ids = ... > for (const ExtensionId extension_id : extension_ids) { > event_router->DispatchEventToExtension( > extension_id, > make_scoped_ptr(new extensions::Event( > event_name, > event.Pass() // *1 > )); > ); > } > } I guess the two loops are small enough so we don't have to try to merge.
https://codereview.chromium.org/1043373002/diff/1/chrome/browser/chromeos/lau... File chrome/browser/chromeos/launcher_search_provider/service.cc (right): https://codereview.chromium.org/1043373002/diff/1/chrome/browser/chromeos/lau... chrome/browser/chromeos/launcher_search_provider/service.cc:67: make_scoped_ptr(new extensions::Event( On 2015/04/02 02:34:37, yawano wrote: > As far as I understand correctly, I think DispatchEventToListenerExtensions will > become as the following. But the problem is that we may call event.Pass() (at > *1) more than twice if there are multiple listener extensions. > > DispatchEventToListenerExtensions(const std::string& event_name, > scoped_ptr<base::ListValue> event) { > extensions::EventRouter* event_router = ... > std::set<ExtensionId> extension_ids = ... > for (const ExtensionId extension_id : extension_ids) { > event_router->DispatchEventToExtension( > extension_id, > make_scoped_ptr(new extensions::Event( > event_name, > event.Pass() // *1 > )); > ); > } > } You can use Value::DeepCopy? Also, you should probably call the ListValue args or event_args.
@mgiuca or @calamity: PTAL. Thank you! https://codereview.chromium.org/1043373002/diff/1/chrome/browser/chromeos/lau... File chrome/browser/chromeos/launcher_search_provider/service.cc (right): https://codereview.chromium.org/1043373002/diff/1/chrome/browser/chromeos/lau... chrome/browser/chromeos/launcher_search_provider/service.cc:67: make_scoped_ptr(new extensions::Event( Yes, but I'm not sure it's okay to deep copy event object. api_launcher_search_provider::OnQueryEnded::Create method is a generated code from IDL. Interfaces of OnQueryStarted::Create and OnQueryEnded::Create are different. It seems difficult to create a private method to extract these loops. I prefer to keep these two loops to keep the code simple. WDYT? https://codereview.chromium.org/1043373002/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.cc (right): https://codereview.chromium.org/1043373002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.cc:38: // that no query is running at service side. On 2015/04/01 23:05:04, Matt Giuca (OOO until April 9) wrote: > Since that is a very separate system to this, yes, we should consider that case. > > (You should assume that the app_list code can call Stop() whenever it feels like > it, and handle that case accordingly.) Done.
@calamity: Just in case you missed, PTAL this CL. Thank you!
lgtm
On 2015/04/07 03:10:40, calamity wrote: > lgtm Thank you!
The CQ bit was checked by yawano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from satorux@chromium.org Link to the patchset: https://codereview.chromium.org/1043373002/#ps40001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1043373002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/00605fc02bab95e68aeec627ce4d786d77e84e20 Cr-Commit-Position: refs/heads/master@{#324010} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
