Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
695493005:
Add local search API to Enhanced Bookmark Bridge (Closed)
DescriptionAdd local search API to Enhanced Bookmark Bridge
Local search function has the same logic with how omnibox retrieves
bookmarks for a given keyword.
BUG=415774
Committed: https://crrev.com/f910bb982fcc393438d59afb4d03fc8c49a316e2
Cr-Commit-Position: refs/heads/master@{#302349}
Patch Set 1 #Patch Set 2 : nits #
Total comments: 8
Patch Set 3 : move a function to bookmarks bridge #
Total comments: 5
Patch Set 4 : #
Total comments: 15
Patch Set 5 : format code better #Patch Set 6 : make it compile #Patch Set 7 : #Patch Set 8 : rename function #
Total comments: 4
Patch Set 9 : #
Messages
Total messages: 22 (3 generated)
ianwen@chromium.org changed reviewers: + kkimlabs@chromium.org, lpromero@chromium.org, tedchoc@chromium.org
https://codereview.chromium.org/695493005/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java (right): https://codereview.chromium.org/695493005/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java:154: */ Refer to BookmarkModel::GetBookmarksMatching in the comment? I feel like this comment could be better worded, but well, my English is rubbish so I'll let others to suggest if there is anything to improve. https://codereview.chromium.org/695493005/diff/20001/chrome/browser/android/e... File chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc (right): https://codereview.chromium.org/695493005/diff/20001/chrome/browser/android/e... chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc:221: jstring j_query) { This function doesn't use any enhanced bookmark feature so it should go BookmarksBridge. I think EnhancedBookmarksBridge should be a place for enhanced bookmarks features, not a collection of what we use in our enhanced bookmark dialog. If so, we should move BookmarksBridge#getOtherFolderId() EnhancedBookmarksBridge because we don't call that in our old bookmark UI. But then it just introduces unnecessary code dependency. https://codereview.chromium.org/695493005/diff/20001/chrome/browser/android/e... chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc:227: const size_t kMaxBookmarkMatches = 1000; This is quite arbitrary. Let's make it a function argument or std::numeric_limits<size_t>::max() or BookmarkModel's bookmark count if available.
kkimlabs@chromium.org changed reviewers: + danduong@chromium.org
https://codereview.chromium.org/695493005/diff/20001/chrome/browser/android/e... File chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc (right): https://codereview.chromium.org/695493005/diff/20001/chrome/browser/android/e... chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc:228: enhanced_bookmark_model_->bookmark_model()->GetBookmarksMatching( As per email thread on mobile-stars, this search function will not return anything when ANY term in the query is less than 3 characters and does not match exactly (ie is equal, not a substring) of a term in the titles. Do we really want to use this? The resulting UX feels really weird. On iOS, I will put a flag for this search and let people see what they think of both.
Hey Louis, could you please try renaming a bookmark to be "v" and then
search for "v" using that function again?
Quote the source code: "Term is too short for prefix matching, compare
using exact match".
bool BookmarkIndex::GetBookmarksMatchingTerm(const base::string16&
term, bool
first_term,
Matches* matches) { Index::const_iterator i =
index_.lower_bound(term); if (i == index_.end()) return false; if
(!query_parser::QueryParser::IsWordLongEnoughForPrefixSearch(term)) {
// Term is too short for prefix match, compare using exact match.
if (i->first != term) return false; // No bookmarks with this
term. if (first_term) { Match match;
match.terms.push_back(i); matches->push_back(match); return
true; }
On Fri Oct 31 2014 at 9:27:39 AM <lpromero@chromium.org> wrote:
>
> https://codereview.chromium.org/695493005/diff/20001/
> chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc
> File
> chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc
> (right):
>
> https://codereview.chromium.org/695493005/diff/20001/
> chrome/browser/android/enhanced_bookmarks/enhanced_
> bookmarks_bridge.cc#newcode228
> chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc:228
>
<https://codereview.chromium.org/695493005/diff/20001/chrome/browser/android/e...>
> :
> enhanced_bookmark_model_->bookmark_model()->GetBookmarksMatching(
> As per email thread on mobile-stars, this search function will not
> return anything when ANY term in the query is less than 3 characters and
> does not match exactly (ie is equal, not a substring) of a term in the
> titles.
>
> Do we really want to use this? The resulting UX feels really weird.
>
> On iOS, I will put a flag for this search and let people see what they
> think of both.
>
> https://codereview.chromium.org/695493005/
>
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.
On 2014/10/31 16:55:42, Ian Wen wrote:
> Hey Louis, could you please try renaming a bookmark to be "v" and then
> search for "v" using that function again?
>
> Quote the source code: "Term is too short for prefix matching, compare
> using exact match".
>
> bool BookmarkIndex::GetBookmarksMatchingTerm(const base::string16&
> term, bool
> first_term,
> Matches* matches) { Index::const_iterator i =
> index_.lower_bound(term); if (i == index_.end()) return false; if
> (!query_parser::QueryParser::IsWordLongEnoughForPrefixSearch(term)) {
> // Term is too short for prefix match, compare using exact match.
> if (i->first != term) return false; // No bookmarks with this
> term. if (first_term) { Match match;
> match.terms.push_back(i); matches->push_back(match); return
> true; }
>
>
> On Fri Oct 31 2014 at 9:27:39 AM <mailto:lpromero@chromium.org> wrote:
>
> >
> > https://codereview.chromium.org/695493005/diff/20001/
> > chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc
> > File
> > chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc
> > (right):
> >
> > https://codereview.chromium.org/695493005/diff/20001/
> > chrome/browser/android/enhanced_bookmarks/enhanced_
> > bookmarks_bridge.cc#newcode228
> > chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc:228
> >
>
<https://codereview.chromium.org/695493005/diff/20001/chrome/browser/android/e...>
> > :
> > enhanced_bookmark_model_->bookmark_model()->GetBookmarksMatching(
> > As per email thread on mobile-stars, this search function will not
> > return anything when ANY term in the query is less than 3 characters and
> > does not match exactly (ie is equal, not a substring) of a term in the
> > titles.
> >
> > Do we really want to use this? The resulting UX feels really weird.
> >
> > On iOS, I will put a flag for this search and let people see what they
> > think of both.
> >
> > https://codereview.chromium.org/695493005/
> >
>
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
That's what I meant by "does not match exactly" for less than 3 characters
strings.
The exact matching for less than 3 is useless to us, in 99% of cases.
I am working on modifying its behavior in bookmark_index.cc. When I demo the current implementation to my manager Ted, he was okay with the 3-letter thing. If you guys oppose to it, we can definitely improve it. https://codereview.chromium.org/695493005/diff/20001/chrome/browser/android/e... File chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc (right): https://codereview.chromium.org/695493005/diff/20001/chrome/browser/android/e... chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc:221: jstring j_query) { On 2014/10/31 09:13:51, Kibeom Kim wrote: > This function doesn't use any enhanced bookmark feature so it should go > BookmarksBridge. > > I think EnhancedBookmarksBridge should be a place for enhanced bookmarks > features, not a collection of what we use in our enhanced bookmark dialog. If > so, we should move BookmarksBridge#getOtherFolderId() EnhancedBookmarksBridge > because we don't call that in our old bookmark UI. But then it just introduces > unnecessary code dependency. Done. https://codereview.chromium.org/695493005/diff/20001/chrome/browser/android/e... chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc:227: const size_t kMaxBookmarkMatches = 1000; On 2014/10/31 09:13:51, Kibeom Kim wrote: > This is quite arbitrary. Let's make it a function argument or > std::numeric_limits<size_t>::max() or BookmarkModel's bookmark count if > available. Basically we have 3 options here. 1. A number that is too large for the user to browse.(1000 here) 2. Total number of children of root node. 3. Maximum number allowed by size_t. Option 2 and 3 are okay in theory, but option 1 might be a more practical choice here. Having 1000 result or 10000 looks exactly the same to a mobile user (scroll bar is already at minimum size, and user is definitely not going to browse these 1000 results to find something). Also this kind of ensures that even if user search for something really broad terms like "com", it still does not kill the performance. Correct me if I am wrong. https://codereview.chromium.org/695493005/diff/20001/chrome/browser/android/e... chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc:228: enhanced_bookmark_model_->bookmark_model()->GetBookmarksMatching( On 2014/10/31 16:27:36, lpromero wrote: > As per email thread on mobile-stars, this search function will not return > anything when ANY term in the query is less than 3 characters and does not match > exactly (ie is equal, not a substring) of a term in the titles. > > Do we really want to use this? The resulting UX feels really weird. > > On iOS, I will put a flag for this search and let people see what they think of > both. The function we used can be modified if we really like to have result for only one/two letters. I am okay with both.
https://codereview.chromium.org/695493005/diff/20001/chrome/browser/android/e... File chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc (right): https://codereview.chromium.org/695493005/diff/20001/chrome/browser/android/e... chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc:227: const size_t kMaxBookmarkMatches = 1000; On 2014/10/31 17:46:14, Ian Wen wrote: > On 2014/10/31 09:13:51, Kibeom Kim wrote: > > This is quite arbitrary. Let's make it a function argument or > > std::numeric_limits<size_t>::max() or BookmarkModel's bookmark count if > > available. > > Basically we have 3 options here. > 1. A number that is too large for the user to browse.(1000 here) > 2. Total number of children of root node. > 3. Maximum number allowed by size_t. > > Option 2 and 3 are okay in theory, but option 1 might be a more practical choice > here. Having 1000 result or 10000 looks exactly the same to a mobile user > (scroll bar is already at minimum size, and user is definitely not going to > browse these 1000 results to find something). Also this kind of ensures that > even if user search for something really broad terms like "com", it still does > not kill the performance. > > Correct me if I am wrong. Yes I agree that it's good to do such optimization, but that should be up to UI code, not built into the API, so that's why I suggested making as an argument so that UI code can decide what it requires. If we're going to trim the list, I think we should show something like "..." at the end of the list to let user know there is actually more. Or better, Maybe something like: 1. For search, request 100 bookmarks. 2. If user scrolls down below half, request 500 bookmarks 3. repeat 2 and increase the number. https://codereview.chromium.org/695493005/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java (right): https://codereview.chromium.org/695493005/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:637: } you can use JavaBookmarkIdCreateBookmarkId inside bookmark_id.h function. https://codereview.chromium.org/695493005/diff/40001/chrome/browser/android/b... File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right): https://codereview.chromium.org/695493005/diff/40001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:673: node->id(), node->type()); nit: indent fix?
https://codereview.chromium.org/695493005/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java (right): https://codereview.chromium.org/695493005/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:637: } On 2014/10/31 18:04:42, Kibeom Kim wrote: > you can use JavaBookmarkIdCreateBookmarkId inside bookmark_id.h function. This returns a list, not an ID??? https://codereview.chromium.org/695493005/diff/40001/chrome/browser/android/b... File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right): https://codereview.chromium.org/695493005/diff/40001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:673: node->id(), node->type()); On 2014/10/31 18:04:42, Kibeom Kim wrote: > nit: indent fix? Done.
lgtm Ted : ptal https://codereview.chromium.org/695493005/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java (right): https://codereview.chromium.org/695493005/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:637: } On 2014/10/31 20:31:23, Ian Wen wrote: > On 2014/10/31 18:04:42, Kibeom Kim wrote: > > you can use JavaBookmarkIdCreateBookmarkId inside bookmark_id.h function. > > This returns a list, not an ID??? oops, sorry my mistake!
https://codereview.chromium.org/695493005/diff/60001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java (right): https://codereview.chromium.org/695493005/diff/60001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:377: * method uses the same logic with omnibox to fetch related bookmarks and is thus synchronous. Don't describe the logic behavior as you should be able to change it underlying without updating the comment. I think simply: Synchronously gets a list of bookmarks that match the specified search query. For the other stuff, I would just let them read the c++ code for implementation information. https://codereview.chromium.org/695493005/diff/60001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:380: * @param maxNumberOfResult Maximum number of result to fetch in case the user searches for I wouldn't add the stuff after fetch [in case the user searches...]. I don't think it adds anything. https://codereview.chromium.org/695493005/diff/60001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:384: public List<BookmarkId> getLocalSearchResults(String query, int maxNumberOfResult) { I think this should just be called getSearchResults (or getBookmarksMatchingQuery). This class has no concept of local/remote so no need to add that now. https://codereview.chromium.org/695493005/diff/60001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:636: private static List<BookmarkId> createBookmarkIdList() { all other places pass in a newed list, so either do that or convert them all to use this. https://codereview.chromium.org/695493005/diff/60001/chrome/browser/android/b... File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right): https://codereview.chromium.org/695493005/diff/60001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:665: Java_BookmarksBridge_createBookmarkIdList(env); indented too much, and create this above the for loop below since that is where it's used https://codereview.chromium.org/695493005/diff/60001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:669: max_results, &results); &results needs to go on it's own line https://codereview.chromium.org/695493005/diff/60001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:672: Java_BookmarksBridge_addToBookmarkIdList(env, j_list.obj(), node->id(), If all the params don't fit on a single line, they all go on their own. With that said, all the params should fit on the second line.
https://codereview.chromium.org/695493005/diff/60001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java (right): https://codereview.chromium.org/695493005/diff/60001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:377: * method uses the same logic with omnibox to fetch related bookmarks and is thus synchronous. On 2014/10/31 20:53:07, Ted C wrote: > Don't describe the logic behavior as you should be able to change it underlying > without updating the comment. > > I think simply: > > Synchronously gets a list of bookmarks that match the specified search query. > > For the other stuff, I would just let them read the c++ code for implementation > information. Done. https://codereview.chromium.org/695493005/diff/60001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:380: * @param maxNumberOfResult Maximum number of result to fetch in case the user searches for On 2014/10/31 20:53:06, Ted C wrote: > I wouldn't add the stuff after fetch [in case the user searches...]. I don't > think it adds anything. Done. https://codereview.chromium.org/695493005/diff/60001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:384: public List<BookmarkId> getLocalSearchResults(String query, int maxNumberOfResult) { On 2014/10/31 20:53:06, Ted C wrote: > I think this should just be called getSearchResults (or > getBookmarksMatchingQuery). This class has no concept of local/remote so no > need to add that now. We have a server search counterpart in EnhancedBookmarksBridge.java, with the name of "getSearchResultsForQuery". Should we add "local" here to distinguish them? https://codereview.chromium.org/695493005/diff/60001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:636: private static List<BookmarkId> createBookmarkIdList() { On 2014/10/31 20:53:07, Ted C wrote: > all other places pass in a newed list, so either do that or convert them all to > use this. Yes you are right. It was initially wrote in a different way because of the server search structure. Now since it's synchronous, we should not have this sort of discrepancy anymore. Done. https://codereview.chromium.org/695493005/diff/60001/chrome/browser/android/b... File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right): https://codereview.chromium.org/695493005/diff/60001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:665: Java_BookmarksBridge_createBookmarkIdList(env); On 2014/10/31 20:53:07, Ted C wrote: > indented too much, and create this above the for loop below since that is where > it's used Done. https://codereview.chromium.org/695493005/diff/60001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:669: max_results, &results); On 2014/10/31 20:53:07, Ted C wrote: > &results needs to go on it's own line Done. https://codereview.chromium.org/695493005/diff/60001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:672: Java_BookmarksBridge_addToBookmarkIdList(env, j_list.obj(), node->id(), On 2014/10/31 20:53:07, Ted C wrote: > If all the params don't fit on a single line, they all go on their own. > > With that said, all the params should fit on the second line. Done. Eclipse auto formatter is no silver bullet. :(
https://codereview.chromium.org/695493005/diff/60001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java (right): https://codereview.chromium.org/695493005/diff/60001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:384: public List<BookmarkId> getLocalSearchResults(String query, int maxNumberOfResult) { On 2014/10/31 21:28:23, Ian Wen wrote: > On 2014/10/31 20:53:06, Ted C wrote: > > I think this should just be called getSearchResults (or > > getBookmarksMatchingQuery). This class has no concept of local/remote so no > > need to add that now. > > We have a server search counterpart in EnhancedBookmarksBridge.java, with the > name of "getSearchResultsForQuery". Should we add "local" here to distinguish > them? Done.
https://codereview.chromium.org/695493005/diff/140001/chrome/browser/android/... File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right): https://codereview.chromium.org/695493005/diff/140001/chrome/browser/android/... chrome/browser/android/bookmarks/bookmarks_bridge.cc:661: jint max_results) { nit: indent https://codereview.chromium.org/695493005/diff/140001/chrome/browser/android/... File chrome/browser/android/bookmarks/bookmarks_bridge.h (right): https://codereview.chromium.org/695493005/diff/140001/chrome/browser/android/... chrome/browser/android/bookmarks/bookmarks_bridge.h:125: jint max_results); nit: indent
https://codereview.chromium.org/695493005/diff/140001/chrome/browser/android/... File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right): https://codereview.chromium.org/695493005/diff/140001/chrome/browser/android/... chrome/browser/android/bookmarks/bookmarks_bridge.cc:661: jint max_results) { On 2014/10/31 22:23:21, Kibeom Kim wrote: > nit: indent Done. https://codereview.chromium.org/695493005/diff/140001/chrome/browser/android/... File chrome/browser/android/bookmarks/bookmarks_bridge.h (right): https://codereview.chromium.org/695493005/diff/140001/chrome/browser/android/... chrome/browser/android/bookmarks/bookmarks_bridge.h:125: jint max_results); On 2014/10/31 22:23:21, Kibeom Kim wrote: > nit: indent Done.
lgtm
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/695493005/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/f910bb982fcc393438d59afb4d03fc8c49a316e2 Cr-Commit-Position: refs/heads/master@{#302349} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
