Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
506353003:
[Enhanced Bookmark] Add GetAllFoldersWithDepth in bookmark bridge (Closed)
Description[Enhanced Bookmark] Add GetAllFoldersWithDepth in bookmark bridge
GetAllFoldersWithDepth is used to get all folders under which users are able to move their own bookmarks. Mobile, other, root node and managed folder are not included in the returned result.
BUG=386785
Committed: https://crrev.com/6fc35d4dd9a9a0eb92dd224b84e114a37cf4831c
Cr-Commit-Position: refs/heads/master@{#293979}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Remove partner bookmark from result #
Total comments: 12
Patch Set 3 : Changing function name #
Total comments: 1
Patch Set 4 : Change to a more efficient tree traversal method #Patch Set 5 : Add GetMobileFolderId in bridge #
Total comments: 13
Patch Set 6 : changed some details. added more comments. #
Total comments: 14
Patch Set 7 : Rename variables #Patch Set 8 : Add comments #
Total comments: 2
Messages
Total messages: 40 (4 generated)
ianwen@chromium.org changed reviewers: + kkimlabs@chromium.org, tedchoc@chromium.org, yfriedman@chromium.org
yfriedman@chromium.org: Please review changes in tedchoc@chromium.org: Please review changes in
https://codereview.chromium.org/506353003/diff/1/chrome/browser/android/bookm... File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right): https://codereview.chromium.org/506353003/diff/1/chrome/browser/android/bookm... chrome/browser/android/bookmarks/bookmarks_bridge.cc:149: jobject j_result_obj) { nit: alien the jobject arguments to JNIEnv* https://codereview.chromium.org/506353003/diff/1/chrome/browser/android/bookm... chrome/browser/android/bookmarks/bookmarks_bridge.cc:159: partner_bookmarks_shim_->GetPartnerBookmarksRoot()); I thought we cannot add/move nodes to partner bookmarks, could you double check? (I think looking up the partner bookmark class comments should be enough) https://codereview.chromium.org/506353003/diff/1/chrome/browser/android/bookm... chrome/browser/android/bookmarks/bookmarks_bridge.cc:165: if (*it != NULL && client_->CanBeEditedByUser((*it))) { IIRC, partner bookmark nodes cannot be queried by client_ . We have many partner bookmark code examples in this file.
ianwen@chromium.org changed reviewers: + newt@chromium.org - yfriedman@chromium.org
https://codereview.chromium.org/506353003/diff/1/chrome/browser/android/bookm... File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right): https://codereview.chromium.org/506353003/diff/1/chrome/browser/android/bookm... chrome/browser/android/bookmarks/bookmarks_bridge.cc:149: jobject j_result_obj) { On 2014/08/28 05:11:03, Kibeom Kim wrote: > nit: alien the jobject arguments to JNIEnv* Done. https://codereview.chromium.org/506353003/diff/1/chrome/browser/android/bookm... chrome/browser/android/bookmarks/bookmarks_bridge.cc:159: partner_bookmarks_shim_->GetPartnerBookmarksRoot()); Per comments of DiablePartnerBookmarkEditing(), Partner bookmark can be edited if our partner permits user doing so. I added extra check for it. On 2014/08/28 05:11:03, Kibeom Kim wrote: > I thought we cannot add/move nodes to partner bookmarks, could you double check? > (I think looking up the partner bookmark class comments should be enough) https://codereview.chromium.org/506353003/diff/1/chrome/browser/android/bookm... chrome/browser/android/bookmarks/bookmarks_bridge.cc:165: if (*it != NULL && client_->CanBeEditedByUser((*it))) { On 2014/08/28 05:11:03, Kibeom Kim wrote: > IIRC, partner bookmark nodes cannot be queried by client_ . We have many partner > bookmark code examples in this file. Done.
What are you going to be using this method for out of curiosity? https://codereview.chromium.org/506353003/diff/20001/chrome/browser/android/b... File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right): https://codereview.chromium.org/506353003/diff/20001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:147: void BookmarksBridge::GetEditablePermanentIDs(JNIEnv* env, Editable is an overloaded term in this file. We have used it here to specify whether you can actually edit the node itself (name/title/etc...). For these, they aren't really editable, but they are viable parent nodes. GetPermanentIDsAllowingChildren or something like that is the best that I can come up with. I can't say it's a great name, but at least it avoids the conflict with Editable below.
https://codereview.chromium.org/506353003/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java (right): https://codereview.chromium.org/506353003/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:173: * Note every bookmark node below "bookmark bar", "other bookmark", Move this note out of the return value specification. https://codereview.chromium.org/506353003/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:184: * @return All permanent folder nodes that are not root nor under managed folder. Please explain in this comment what "editable permanent IDs" are (as you did in the commit message) https://codereview.chromium.org/506353003/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:186: public List<BookmarkId> getEditablePermanentIDs() { for consistency, I'd call this getEditablePermanentNodeIDs() https://codereview.chromium.org/506353003/diff/20001/chrome/browser/android/b... File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right): https://codereview.chromium.org/506353003/diff/20001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:163: if (*it != NULL && client_->CanBeEditedByUser((*it))) { suggestion: check client_->CanBeEditedByUser(*it) on line 156 before calling permanent_nodes->push_back(). Also, s/((*it))/(*it) https://codereview.chromium.org/506353003/diff/20001/chrome/browser/android/b... File chrome/browser/android/bookmarks/bookmarks_bridge.h (right): https://codereview.chromium.org/506353003/diff/20001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.h:42: // Returns permanent ids that are not root nor nodes under managed folder. Don't comment this method, since it's already commented in Java. Duplicate comments lead to comments getting out of date and conflicting comments.
GetEditablePermanentIDs() is the first step to show folder hierachy in moving bookmark dialog. It is used to get a list of root folders, i.e. "Desktop", "Other" and "Mobile". The previous function, GetPermanentNodeIDs, will return folders that user should not add bookmarks to, such as "Managed folder". What's worse, the result also contains the ROOT node, which is parent of all root folders and their 1st generation children. After acquiring these "bookmark-addable/movable/allowing" root folders, getChildIds() is used to traverse the tree. https://codereview.chromium.org/506353003/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java (right): https://codereview.chromium.org/506353003/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:173: * Note every bookmark node below "bookmark bar", "other bookmark", On 2014/08/28 18:51:38, newt wrote: > Move this note out of the return value specification. Done. https://codereview.chromium.org/506353003/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:184: * @return All permanent folder nodes that are not root nor under managed folder. On 2014/08/28 18:51:38, newt wrote: > Please explain in this comment what "editable permanent IDs" are (as you did in > the commit message) Done. https://codereview.chromium.org/506353003/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:186: public List<BookmarkId> getEditablePermanentIDs() { On 2014/08/28 18:51:38, newt wrote: > for consistency, I'd call this getEditablePermanentNodeIDs() Done. https://codereview.chromium.org/506353003/diff/20001/chrome/browser/android/b... File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right): https://codereview.chromium.org/506353003/diff/20001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:147: void BookmarksBridge::GetEditablePermanentIDs(JNIEnv* env, This function is the first step to show folder hierachy in moving bookmark dialog. It is used to get a list of root folders, i.e. "Desktop", "Other" and "Mobile". The previous function, GetPermanentNodeIDs, will return folders that user should not add bookmarks to, such as "Managed folder". What's worse, the result also contains root node, which is parent of all root folders and their 1st generation children. After acquiring these "bookmark-addable/movable/allowing" root folders, getChildIds() is used to traverse the tree. On 2014/08/28 18:46:21, Ted C wrote: > Editable is an overloaded term in this file. We have used it here to specify > whether you can actually edit the node itself (name/title/etc...). For these, > they aren't really editable, but they are viable parent nodes. > > GetPermanentIDsAllowingChildren or something like that is the best that I can > come up with. I can't say it's a great name, but at least it avoids the > conflict with Editable below. https://codereview.chromium.org/506353003/diff/20001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:163: if (*it != NULL && client_->CanBeEditedByUser((*it))) { On 2014/08/28 18:51:38, newt wrote: > suggestion: check client_->CanBeEditedByUser(*it) on line 156 before calling > permanent_nodes->push_back(). Also, s/((*it))/(*it) Done. https://codereview.chromium.org/506353003/diff/20001/chrome/browser/android/b... File chrome/browser/android/bookmarks/bookmarks_bridge.h (right): https://codereview.chromium.org/506353003/diff/20001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.h:42: // Returns permanent ids that are not root nor nodes under managed folder. On 2014/08/28 18:51:38, newt wrote: > Don't comment this method, since it's already commented in Java. Duplicate > comments lead to comments getting out of date and conflicting comments. Done.
https://codereview.chromium.org/506353003/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java (right): https://codereview.chromium.org/506353003/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:176: * @return All the permanent nodes, Changed to "." instead of "," locally.
lgtm
On 2014/08/28 21:18:05, Ian Wen wrote: > GetEditablePermanentIDs() is the first step to show folder hierachy in moving > bookmark dialog. It is used to get a list of root folders, i.e. "Desktop", > "Other" and "Mobile". The previous function, GetPermanentNodeIDs, will return > folders that user should not add bookmarks to, such as "Managed folder". What's > worse, the result also contains the ROOT node, which is parent of all root > folders and their 1st generation children. > > After acquiring these "bookmark-addable/movable/allowing" root folders, > getChildIds() is used to traverse the tree. Are you going to be using this function to build the full folder hierarchy? I'm just wondering if the UI is designed that you can fetch only certain subfolders efficiently or if it shows all the folders in one single expanded view (i.e. if you can not toggle sub folder visibility). If you have to show all the folders at once in the UI, then I think this is probably going to be a really ineffective means of collecting this information. It will be: GetPermanentIDsAllowingChildren() iterate over that list and call GetChildIds (folders only) iterate over each of those lists and call GetChildIds (folders only) Each one of those GetChildIds will result in a GetNodeByID, which is a recursive search of the bookmark tree. I think this approach works if you want to show one level of bookmarks at a time, but if you need to show them all expanded then I think you want to write an explicit GetAllFoldersAllowingChildren or something of the like. > > https://codereview.chromium.org/506353003/diff/20001/chrome/android/java/src/... > File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java > (right): > > https://codereview.chromium.org/506353003/diff/20001/chrome/android/java/src/... > chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:173: * > Note every bookmark node below "bookmark bar", "other bookmark", > On 2014/08/28 18:51:38, newt wrote: > > Move this note out of the return value specification. > > Done. > > https://codereview.chromium.org/506353003/diff/20001/chrome/android/java/src/... > chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:184: * > @return All permanent folder nodes that are not root nor under managed folder. > On 2014/08/28 18:51:38, newt wrote: > > Please explain in this comment what "editable permanent IDs" are (as you did > in > > the commit message) > > Done. > > https://codereview.chromium.org/506353003/diff/20001/chrome/android/java/src/... > chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:186: > public List<BookmarkId> getEditablePermanentIDs() { > On 2014/08/28 18:51:38, newt wrote: > > for consistency, I'd call this getEditablePermanentNodeIDs() > > Done. > > https://codereview.chromium.org/506353003/diff/20001/chrome/browser/android/b... > File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right): > > https://codereview.chromium.org/506353003/diff/20001/chrome/browser/android/b... > chrome/browser/android/bookmarks/bookmarks_bridge.cc:147: void > BookmarksBridge::GetEditablePermanentIDs(JNIEnv* env, > This function is the first step to show folder hierachy in moving bookmark > dialog. It is used to get a list of root folders, i.e. "Desktop", "Other" and > "Mobile". The previous function, GetPermanentNodeIDs, will return folders that > user should not add bookmarks to, such as "Managed folder". What's worse, the > result also contains root node, which is parent of all root folders and their > 1st generation children. > > After acquiring these "bookmark-addable/movable/allowing" root folders, > getChildIds() is used to traverse the tree. > > On 2014/08/28 18:46:21, Ted C wrote: > > Editable is an overloaded term in this file. We have used it here to specify > > whether you can actually edit the node itself (name/title/etc...). For these, > > they aren't really editable, but they are viable parent nodes. > > > > GetPermanentIDsAllowingChildren or something like that is the best that I can > > come up with. I can't say it's a great name, but at least it avoids the > > conflict with Editable below. > > https://codereview.chromium.org/506353003/diff/20001/chrome/browser/android/b... > chrome/browser/android/bookmarks/bookmarks_bridge.cc:163: if (*it != NULL && > client_->CanBeEditedByUser((*it))) { > On 2014/08/28 18:51:38, newt wrote: > > suggestion: check client_->CanBeEditedByUser(*it) on line 156 before calling > > permanent_nodes->push_back(). Also, s/((*it))/(*it) > > Done. > > https://codereview.chromium.org/506353003/diff/20001/chrome/browser/android/b... > File chrome/browser/android/bookmarks/bookmarks_bridge.h (right): > > https://codereview.chromium.org/506353003/diff/20001/chrome/browser/android/b... > chrome/browser/android/bookmarks/bookmarks_bridge.h:42: // Returns permanent ids > that are not root nor nodes under managed folder. > On 2014/08/28 18:51:38, newt wrote: > > Don't comment this method, since it's already commented in Java. Duplicate > > comments lead to comments getting out of date and conflicting comments. > > Done.
On 2014/08/28 22:47:29, Ted C wrote: > ... > Each one of those GetChildIds will result in a GetNodeByID, which is a > recursive search of the bookmark tree. > > I think this approach works if you want to show one level of bookmarks > at a time, but if you need to show them all expanded then I think you > want to write an explicit GetAllFoldersAllowingChildren or something > of the like. > Good point. Also, I think we're abusing node ID on java side, when a native node pointer is sufficient. Maybe a TODO item for Q4 or spring clean up.
yfriedman@chromium.org changed reviewers: + yfriedman@chromium.org
is this ready for review? the CL description doesn't appear to match the content
On 2014/09/03 01:07:42, Yaron wrote: > is this ready for review? the CL description doesn't appear to match the content Please do not review this CL for now. I am waiting for Kimbeom's CL to compare titles for sorting. Thanks!
This CL is now ready to be reviewed. ptal.
FYI, the title and commit message are completely unrelated to the patch.
On 2014/09/04 20:43:37, newt wrote: > FYI, the title and commit message are completely unrelated to the patch. *mostly unrelated. They need updating in any case :)
On 2014/09/04 20:44:44, newt wrote: > On 2014/09/04 20:43:37, newt wrote: > > FYI, the title and commit message are completely unrelated to the patch. > > *mostly unrelated. They need updating in any case :) Sorry guys... I updated the commit message locally but it did not show up in this webpage. I changed it now by clicking edit issue button.
On 2014/09/04 21:22:12, Ian Wen wrote: > On 2014/09/04 20:44:44, newt wrote: > > On 2014/09/04 20:43:37, newt wrote: > > > FYI, the title and commit message are completely unrelated to the patch. > > > > *mostly unrelated. They need updating in any case :) > > Sorry guys... I updated the commit message locally but it did not show up in > this webpage. I changed it now by clicking edit issue button. On codereview, the commit message is only updated when you first upload the CL. On gerrit, the commit message is updated every time you upload a patch set.
https://codereview.chromium.org/506353003/diff/80001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java (right): https://codereview.chromium.org/506353003/diff/80001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:207: * Populates folderList with BookmarkIds of folders user can move bookmarks s/user/that users https://codereview.chromium.org/506353003/diff/80001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:208: * to and folderList with corresponding integer as depths. I'd explain the depths a bit more. In particular, do depths start at 0 or 1? Which folders have those depths? https://codereview.chromium.org/506353003/diff/80001/chrome/browser/android/b... File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right): https://codereview.chromium.org/506353003/diff/80001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:260: std::vector<const BookmarkNode*> vct; Explain how these variables are used. This method is complicated enough to warrant an implementation comment. https://codereview.chromium.org/506353003/diff/80001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:270: std::sort(vct.begin(), vct.end(), BookmarkTitleComparer(collator.get())); I'd use stable_sort in case there are two folders with the same name. We want those to be sorted in the same order each time, so as not to confuse the user too too much. https://codereview.chromium.org/506353003/diff/80001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:272: for (std::vector<const BookmarkNode*>::reverse_iterator it = vct.rbegin(); You can move lines 270-275 to the beginning of the while loop, and remove the duplication on lines 294-299. You'll also need to move the !stk.empty() check to the middle of the while loop. https://codereview.chromium.org/506353003/diff/80001/chrome/browser/android/b... File chrome/browser/android/bookmarks/bookmarks_bridge.h (right): https://codereview.chromium.org/506353003/diff/80001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.h:54: jobject j_folder_obj, s/folder/folders s/depth/depths
https://codereview.chromium.org/506353003/diff/80001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java (right): https://codereview.chromium.org/506353003/diff/80001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:207: * Populates folderList with BookmarkIds of folders user can move bookmarks On 2014/09/04 21:49:55, newt wrote: > s/user/that users Done. https://codereview.chromium.org/506353003/diff/80001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:208: * to and folderList with corresponding integer as depths. On 2014/09/04 21:49:55, newt wrote: > I'd explain the depths a bit more. In particular, do depths start at 0 or 1? > Which folders have those depths? Done. https://codereview.chromium.org/506353003/diff/80001/chrome/browser/android/b... File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right): https://codereview.chromium.org/506353003/diff/80001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:260: std::vector<const BookmarkNode*> vct; On 2014/09/04 21:49:55, newt wrote: > Explain how these variables are used. This method is complicated enough to > warrant an implementation comment. Done. https://codereview.chromium.org/506353003/diff/80001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:270: std::sort(vct.begin(), vct.end(), BookmarkTitleComparer(collator.get())); On 2014/09/04 21:49:55, newt wrote: > I'd use stable_sort in case there are two folders with the same name. We want > those to be sorted in the same order each time, so as not to confuse the user > too too much. Done. https://codereview.chromium.org/506353003/diff/80001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:272: for (std::vector<const BookmarkNode*>::reverse_iterator it = vct.rbegin(); On 2014/09/04 21:49:55, newt wrote: > You can move lines 270-275 to the beginning of the while loop, and remove the > duplication on lines 294-299. You'll also need to move the !stk.empty() check to > the middle of the while loop. But this for loop is not completely the same as the one in while loop. In while loop depth is incremented while here it is a step for stack initialization... https://codereview.chromium.org/506353003/diff/80001/chrome/browser/android/b... File chrome/browser/android/bookmarks/bookmarks_bridge.h (right): https://codereview.chromium.org/506353003/diff/80001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.h:54: jobject j_folder_obj, On 2014/09/04 21:49:55, newt wrote: > s/folder/folders > s/depth/depths Done.
https://codereview.chromium.org/506353003/diff/80001/chrome/browser/android/b... File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right): https://codereview.chromium.org/506353003/diff/80001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:272: for (std::vector<const BookmarkNode*>::reverse_iterator it = vct.rbegin(); On 2014/09/05 01:00:19, Ian Wen wrote: > On 2014/09/04 21:49:55, newt wrote: > > You can move lines 270-275 to the beginning of the while loop, and remove the > > duplication on lines 294-299. You'll also need to move the !stk.empty() check > to > > the middle of the while loop. > > But this for loop is not completely the same as the one in while loop. In while > loop depth is incremented while here it is a step for stack initialization... Right. You'd need to initialize depth to -1, then the code is the same. Just a suggestion. It's six lines of somewhat complex duplicated code -- seems like a good candidate for deduping. https://codereview.chromium.org/506353003/diff/100001/chrome/browser/android/... File chrome/browser/android/bookmarks/bookmarks_bridge.h (right): https://codereview.chromium.org/506353003/diff/100001/chrome/browser/android/... chrome/browser/android/bookmarks/bookmarks_bridge.h:54: jobject j_folder_obj, I'd also change the parameter names: s/j_folder_obj/j_folders_obj and depth->depths
can you add a test for this api? https://codereview.chromium.org/506353003/diff/100001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java (right): https://codereview.chromium.org/506353003/diff/100001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:229: nit: remove line https://codereview.chromium.org/506353003/diff/100001/chrome/browser/android/... File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right): https://codereview.chromium.org/506353003/diff/100001/chrome/browser/android/... chrome/browser/android/bookmarks/bookmarks_bridge.cc:261: std::vector<const BookmarkNode*> vct; nit: prefer meaningful names over abbreviated names. allBookmarks/bookmarks/bookmarkList would all be better https://codereview.chromium.org/506353003/diff/100001/chrome/browser/android/... chrome/browser/android/bookmarks/bookmarks_bridge.cc:264: std::stack<std::pair<const BookmarkNode*, int> > stk; stack/bookmarkDepth? https://codereview.chromium.org/506353003/diff/100001/chrome/browser/android/... chrome/browser/android/bookmarks/bookmarks_bridge.cc:270: vct.push_back(other->GetChild(i)); as new bookmarks by default are added to "other" and you aren't filtering before sorting on (L273) this will get unnecessarily expensive over time. I'd say to add the is_folder check here and above. https://codereview.chromium.org/506353003/diff/100001/chrome/browser/android/... chrome/browser/android/bookmarks/bookmarks_bridge.cc:277: for (std::vector<const BookmarkNode*>::reverse_iterator it = vct.rbegin(); why reverse_iterator? also, why not use const_iterator? https://codereview.chromium.org/506353003/diff/100001/chrome/browser/android/... chrome/browser/android/bookmarks/bookmarks_bridge.cc:297: vct.push_back(node->GetChild(i)); same comment about effeciency
https://codereview.chromium.org/506353003/diff/100001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java (right): https://codereview.chromium.org/506353003/diff/100001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:229: On 2014/09/05 03:30:58, Yaron wrote: > nit: remove line Done. https://codereview.chromium.org/506353003/diff/100001/chrome/browser/android/... File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right): https://codereview.chromium.org/506353003/diff/100001/chrome/browser/android/... chrome/browser/android/bookmarks/bookmarks_bridge.cc:261: std::vector<const BookmarkNode*> vct; On 2014/09/05 03:30:58, Yaron wrote: > nit: prefer meaningful names over abbreviated names. > allBookmarks/bookmarks/bookmarkList would all be better Done. https://codereview.chromium.org/506353003/diff/100001/chrome/browser/android/... chrome/browser/android/bookmarks/bookmarks_bridge.cc:264: std::stack<std::pair<const BookmarkNode*, int> > stk; On 2014/09/05 03:30:59, Yaron wrote: > stack/bookmarkDepth? Stack is the symbolic sign of depth first traversal of the tree. Here it is solely used for dfs traversal as a "first in last out" container. I believe in this case stack had better be named as stack or stk... https://codereview.chromium.org/506353003/diff/100001/chrome/browser/android/... chrome/browser/android/bookmarks/bookmarks_bridge.cc:270: vct.push_back(other->GetChild(i)); On 2014/09/05 03:30:59, Yaron wrote: > as new bookmarks by default are added to "other" and you aren't filtering before > sorting on (L273) this will get unnecessarily expensive over time. I'd say to > add the is_folder check here and above. Done. https://codereview.chromium.org/506353003/diff/100001/chrome/browser/android/... chrome/browser/android/bookmarks/bookmarks_bridge.cc:277: for (std::vector<const BookmarkNode*>::reverse_iterator it = vct.rbegin(); On 2014/09/05 03:30:58, Yaron wrote: > why reverse_iterator? also, why not use const_iterator? Changed to const_reverse_iterator. In vector, folders are ordered from A to Z. I also need to push them to a stack for DFS traversal. To ensure same order in final output, they should be pushed to stack from Z to A and be popped out from A to Z in traversal. https://codereview.chromium.org/506353003/diff/100001/chrome/browser/android/... chrome/browser/android/bookmarks/bookmarks_bridge.cc:297: vct.push_back(node->GetChild(i)); On 2014/09/05 03:30:59, Yaron wrote: > same comment about effeciency Done. https://codereview.chromium.org/506353003/diff/100001/chrome/browser/android/... File chrome/browser/android/bookmarks/bookmarks_bridge.h (right): https://codereview.chromium.org/506353003/diff/100001/chrome/browser/android/... chrome/browser/android/bookmarks/bookmarks_bridge.h:54: jobject j_folder_obj, On 2014/09/05 02:10:21, newt wrote: > I'd also change the parameter names: s/j_folder_obj/j_folders_obj and > depth->depths Done.
https://codereview.chromium.org/506353003/diff/140001/chrome/browser/android/... File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right): https://codereview.chromium.org/506353003/diff/140001/chrome/browser/android/... chrome/browser/android/bookmarks/bookmarks_bridge.cc:318: for (std::vector<const BookmarkNode*>::reverse_iterator it = const_reverse_iterator cannot be applied here as it doesn't compile. :( Use reverse_iterator instead. Since entries in vector are const pointers, it should be fine.
ok, that's fien but I asked for a test in previous revision. it seems like you should be able to write a self-contained test as this is just model code
lgtm offline discussion: need crud operations in the bookmarksbridge before we can test it (which is the next cl). please file a bug to follow-up
On 2014/09/06 00:43:05, Yaron wrote: > lgtm > > offline discussion: need crud operations in the bookmarksbridge before we can > test it (which is the next cl). please file a bug to follow-up What are "crud operations"?
http://en.wikipedia.org/wiki/Create,_read,_update_and_delete :) On Fri, Sep 5, 2014 at 5:45 PM, <newt@chromium.org> wrote: > On 2014/09/06 00:43:05, Yaron wrote: > >> lgtm >> > > offline discussion: need crud operations in the bookmarksbridge before we >> can >> test it (which is the next cl). please file a bug to follow-up >> > > What are "crud operations"? > > https://codereview.chromium.org/506353003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/09/06 00:46:29, Yaron wrote: > http://en.wikipedia.org/wiki/Create,_read,_update_and_delete :) > > > On Fri, Sep 5, 2014 at 5:45 PM, <mailto:newt@chromium.org> wrote: > > > On 2014/09/06 00:43:05, Yaron wrote: > > > >> lgtm > >> > > > > offline discussion: need crud operations in the bookmarksbridge before we > >> can > >> test it (which is the next cl). please file a bug to follow-up > >> > > > > What are "crud operations"? > > > > https://codereview.chromium.org/506353003/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Unit Test bug is here: https://code.google.com/p/chromium/issues/detail?id=411545
The CQ bit was checked by ianwen@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ianwen@chromium.org/506353003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded time limit waiting for builds to trigger.
https://codereview.chromium.org/506353003/diff/140001/chrome/browser/android/... File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right): https://codereview.chromium.org/506353003/diff/140001/chrome/browser/android/... chrome/browser/android/bookmarks/bookmarks_bridge.cc:318: for (std::vector<const BookmarkNode*>::reverse_iterator it = On 2014/09/05 22:35:21, Ian Wen wrote: > const_reverse_iterator cannot be applied here as it doesn't compile. :( Use > reverse_iterator instead. Since entries in vector are const pointers, it should > be fine. fyi workaround: std::vector<const BookmarkNode*>::const_reverse_iterator crend = bookmarkList.rend(); for (std::vector<const BookmarkNode*>::const_reverse_iterator it = bookmarkList.rbegin(); it != crend; ++it) {
The CQ bit was checked by ianwen@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ianwen@chromium.org/506353003/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as 1d85a2b3f469be657fd52b26c28aa678bebd8df2
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/6fc35d4dd9a9a0eb92dd224b84e114a37cf4831c Cr-Commit-Position: refs/heads/master@{#293979} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
