Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
637323005:
Add Search Service in Enhanced Bookmark Bridge (Closed)
DescriptionAdd Search Service in Enhanced Bookmark Bridge
A Search client is created during initialization of
EnhancedBookmarkBridge. And when the bridge destructs, search service is
also destroyed.
BUG=415774
Committed: https://crrev.com/e52346f194e9f2d097b4f6d3fbfa359eca7c92df
Cr-Commit-Position: refs/heads/master@{#302107}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Add MRUCache, make search synchronous if results are already there, make logic simpler #Patch Set 3 : fix few nits #
Total comments: 56
Patch Set 4 : Respond to comments #
Total comments: 13
Patch Set 5 : A few nits #
Total comments: 1
Patch Set 6 : rebase #
Messages
Total messages: 29 (5 generated)
ianwen@chromium.org changed reviewers: + danduong@chromium.org, kkimlabs@chromium.org, tedchoc@chromium.org
lgtm just a side note: |searches_| in BookmarkServerSearchService is just growing, and cleared on only certain events. Since the size won't be big and we will be destroying the class at the end of usage, I guess it's practically fine though. https://codereview.chromium.org/637323005/diff/1/chrome/android/java/src/org/... File chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java (right): https://codereview.chromium.org/637323005/diff/1/chrome/android/java/src/org/... chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java:107: * called after {@link SearchServiceObserver#onSearchResultReturned()} Let's add a cautious comment that the search result can be removed by bookmark events. https://codereview.chromium.org/637323005/diff/1/chrome/browser/android/enhan... File chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.h (right): https://codereview.chromium.org/637323005/diff/1/chrome/browser/android/enhan... chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.h:59: BookmarkServerSearchService* search_service_; Let's make it scoped_ptr
lgtm https://codereview.chromium.org/637323005/diff/1/chrome/android/java/src/org/... File chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java (right): https://codereview.chromium.org/637323005/diff/1/chrome/android/java/src/org/... chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java:44: * Invoked when client detects that search results have been updated. I think it is worth noting that this might not correspond with the last sendSearchRequest call. https://codereview.chromium.org/637323005/diff/1/chrome/android/java/src/org/... chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java:118: * Registers a FiltersObserver to listen for filter change notifications. copy/paste incorrectness https://codereview.chromium.org/637323005/diff/1/chrome/browser/android/enhan... File chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc (right): https://codereview.chromium.org/637323005/diff/1/chrome/browser/android/enhan... chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc:125: env, j_list, node->id(), node->type()); indented too much
not lgtm (After seeing the downstream, I have better understanding on this code) My understanding is that, |searches_| variable in BookmarkServerSearchService is meant to be correct all the times, so we don't need to call BookmarkServerSearchService::Search every time we want the result, instead, we should check if the result is already in |searches_| first. We don't have such a function yet so we need to add it. Secondly, we cannot distinguish if we don't have search result yet or the result is empty, by |BookmarkServerSearchService::ResultForQuery| function, because it returns an empty list for the both cases. We should able to distinguish, or have a way to ensure that the latest search result is already there. Because if earlier search result arrives lately and triggers callback, there is no way to know 1. if the current search result is arrived and empty, or 2. if it's not arrived yet .
kkimlabs@chromium.org changed reviewers: + lpromero@chromium.org, noyau@chromium.org
+cc Bling people for sync
https://codereview.chromium.org/637323005/diff/1/chrome/android/java/src/org/... File chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java (right): https://codereview.chromium.org/637323005/diff/1/chrome/android/java/src/org/... chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java:44: * Invoked when client detects that search results have been updated. On 2014/10/23 00:31:42, Ted C wrote: > I think it is worth noting that this might not correspond with the last > sendSearchRequest call. This will always be triggered for the most recent query as we clear previous requests everytime we call Search(). https://codereview.chromium.org/637323005/diff/1/chrome/android/java/src/org/... chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java:107: * called after {@link SearchServiceObserver#onSearchResultReturned()} On 2014/10/22 18:42:01, Kibeom Kim wrote: > Let's add a cautious comment that the search result can be removed by bookmark > events. This will only happen in a race condition when syncing takes longer than the total amount of time that user click delete + click search + type query + get request from server. Maybe I should still add a null check in onItemSelected? https://codereview.chromium.org/637323005/diff/1/chrome/android/java/src/org/... chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java:118: * Registers a FiltersObserver to listen for filter change notifications. On 2014/10/23 00:31:42, Ted C wrote: > copy/paste incorrectness Done. https://codereview.chromium.org/637323005/diff/1/chrome/browser/android/enhan... File chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc (right): https://codereview.chromium.org/637323005/diff/1/chrome/browser/android/enhan... chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc:125: env, j_list, node->id(), node->type()); On 2014/10/23 00:31:42, Ted C wrote: > indented too much Function deleted. https://codereview.chromium.org/637323005/diff/1/chrome/browser/android/enhan... File chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.h (right): https://codereview.chromium.org/637323005/diff/1/chrome/browser/android/enhan... chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.h:59: BookmarkServerSearchService* search_service_; On 2014/10/22 18:42:01, Kibeom Kim wrote: > Let's make it scoped_ptr This service should never be deleted.
https://codereview.chromium.org/637323005/diff/1/chrome/browser/android/enhan... File chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.h (right): https://codereview.chromium.org/637323005/diff/1/chrome/browser/android/enhan... chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.h:59: BookmarkServerSearchService* search_service_; On 2014/10/29 00:11:39, Ian Wen wrote: > On 2014/10/22 18:42:01, Kibeom Kim wrote: > > Let's make it scoped_ptr > > This service should never be deleted. But it looks you're deleting in enhanced_bookmarks_bridge.cc ? And also, what's the reason it shouldn't be deleted? https://codereview.chromium.org/637323005/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java (right): https://codereview.chromium.org/637323005/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java:145: private static List<BookmarkId> createBookmarkIdList() { Let's move this function and below |addToBookmarkIdList| to BookmarkId.java , and also delete those two functions in BookmarksBridge.java to reduce unnecessary code duplication. https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... File components/enhanced_bookmarks/bookmark_server_search_service.cc (right): https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.cc:15: const int kMRUCacheMaxSize = 50; I think it's better to indicate what this variable is for, in the name. Maybe kSearchCacheMaxSize. https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.cc:29: searches_ = new base::MRUCache<std::string, std::vector<std::string> >( nit: I think now we can use >> instead of > > https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... File components/enhanced_bookmarks/bookmark_server_search_service.h (right): https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.h:66: base::MRUCache<std::string, std::vector<std::string> >* searches_; Since the lifetime of |searches_| is the scope of this class, it doesn't have to be a pointer.
https://codereview.chromium.org/637323005/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java (right): https://codereview.chromium.org/637323005/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java:45: * guaranteed to be called only once and only for the most reset query. s/most reset/most recent https://codereview.chromium.org/637323005/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java:47: void onSearchResultReturned(String query, List<BookmarkId> results); s/onSearchResultReturned/onSearchResultsReturned since comments and parameters always mention |results|. https://codereview.chromium.org/637323005/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java:98: * Send request to search server for querying related bookmarks. s/Send/Sends https://codereview.chromium.org/637323005/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java:106: * Registers a SearchObserver to listen for filter change notifications. Mentioning filters incorrectly here. https://codereview.chromium.org/637323005/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java:114: * Unregisters a SearchObserver from listening to filter change notifications. Ditto. https://codereview.chromium.org/637323005/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java:163: private native void nativeGetSearchResults(long nativeEnhancedBookmarksBridge, Seems unused. https://codereview.chromium.org/637323005/diff/40001/chrome/browser/android/e... File chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.h (right): https://codereview.chromium.org/637323005/diff/40001/chrome/browser/android/e... chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.h:55: BookmarkServerSearchService* search_service_; You could use either a scoped_ptr or not making it a pointer at all, no? https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... File components/enhanced_bookmarks/bookmark_server_search_service.h (right): https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.h:33: // only be called for the last query. This comment contradicts the previous sentence: "OnChange() is garanteed to be called once per query." Now this is not true anymore. https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.h:39: std::vector<const BookmarkNode*> ResultForQuery(const std::string& query); With the new API you introduce, will we ever use this one? Might as well remove it. Or private for the time being since you use it in this class. https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.h:41: // Returns search retults for most recent query. s/retults/results https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.h:42: std::vector<const BookmarkNode*> ResultForQuery(); s/ResultForQuery/ResultsForLastQuery or ResultsForMostRecentQuery or ResultsForCurrentQuery https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.h:65: // The search data, a map from query to a vector of stars.id. s/map/cache?
This CL tries to do 3 things at once, only one of them I agree with: - The MRU changes: +1 - Added API to get to the current_query_: I'm not in favor of this added API that changes the meaning of current_query_. This variable is only there to store the result of a search, not to use as a crutch for the client of this class. The client should have the string it asked for in the first place, there is no need to ask for it again. The method currentQuery() could be added, but would only returns the query currently fetching, and an empty string if the fetch is finished. ResultForQuery() should be eliminated. - Checking is results are present: As Kibeom pointed out there is a need for a HasResultForQuery() method which should return one of two values: no results or results are here. In combination with currentQuery() this will give a client full visibility on the state of a query (not here, in progress, here). Without this method this class is quite hard to use I agree. As for the change in the Search method, I'm not against or for, I see some value in checking in the cache there. https://codereview.chromium.org/637323005/diff/40001/chrome/browser/android/e... File chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc (right): https://codereview.chromium.org/637323005/diff/40001/chrome/browser/android/e... chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc:52: delete search_service_; There should be no delete in Chrome code, please use a scoped_ptr instead. https://codereview.chromium.org/637323005/diff/40001/chrome/browser/android/e... File chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.h (right): https://codereview.chromium.org/637323005/diff/40001/chrome/browser/android/e... chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.h:55: BookmarkServerSearchService* search_service_; I'm not sure it is the right scope here. The search service should be short lived, as soon as the UI which needs it goes away it should be deleted. By tying up to the enhancedBookmark this implies that it will stay up for the life of the app? If this is the case, this is not its intended use. I would suggest giving the search service a bridge of its own, which is released ASAP. https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... File components/enhanced_bookmarks/bookmark_server_search_service.cc (left): https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.cc:94: current_query_.clear(); Again, I'm not convinced the lifecycle of current_query needs to change. https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... File components/enhanced_bookmarks/bookmark_server_search_service.cc (right): https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.cc:29: searches_ = new base::MRUCache<std::string, std::vector<std::string> >( On 2014/10/29 07:51:47, Kibeom Kim wrote: > nit: I think now we can use >> instead of > > Better, don't use new and delete at all, make it a scoped_ptr. Bestest(!), just don't make it a pointer at all. https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.cc:38: DCHECK(query.length()); Add if (query == current_query_) return; to protect against the edge case of cancelling a query that's actually needed. https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.cc:39: current_query_ = query; Move after the if, only set it if a request is to be made. https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.cc:55: base::MRUCache<std::string, std::vector<std::string> >::iterator it = use auto? At least remove the space between the two '>'. https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.cc:62: ++clip_it) { for (const std::string& clip_id : it->second) https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.cc:107: const std::string& clip_id = it->clip_id(); Replace this whole mess by for (const std::string& clip_id : response_proto.results()) { https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.cc:117: searches_->Clear(); Bug: This should clear the current_query as well. https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.cc:122: searches_->Clear(); What if there is a query in progress at that point? The code as it is will just store it. Maybe it should cancel the fetching part of the query and restart it? Maybe not in this CL. https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... File components/enhanced_bookmarks/bookmark_server_search_service.h (right): https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.h:39: std::vector<const BookmarkNode*> ResultForQuery(const std::string& query); On 2014/10/29 10:31:35, lpromero wrote: > With the new API you introduce, will we ever use this one? Might as well remove > it. Or private for the time being since you use it in this class. Probably add HasResultForQuery() returning a boolean, and adding a DCHECK in resultForQuery. https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.h:42: std::vector<const BookmarkNode*> ResultForQuery(); On 2014/10/29 10:31:35, lpromero wrote: > s/ResultForQuery/ResultsForLastQuery or ResultsForMostRecentQuery or > ResultsForCurrentQuery I'm curious why you think you need this API? I'm reluctant to approve it as it is not very well defined what the last query is. It could be easy for the UI and the last_query to get out of sync. The UI will have the query somewhere anyway as the user sees it, so why duplicate this info? As it is the current_query_ is short lived, only used to build the query and then discarded. Your CL keeps it, and I'm not sure what its life cycle it now. https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.h:45: std::string GetCurrentQuery(); GetSomething is a javaism. Just use CurrentQuery() instead. But I'd actually favor not adding this API at all. https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.h:67: std::string current_query_; Add a comment to this variable: current_query_ holds the query currently in flight, and is cleared as soon as the result is available.
https://codereview.chromium.org/637323005/diff/1/chrome/browser/android/enhan... File chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.h (right): https://codereview.chromium.org/637323005/diff/1/chrome/browser/android/enhan... chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.h:59: BookmarkServerSearchService* search_service_; On 2014/10/29 07:51:46, Kibeom Kim wrote: > On 2014/10/29 00:11:39, Ian Wen wrote: > > On 2014/10/22 18:42:01, Kibeom Kim wrote: > > > Let's make it scoped_ptr > > > > This service should never be deleted. > > But it looks you're deleting in enhanced_bookmarks_bridge.cc ? > And also, what's the reason it shouldn't be deleted? Made it scoped ptr https://codereview.chromium.org/637323005/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java (right): https://codereview.chromium.org/637323005/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java:45: * guaranteed to be called only once and only for the most reset query. On 2014/10/29 10:31:35, lpromero wrote: > s/most reset/most recent Done. https://codereview.chromium.org/637323005/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java:47: void onSearchResultReturned(String query, List<BookmarkId> results); On 2014/10/29 10:31:34, lpromero wrote: > s/onSearchResultReturned/onSearchResultsReturned since comments and parameters > always mention |results|. Done. https://codereview.chromium.org/637323005/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java:98: * Send request to search server for querying related bookmarks. On 2014/10/29 10:31:35, lpromero wrote: > s/Send/Sends Done. https://codereview.chromium.org/637323005/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java:106: * Registers a SearchObserver to listen for filter change notifications. On 2014/10/29 10:31:35, lpromero wrote: > Mentioning filters incorrectly here. Done. https://codereview.chromium.org/637323005/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java:114: * Unregisters a SearchObserver from listening to filter change notifications. On 2014/10/29 10:31:35, lpromero wrote: > Ditto. Done. https://codereview.chromium.org/637323005/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java:163: private native void nativeGetSearchResults(long nativeEnhancedBookmarksBridge, On 2014/10/29 10:31:34, lpromero wrote: > Seems unused. Done. https://codereview.chromium.org/637323005/diff/40001/chrome/browser/android/e... File chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc (right): https://codereview.chromium.org/637323005/diff/40001/chrome/browser/android/e... chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc:52: delete search_service_; On 2014/10/29 12:49:48, noyau wrote: > There should be no delete in Chrome code, please use a scoped_ptr instead. Done. https://codereview.chromium.org/637323005/diff/40001/chrome/browser/android/e... File chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.h (right): https://codereview.chromium.org/637323005/diff/40001/chrome/browser/android/e... chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.h:55: BookmarkServerSearchService* search_service_; On 2014/10/29 12:49:48, noyau wrote: > I'm not sure it is the right scope here. The search service should be short > lived, as soon as the UI which needs it goes away it should be deleted. By tying > up to the enhancedBookmark this implies that it will stay up for the life of the > app? If this is the case, this is not its intended use. > > I would suggest giving the search service a bridge of its own, which is released > ASAP. Here search_service has a MRUCache in it for faster results. Maybe at least for this reason we should let the service to live a bit longer. I would make the service scoped here. ;) https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... File components/enhanced_bookmarks/bookmark_server_search_service.cc (left): https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.cc:94: current_query_.clear(); On 2014/10/29 12:49:48, noyau wrote: > Again, I'm not convinced the lifecycle of current_query needs to change. Done. Put it back. https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... File components/enhanced_bookmarks/bookmark_server_search_service.cc (right): https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.cc:15: const int kMRUCacheMaxSize = 50; On 2014/10/29 07:51:47, Kibeom Kim wrote: > I think it's better to indicate what this variable is for, in the name. Maybe > kSearchCacheMaxSize. Done. https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.cc:29: searches_ = new base::MRUCache<std::string, std::vector<std::string> >( On 2014/10/29 12:49:48, noyau wrote: > On 2014/10/29 07:51:47, Kibeom Kim wrote: > > nit: I think now we can use >> instead of > > > > Better, don't use new and delete at all, make it a scoped_ptr. Bestest(!), just > don't make it a pointer at all. LOL "Bestest". Made it not a pointer. Done. https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.cc:38: DCHECK(query.length()); On 2014/10/29 12:49:48, noyau wrote: > Add > > if (query == current_query_) > return; > > to protect against the edge case of cancelling a query that's actually needed. Done. https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.cc:39: current_query_ = query; On 2014/10/29 12:49:48, noyau wrote: > Move after the if, only set it if a request is to be made. Done. https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.cc:55: base::MRUCache<std::string, std::vector<std::string> >::iterator it = On 2014/10/29 12:49:48, noyau wrote: > use auto? At least remove the space between the two '>'. Done. https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.cc:62: ++clip_it) { On 2014/10/29 12:49:48, noyau wrote: > for (const std::string& clip_id : it->second) Done. https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.cc:107: const std::string& clip_id = it->clip_id(); On 2014/10/29 12:49:48, noyau wrote: > Replace this whole mess by > > for (const std::string& clip_id : response_proto.results()) { Done. https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.cc:117: searches_->Clear(); On 2014/10/29 12:49:48, noyau wrote: > Bug: This should clear the current_query as well. Done. https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... File components/enhanced_bookmarks/bookmark_server_search_service.h (right): https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.h:33: // only be called for the last query. On 2014/10/29 10:31:35, lpromero wrote: > This comment contradicts the previous sentence: > "OnChange() is garanteed to be called once per query." > Now this is not true anymore. Done. https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.h:41: // Returns search retults for most recent query. On 2014/10/29 10:31:35, lpromero wrote: > s/retults/results Done. https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.h:42: std::vector<const BookmarkNode*> ResultForQuery(); On 2014/10/29 12:49:48, noyau wrote: > On 2014/10/29 10:31:35, lpromero wrote: > > s/ResultForQuery/ResultsForLastQuery or ResultsForMostRecentQuery or > > ResultsForCurrentQuery > > I'm curious why you think you need this API? I'm reluctant to approve it as it > is not very well defined what the last query is. It could be easy for the UI and > the last_query to get out of sync. The UI will have the query somewhere anyway > as the user sees it, so why duplicate this info? > > As it is the current_query_ is short lived, only used to build the query and > then discarded. Your CL keeps it, and I'm not sure what its life cycle it now. The reason why I made the server to remember the latest query is that I noticed the fact that as long as UI and the service run on the same thread (which is the case on Android, not sure about iOS). Then this service will only call onChange() for the last query. For example, if UI searches Apple first and before the result of Apple ever arrives, UI seaches Banana. Then if onChange() is called in future, it will be meant for Banana only (as we destroy the URL_fetcher for previous searches). Pros for this approach: less function call stack between UI and server (one round trip instead of three). Cons: 1. Restriction of the usage of this class as other classes can only retrieve results for the most recent query using this ResultForQuery(). 2. This approach assumes frontend and backend runs on same thread and URL_Fetcher does what it should do, which may fail? After careful consideration I decided to hold back changes that I made about keeping current_query_ in server, in order to help this CL land faster. We are targeting M40, isn't it? :) Still there are two things that we all agree: 1. Introduce cache to reduce time for repeated search queries. 2. Make the service live a bit longer so that we could fully utilize this cache. https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.h:45: std::string GetCurrentQuery(); On 2014/10/29 12:49:48, noyau wrote: > GetSomething is a javaism. Just use CurrentQuery() instead. But I'd actually > favor not adding this API at all. Done. https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.h:65: // The search data, a map from query to a vector of stars.id. On 2014/10/29 10:31:35, lpromero wrote: > s/map/cache? Done. https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.h:66: base::MRUCache<std::string, std::vector<std::string> >* searches_; On 2014/10/29 07:51:47, Kibeom Kim wrote: > Since the lifetime of |searches_| is the scope of this class, it doesn't have to > be a pointer. Done. https://codereview.chromium.org/637323005/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.h:67: std::string current_query_; On 2014/10/29 12:49:48, noyau wrote: > Add a comment to this variable: current_query_ holds the query currently in > flight, and is cleared as soon as the result is available. Done.
lgtm. Please wait for Eric and Louis' lgtms 1. I'm not sure we still need to add |HasResultForQuery| because now |ResultForQuery| can tell us cache miss by returning null. 2. Just a note: so on Android, it should go like... On text change, check if the result is in the cache by calling |getSearchResultsForQuery|. - true : Display the result. - false : Wait 200ms and call BookmarkServerSearchService::Search if the text is still unchanged. https://chrome-internal-review.googlesource.com/#/c/180705/6/java/apps/chrome... https://codereview.chromium.org/637323005/diff/40001/chrome/browser/android/e... File chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.h (right): https://codereview.chromium.org/637323005/diff/40001/chrome/browser/android/e... chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.h:55: BookmarkServerSearchService* search_service_; On 2014/10/29 12:49:48, noyau wrote: > I'm not sure it is the right scope here. The search service should be short > lived, as soon as the UI which needs it goes away it should be deleted. By tying > up to the enhancedBookmark this implies that it will stay up for the life of the > app? We destroy EnhancedBookmarksBridge once the bookmark UI dialog is closed. Maybe destroying |search_service_| at closing search UI is more ideal but I think the current lifetime is acceptable. https://codereview.chromium.org/637323005/diff/60001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java (right): https://codereview.chromium.org/637323005/diff/60001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java:125: * Unregisters Registers a SearchObserver that listens to search request updates. Remove "Registers" ? https://codereview.chromium.org/637323005/diff/60001/chrome/browser/android/e... File chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.h (right): https://codereview.chromium.org/637323005/diff/60001/chrome/browser/android/e... chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.h:57: EnhancedBookmarkModel* enhanced_bookmark_model_; Let's keep "// weak" comment. https://codereview.chromium.org/637323005/diff/60001/chrome/browser/android/e... chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.h:59: scoped_ptr<BookmarkServerSearchService> search_service_; Actually, seems like search_service_ can be just a value type, not scoped_ptr. https://codereview.chromium.org/637323005/diff/60001/components/enhanced_book... File components/enhanced_bookmarks/bookmark_server_search_service.cc (right): https://codereview.chromium.org/637323005/diff/60001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.cc:45: } Just a note: In practice, this seems redundant since UI code will call |ResultForQuery| first to see if we have the result already. Though, I think it's good to keep this code. https://codereview.chromium.org/637323005/diff/60001/components/enhanced_book... File components/enhanced_bookmarks/bookmark_server_search_service.h (right): https://codereview.chromium.org/637323005/diff/60001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.h:38: // returns null pointer, it means we are still loading and no results have We aren't necessarily loading if |Search| has not been called before for the query. https://codereview.chromium.org/637323005/diff/60001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.h:64: base::MRUCache<std::string, std::vector<std::string> > cache_; nit: >>
https://codereview.chromium.org/637323005/diff/60001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java (right): https://codereview.chromium.org/637323005/diff/60001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java:125: * Unregisters Registers a SearchObserver that listens to search request updates. On 2014/10/30 03:13:11, Kibeom Kim wrote: > Remove "Registers" ? Done. https://codereview.chromium.org/637323005/diff/60001/chrome/browser/android/e... File chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.h (right): https://codereview.chromium.org/637323005/diff/60001/chrome/browser/android/e... chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.h:57: EnhancedBookmarkModel* enhanced_bookmark_model_; On 2014/10/30 03:13:11, Kibeom Kim wrote: > Let's keep "// weak" comment. Done. https://codereview.chromium.org/637323005/diff/60001/chrome/browser/android/e... chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.h:59: scoped_ptr<BookmarkServerSearchService> search_service_; On 2014/10/30 03:13:11, Kibeom Kim wrote: > Actually, seems like search_service_ can be just a value type, not scoped_ptr. Yes, but initializing it is such a mess that maybe we should not add after the colon of the constructor. Plus Louis used to mention that he is about to create a factory for this service, so use a pointer here is somewhat legit. https://codereview.chromium.org/637323005/diff/60001/components/enhanced_book... File components/enhanced_bookmarks/bookmark_server_search_service.cc (right): https://codereview.chromium.org/637323005/diff/60001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.cc:45: } On 2014/10/30 03:13:11, Kibeom Kim wrote: > Just a note: In practice, this seems redundant since UI code will call > |ResultForQuery| first to see if we have the result already. Though, I think > it's good to keep this code. I am afraid this is not how we are using this class. UI will call search(Banana), and onChange() will be called synchronously (if it hits the cache) and UI will then call ResultForQuery(Banana) to get a list of results. Calling resultForQuery() directly does not make much sense, isn't it? If there has been no query, why should there be any result? :) What I am saying, in conclusion, is that whenever the UI wants to search for something, it should always call Search(SomeFruit). And it is the bridge/model's responsibility to call ResultForQuery() later after onChange callback is triggered. Details of how we use this class on Android can be found at: https://chrome-internal-review.googlesource.com/#/c/180705/ https://codereview.chromium.org/637323005/diff/60001/components/enhanced_book... File components/enhanced_bookmarks/bookmark_server_search_service.h (right): https://codereview.chromium.org/637323005/diff/60001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.h:38: // returns null pointer, it means we are still loading and no results have On 2014/10/30 03:13:11, Kibeom Kim wrote: > We aren't necessarily loading if |Search| has not been called before for the > query. > Yes. https://codereview.chromium.org/637323005/diff/60001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.h:64: base::MRUCache<std::string, std::vector<std::string> > cache_; On 2014/10/30 03:13:11, Kibeom Kim wrote: > nit: >> Done.
https://codereview.chromium.org/637323005/diff/60001/components/enhanced_book... File components/enhanced_bookmarks/bookmark_server_search_service.cc (right): https://codereview.chromium.org/637323005/diff/60001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.cc:45: } On 2014/10/30 04:31:00, Ian Wen wrote: > On 2014/10/30 03:13:11, Kibeom Kim wrote: > > Just a note: In practice, this seems redundant since UI code will call > > |ResultForQuery| first to see if we have the result already. Though, I think > > it's good to keep this code. > > I am afraid this is not how we are using this class. UI will call > search(Banana), and onChange() will be called synchronously (if it hits the > cache) and UI will then call ResultForQuery(Banana) to get a list of results. > Calling resultForQuery() directly does not make much sense, isn't it? If there > has been no query, why should there be any result? :) > > What I am saying, in conclusion, is that whenever the UI wants to search for > something, it should always call Search(SomeFruit). And it is the bridge/model's > responsibility to call ResultForQuery() later after onChange callback is > triggered. > > Details of how we use this class on Android can be found at: > https://chrome-internal-review.googlesource.com/#/c/180705/ Yes, so we should change the downstream CL(I didn't send comments yet there). If the result is already in the cache, there is no point at waiting 200ms so we should not wait. But the reason you put the 200ms delay is to avoid issuing too many search queries. So we need a way to check if the cache has the result, but not issuing search function, and that is your current |ResultForQuery| implementation, although maybe you didn't intend explicitly.
On 2014/10/30 04:39:05, Kibeom Kim wrote: > https://codereview.chromium.org/637323005/diff/60001/components/enhanced_book... > File components/enhanced_bookmarks/bookmark_server_search_service.cc (right): > > https://codereview.chromium.org/637323005/diff/60001/components/enhanced_book... > components/enhanced_bookmarks/bookmark_server_search_service.cc:45: } > On 2014/10/30 04:31:00, Ian Wen wrote: > > On 2014/10/30 03:13:11, Kibeom Kim wrote: > > > Just a note: In practice, this seems redundant since UI code will call > > > |ResultForQuery| first to see if we have the result already. Though, I think > > > it's good to keep this code. > > > > I am afraid this is not how we are using this class. UI will call > > search(Banana), and onChange() will be called synchronously (if it hits the > > cache) and UI will then call ResultForQuery(Banana) to get a list of results. > > Calling resultForQuery() directly does not make much sense, isn't it? If there > > has been no query, why should there be any result? :) > > > > What I am saying, in conclusion, is that whenever the UI wants to search for > > something, it should always call Search(SomeFruit). And it is the > bridge/model's > > responsibility to call ResultForQuery() later after onChange callback is > > triggered. > > > > Details of how we use this class on Android can be found at: > > https://chrome-internal-review.googlesource.com/#/c/180705/ > > Yes, so we should change the downstream CL(I didn't send comments yet there). If > the result is already in the cache, there is no point at waiting 200ms so we > should not wait. But the reason you put the 200ms delay is to avoid issuing too > many search queries. So we need a way to check if the cache has the result, but > not issuing search function, and that is your current |ResultForQuery| > implementation, although maybe you didn't intend explicitly. I tried this once. There will be flashes on screen when user types fast, if we do not wait for an amount of time. Say you are typing "joke", for my test bookmark model, "j" will return one result about David Fincher, "jo" and "jok" will reset the list to show empty "Star" icon, and "joke" will return page for www.joke.com. That is three flashes while typing. Also cleer@ used to mention that if we ever wanna show loading icon, we should make sure loading icon is shown for sufficient amount of time so that their will be no flash if loading is quick.
+Mark, Yaron On 2014/10/30 05:02:38, Ian Wen wrote: > > I tried this once. There will be flashes on screen when user types fast, if we > do not wait for an amount of time. Say you are typing "joke", for my test > bookmark model, "j" will return one result about David Fincher, "jo" and "jok" > will reset the list to show empty "Star" icon, and "joke" will return page for > http://www.joke.com. That is three flashes while typing. Also cleer@ used to mention > that if we ever wanna show loading icon, we should make sure loading icon is > shown for sufficient amount of time so that their will be no flash if loading is > quick. Oh, I see. I think flashing itself shouldn't be a problem. Current iOS local search is like that and also Chrome's omnibox is updated very character too. But I didn't know that server search is not incremental. On desktop it looks like they update the suggested-list every character but we don't have it yet. In the current form on mobile, I don't see any reason to prefer server search over the iOS incremental local search.
lgtm https://codereview.chromium.org/637323005/diff/80001/components/enhanced_book... File components/enhanced_bookmarks/bookmark_server_search_service.h (right): https://codereview.chromium.org/637323005/diff/80001/components/enhanced_book... components/enhanced_bookmarks/bookmark_server_search_service.h:44: const std::string& query); Why did you change from std::vector to a scoped_ptr here?
On 2014/10/30 10:38:40, noyau wrote: > lgtm > > https://codereview.chromium.org/637323005/diff/80001/components/enhanced_book... > File components/enhanced_bookmarks/bookmark_server_search_service.h (right): > > https://codereview.chromium.org/637323005/diff/80001/components/enhanced_book... > components/enhanced_bookmarks/bookmark_server_search_service.h:44: const > std::string& query); > Why did you change from std::vector to a scoped_ptr here? Ignore this, just read kkimlabs@ note about returning null for inexistent results. This should go in the comment on the .h.
The CQ bit was checked by lpromero@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/637323005/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/75752) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) win_chromium_rel_ng on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by ianwen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/637323005/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/e52346f194e9f2d097b4f6d3fbfa359eca7c92df Cr-Commit-Position: refs/heads/master@{#302107}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/688883002/ by kbr@chromium.org. The reason for reverting is: Broke compilation in http://build.chromium.org/p/chromium.gpu/builders/Android%20Debug%20%28Nexus%... . |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
