Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
516323003:
[Android] JNI bridges for querying top level bookmark folders. (Closed)
|
Created:
6 years, 3 months ago by Kibeom Kim (inactive) Modified:
6 years, 3 months ago CC:
chromium-reviews, trchen Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Description[Android] JNI bridges for querying top level bookmark folders.
The new enhanced bookmark UI redefines what top folders are:
- Sub-folders of mobile node and others node.
- Desktop node.
- Managed bookmark node.
- Partner bookmark node.
So we need JNI bridge functions to query them.
BUG=386785
Committed: https://crrev.com/78cb62e9a8bf1af39b11a95b20a7b5a51482e2f5
Cr-Commit-Position: refs/heads/master@{#293291}
Patch Set 1 #Patch Set 2 : added missing other node. #Patch Set 3 : Remove debug log #
Total comments: 6
Patch Set 4 : use ICU to sort title #Patch Set 5 : addressed newt's nits #
Total comments: 8
Patch Set 6 : added comment #
Total comments: 8
Patch Set 7 : folder check #
Total comments: 2
Patch Set 8 : factory to a function #
Messages
Total messages: 31 (8 generated)
kkimlabs@chromium.org changed reviewers: + ianwen@chromium.org, newt@chromium.org, tedchoc@chromium.org
I like this better :) lgtm after some nits https://codereview.chromium.org/516323003/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java (right): https://codereview.chromium.org/516323003/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:194: * @return The top level folders. Note that normal top folders will be in the alphabetical do the special folders come before or after the normal folders? https://codereview.chromium.org/516323003/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:195: * order. Special top folders are managed bookmark root folder and partner bookmark remove "root" and "folder root folder" https://codereview.chromium.org/516323003/diff/40001/chrome/browser/android/b... File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right): https://codereview.chromium.org/516323003/diff/40001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:208: if (*it != NULL) { How could *it be NULL? Does GetChild(i) sometimes return NULL?
ptal. Added ICU to compare strings better. https://codereview.chromium.org/516323003/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java (right): https://codereview.chromium.org/516323003/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:194: * @return The top level folders. Note that normal top folders will be in the alphabetical On 2014/09/02 19:00:10, newt wrote: > do the special folders come before or after the normal folders? Done. (before) https://codereview.chromium.org/516323003/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:195: * order. Special top folders are managed bookmark root folder and partner bookmark On 2014/09/02 19:00:10, newt wrote: > remove "root" and "folder root folder" Done. https://codereview.chromium.org/516323003/diff/40001/chrome/browser/android/b... File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right): https://codereview.chromium.org/516323003/diff/40001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:208: if (*it != NULL) { On 2014/09/02 19:00:10, newt wrote: > How could *it be NULL? Does GetChild(i) sometimes return NULL? Done.
Good call on using ICU :) One last comment inline. https://codereview.chromium.org/516323003/diff/80001/chrome/browser/android/b... File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right): https://codereview.chromium.org/516323003/diff/80001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:66: class BookmarkNodeTitleCompareFunctorFactory { What's the point of making this a separate class? Seems like BookmarkNodeTitleCompareFunctorFactory could be merged into BookmarkNodeTitleCompareFunctor, and this avoids the lifetime issues you mentioned in the comment above.
https://codereview.chromium.org/516323003/diff/80001/chrome/browser/android/b... File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right): https://codereview.chromium.org/516323003/diff/80001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:66: class BookmarkNodeTitleCompareFunctorFactory { On 2014/09/03 00:51:50, newt wrote: > What's the point of making this a separate class? Seems like > BookmarkNodeTitleCompareFunctorFactory could be merged into > BookmarkNodeTitleCompareFunctor, and this avoids the lifetime issues you > mentioned in the comment above. I thought so.. and tried that first, it turned out that std::sort requries Compare functor to be copy-constructible. If not, it gives a bunch of compiler errors :) . I think the most right way to get around this problem is using std::reference_wrapper on the functor object, something like: std::sort(x.begin(), x.end(), std::cref(BookmarkNodeTitleCompareFunctor())); but since c++ not available, this was an alternative (thanks to trchen@ for the suggestion).
> but since c++ not available, this was an alternative (thanks to trchen@ for the > suggestion). s/c++/c++11
https://codereview.chromium.org/516323003/diff/80001/chrome/browser/android/b... File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right): https://codereview.chromium.org/516323003/diff/80001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:45: class BookmarkNodeTitleCompareFunctor { simplify this name? maybe BookmarkTitleComparer https://codereview.chromium.org/516323003/diff/80001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:66: class BookmarkNodeTitleCompareFunctorFactory { On 2014/09/03 00:56:00, Kibeom Kim wrote: > On 2014/09/03 00:51:50, newt wrote: > > What's the point of making this a separate class? Seems like > > BookmarkNodeTitleCompareFunctorFactory could be merged into > > BookmarkNodeTitleCompareFunctor, and this avoids the lifetime issues you > > mentioned in the comment above. > > I thought so.. and tried that first, it turned out that std::sort requries > Compare functor to be copy-constructible. If not, it gives a bunch of compiler > errors :) . > > I think the most right way to get around this problem is using > std::reference_wrapper on the functor object, something like: > > std::sort(x.begin(), x.end(), std::cref(BookmarkNodeTitleCompareFunctor())); > > > but since c++ not available, this was an alternative (thanks to trchen@ for the > suggestion). I'd add a comment explaining this, since it's far from obvious :) https://codereview.chromium.org/516323003/diff/80001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:237: BookmarkNodeTitleCompareFunctorFactory().GetFunctor()); It seems you're not obeying the lifetime rules you laid out above. IIUC the BookmarkNodeTitleCompareFunctorFactory could be destructed before std::stable_sort is called. It would be more likely to work across compilers if you assigned BookmarkNodeTitleCompareFunctorFactory to a local variable before calling std::stable_sort.
https://codereview.chromium.org/516323003/diff/80001/chrome/browser/android/b... File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right): https://codereview.chromium.org/516323003/diff/80001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:45: class BookmarkNodeTitleCompareFunctor { On 2014/09/03 01:49:51, newt wrote: > simplify this name? maybe BookmarkTitleComparer Done. https://codereview.chromium.org/516323003/diff/80001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:66: class BookmarkNodeTitleCompareFunctorFactory { On 2014/09/03 01:49:51, newt wrote: > On 2014/09/03 00:56:00, Kibeom Kim wrote: > > On 2014/09/03 00:51:50, newt wrote: > > > What's the point of making this a separate class? Seems like > > > BookmarkNodeTitleCompareFunctorFactory could be merged into > > > BookmarkNodeTitleCompareFunctor, and this avoids the lifetime issues you > > > mentioned in the comment above. > > > > I thought so.. and tried that first, it turned out that std::sort requries > > Compare functor to be copy-constructible. If not, it gives a bunch of compiler > > errors :) . > > > > I think the most right way to get around this problem is using > > std::reference_wrapper on the functor object, something like: > > > > std::sort(x.begin(), x.end(), std::cref(BookmarkNodeTitleCompareFunctor())); > > > > > > but since c++ not available, this was an alternative (thanks to trchen@ for > the > > suggestion). > > I'd add a comment explaining this, since it's far from obvious :) Done. https://codereview.chromium.org/516323003/diff/80001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:237: BookmarkNodeTitleCompareFunctorFactory().GetFunctor()); On 2014/09/03 01:49:51, newt wrote: > It seems you're not obeying the lifetime rules you laid out above. IIUC the > BookmarkNodeTitleCompareFunctorFactory could be destructed before > std::stable_sort is called. It would be more likely to work across compilers if > you assigned BookmarkNodeTitleCompareFunctorFactory to a local variable before > calling std::stable_sort. I think it's safe because BookmarkNodeTitleCompareFunctorFactory is guaranteed to be destructed after std::stable_sort by c++ spec. http://stackoverflow.com/questions/12323049/when-do-temporary-parameter-value...
https://codereview.chromium.org/516323003/diff/100001/chrome/browser/android/... File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right): https://codereview.chromium.org/516323003/diff/100001/chrome/browser/android/... chrome/browser/android/bookmarks/bookmarks_bridge.cc:202: env, j_result_obj, bookmark_model_->other_node()->id(), -2 indent for these two lines https://codereview.chromium.org/516323003/diff/100001/chrome/browser/android/... chrome/browser/android/bookmarks/bookmarks_bridge.cc:219: // TODO(kkimlabs): add partner bookmark root node, if available. I think this should just be: if (partner_bookmarks_shim_->HasPartnerBookmarks()) { top_level_folders.push_back( partner_bookmarks_shim_->GetPartnerBookmarksRoot()); } Is there any reason to not add that right now? https://codereview.chromium.org/516323003/diff/100001/chrome/browser/android/... chrome/browser/android/bookmarks/bookmarks_bridge.cc:226: top_level_folders.push_back(bookmark_model_->bookmark_bar_node()); why the root node in the above function and the bookmark_bar_node here? https://codereview.chromium.org/516323003/diff/100001/chrome/browser/android/... chrome/browser/android/bookmarks/bookmarks_bridge.cc:230: top_level_folders.push_back(mobile_node->GetChild(i)); What if the child isn't a folder? same below? I find the name slightly confusing since it's not really just top folders but some collection of child folders/nodes as well. I can't say that I can come up with a better choice, so really this rambling statement is just useless.
https://codereview.chromium.org/516323003/diff/100001/chrome/browser/android/... File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right): https://codereview.chromium.org/516323003/diff/100001/chrome/browser/android/... chrome/browser/android/bookmarks/bookmarks_bridge.cc:202: env, j_result_obj, bookmark_model_->other_node()->id(), On 2014/09/03 17:43:17, Ted C wrote: > -2 indent for these two lines Done. https://codereview.chromium.org/516323003/diff/100001/chrome/browser/android/... chrome/browser/android/bookmarks/bookmarks_bridge.cc:219: // TODO(kkimlabs): add partner bookmark root node, if available. On 2014/09/03 17:43:17, Ted C wrote: > I think this should just be: > > if (partner_bookmarks_shim_->HasPartnerBookmarks()) { > top_level_folders.push_back( > partner_bookmarks_shim_->GetPartnerBookmarksRoot()); > } > > Is there any reason to not add that right now? Done. https://codereview.chromium.org/516323003/diff/100001/chrome/browser/android/... chrome/browser/android/bookmarks/bookmarks_bridge.cc:226: top_level_folders.push_back(bookmark_model_->bookmark_bar_node()); On 2014/09/03 17:43:17, Ted C wrote: > why the root node in the above function and the bookmark_bar_node here? You mean the DCHECK? I just added for making sure that the root node structure is as we expected before we're using it. Not really important though. https://codereview.chromium.org/516323003/diff/100001/chrome/browser/android/... chrome/browser/android/bookmarks/bookmarks_bridge.cc:230: top_level_folders.push_back(mobile_node->GetChild(i)); On 2014/09/03 17:43:17, Ted C wrote: > What if the child isn't a folder? same below? > > I find the name slightly confusing since it's not really just top folders but > some collection of child folders/nodes as well. I can't say that I can come up > with a better choice, so really this rambling statement is just useless. Actually... I needed a folder check here. When I was testing, everything showed as folders and tapping it shows nothing. So I thought they were empty folders...
https://codereview.chromium.org/516323003/diff/120001/chrome/browser/android/... File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right): https://codereview.chromium.org/516323003/diff/120001/chrome/browser/android/... chrome/browser/android/bookmarks/bookmarks_bridge.cc:247: std::stable_sort(top_level_folders.begin() + special_count, Suggestion: You could replace BookmarkTitleComparerFactory with a method GetICUCollator(). I think this might simplify the code and make it more clearly correct. How about: scoped_ptr<icu::Collator> GetICUCollator() { return ... } ... scoped_ptr<icu::Collator> collator = GetICUCollator(); std::stable_sort(..., BookmarkTitleComparer(collator.get()));
https://codereview.chromium.org/516323003/diff/120001/chrome/browser/android/... File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right): https://codereview.chromium.org/516323003/diff/120001/chrome/browser/android/... chrome/browser/android/bookmarks/bookmarks_bridge.cc:247: std::stable_sort(top_level_folders.begin() + special_count, On 2014/09/03 18:47:58, newt wrote: > Suggestion: You could replace BookmarkTitleComparerFactory with a method > GetICUCollator(). I think this might simplify the code and make it more clearly > correct. How about: > > scoped_ptr<icu::Collator> GetICUCollator() { > return ... > } > > ... > > scoped_ptr<icu::Collator> collator = GetICUCollator(); > std::stable_sort(..., BookmarkTitleComparer(collator.get())); Done.
ping
lgtm
The CQ bit was checked by kkimlabs@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/516323003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by kkimlabs@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/516323003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by kkimlabs@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/516323003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by kkimlabs@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/516323003/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as 19760bcd410720425cad94e7d9e1cc3791ceb9c5
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/78cb62e9a8bf1af39b11a95b20a7b5a51482e2f5 Cr-Commit-Position: refs/heads/master@{#293291} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
