Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
795483004:
In Bookmark manager 'Add page' and 'Add Folder' always add new page or folder at the end. (Closed)
DescriptionIn Bookmark manager 'Add page' and 'Add Folder' always add new page or folder at the end.
1.When we add new page or new folder after selecting some page bookmarked entry
in Bookmark manager, then new item should be added immediately below the
selected item.
2.when we add new page or new folder after selecting some folder bookmarked entry
in Bookmark manager, then new item should be added as last item inside selected
folder.
3.when we add new page or new folder without selecting any bookmarked entry
in Bookmark manager, then new item should be added in the last of already existing
bookmarked entries.
Added code in addPage() and newFolder() in main.js for getting the index of new node to be added so that it will be at proper location.
BUG=409816
Committed: https://crrev.com/829809ec98802b1d19d8f1793d90079833899560
Cr-Commit-Position: refs/heads/master@{#310719}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Addressing review comments. #Patch Set 3 : #Patch Set 4 : #
Total comments: 4
Patch Set 5 : #
Total comments: 2
Patch Set 6 : Changes as per review comments. #
Total comments: 2
Patch Set 7 : Changes as per review comments. #Messages
Total messages: 19 (4 generated)
deepak.m1@samsung.com changed reviewers: + xiyuan@chromium.org
@xiyuan PTAL at my changes.
deepak.m1@samsung.com changed reviewers: + bauerb@chromium.org
Your CL description states the current wrong behavior. That is context that is best placed in the linked bug. The CL description should instead contain the changes made in it. https://codereview.chromium.org/795483004/diff/1/chrome/browser/resources/boo... File chrome/browser/resources/bookmark_manager/js/main.js (right): https://codereview.chromium.org/795483004/diff/1/chrome/browser/resources/boo... chrome/browser/resources/bookmark_manager/js/main.js:1032: var newIndex = ''; |newIndex| is supposed to be an integer. Just because the Javascript type system allows you to assign a value of a different type to it doesn't mean that is a good idea. If you just leave this as undefined (i.e. `var newIndex;`), you can pass it to chrome.bookmarks.create in the same call, so you don't need the additional call below. https://codereview.chromium.org/795483004/diff/1/chrome/browser/resources/boo... chrome/browser/resources/bookmark_manager/js/main.js:1036: if (selectedItem && document.activeElement != bmm.tree) { The indentation is way off in this CL. I would suggest you familiarize yourself with the Chromium style guide (http://dev.chromium.org/developers/coding-style/), specifically the web development style (http://dev.chromium.org/developers/web-development-style-guide). https://codereview.chromium.org/795483004/diff/1/chrome/browser/resources/boo... chrome/browser/resources/bookmark_manager/js/main.js:1038: newIndex = 1 + bmm.list.dataModel.indexOf(selectedItem); It's more idiomatic to put the constant summand as the second argument (`... + 1`). https://codereview.chromium.org/795483004/diff/1/chrome/browser/resources/boo... chrome/browser/resources/bookmark_manager/js/main.js:1047: // As create bookmark folder does not create bookmark node for empty index. I have no idea what this comment is supposed to mean. https://codereview.chromium.org/795483004/diff/1/chrome/browser/resources/boo... chrome/browser/resources/bookmark_manager/js/main.js:1067: if (newIndex != '' && newIndex <= length) This callback will be called asynchronously. What if another item has been added in the mean time, so |newIndex| is not the index of the newly created folder anymore? https://codereview.chromium.org/795483004/diff/1/chrome/browser/resources/boo... chrome/browser/resources/bookmark_manager/js/main.js:1068: length = newIndex; Why do you rename this variable to |length|, but then assign a value to it that is not a length, but an index? Arguably, |index| was a better description. https://codereview.chromium.org/795483004/diff/1/chrome/browser/resources/boo... chrome/browser/resources/bookmark_manager/js/main.js:1117: length = newIndex; Same here; this is not really a length anymore, it's an insertion index.
PTAL https://codereview.chromium.org/795483004/diff/1/chrome/browser/resources/boo... File chrome/browser/resources/bookmark_manager/js/main.js (right): https://codereview.chromium.org/795483004/diff/1/chrome/browser/resources/boo... chrome/browser/resources/bookmark_manager/js/main.js:1032: var newIndex = ''; On 2014/12/22 11:58:20, Bernhard Bauer wrote: > |newIndex| is supposed to be an integer. Just because the Javascript type system > allows you to assign a value of a different type to it doesn't mean that is a > good idea. If you just leave this as undefined (i.e. `var newIndex;`), you can > pass it to chrome.bookmarks.create in the same call, so you don't need the > additional call below. Done. https://codereview.chromium.org/795483004/diff/1/chrome/browser/resources/boo... chrome/browser/resources/bookmark_manager/js/main.js:1036: if (selectedItem && document.activeElement != bmm.tree) { On 2014/12/22 11:58:20, Bernhard Bauer wrote: > The indentation is way off in this CL. I would suggest you familiarize yourself > with the Chromium style guide > (http://dev.chromium.org/developers/coding-style/), specifically the web > development style > (http://dev.chromium.org/developers/web-development-style-guide). Can you please tell me in specific about this. https://codereview.chromium.org/795483004/diff/1/chrome/browser/resources/boo... chrome/browser/resources/bookmark_manager/js/main.js:1038: newIndex = 1 + bmm.list.dataModel.indexOf(selectedItem); On 2014/12/22 11:58:20, Bernhard Bauer wrote: > It's more idiomatic to put the constant summand as the second argument (`... + > 1`). Done. https://codereview.chromium.org/795483004/diff/1/chrome/browser/resources/boo... chrome/browser/resources/bookmark_manager/js/main.js:1047: // As create bookmark folder does not create bookmark node for empty index. On 2014/12/22 11:58:21, Bernhard Bauer wrote: > I have no idea what this comment is supposed to mean. This part of code is not required after taking newIndex as undefined. https://codereview.chromium.org/795483004/diff/1/chrome/browser/resources/boo... chrome/browser/resources/bookmark_manager/js/main.js:1067: if (newIndex != '' && newIndex <= length) On 2014/12/22 11:58:20, Bernhard Bauer wrote: > This callback will be called asynchronously. What if another item has been added > in the mean time, so |newIndex| is not the index of the newly created folder > anymore? I understand. Can you please tell me some way to reproduce this. https://codereview.chromium.org/795483004/diff/1/chrome/browser/resources/boo... chrome/browser/resources/bookmark_manager/js/main.js:1068: length = newIndex; On 2014/12/22 11:58:20, Bernhard Bauer wrote: > Why do you rename this variable to |length|, but then assign a value to it that > is not a length, but an index? Arguably, |index| was a better description. Done. https://codereview.chromium.org/795483004/diff/1/chrome/browser/resources/boo... chrome/browser/resources/bookmark_manager/js/main.js:1117: length = newIndex; On 2014/12/22 11:58:20, Bernhard Bauer wrote: > Same here; this is not really a length anymore, it's an insertion index. Done.
On 2014/12/22 11:58:21, Bernhard Bauer wrote: > Your CL description states the current wrong behavior. That is context that is > best placed in the linked bug. The CL description should instead contain the > changes made in it. ^^^ https://codereview.chromium.org/795483004/diff/1/chrome/browser/resources/boo... File chrome/browser/resources/bookmark_manager/js/main.js (right): https://codereview.chromium.org/795483004/diff/1/chrome/browser/resources/boo... chrome/browser/resources/bookmark_manager/js/main.js:1036: if (selectedItem && document.activeElement != bmm.tree) { On 2014/12/23 04:27:47, Deepak wrote: > On 2014/12/22 11:58:20, Bernhard Bauer wrote: > > The indentation is way off in this CL. I would suggest you familiarize > yourself > > with the Chromium style guide > > (http://dev.chromium.org/developers/coding-style/), specifically the web > > development style > > (http://dev.chromium.org/developers/web-development-style-guide). > > Can you please tell me in specific about this. The issues are fixed now. https://codereview.chromium.org/795483004/diff/1/chrome/browser/resources/boo... chrome/browser/resources/bookmark_manager/js/main.js:1067: if (newIndex != '' && newIndex <= length) On 2014/12/23 04:27:47, Deepak wrote: > On 2014/12/22 11:58:20, Bernhard Bauer wrote: > > This callback will be called asynchronously. What if another item has been > added > > in the mean time, so |newIndex| is not the index of the newly created folder > > anymore? > I understand. > Can you please tell me some way to reproduce this. I don't know how to reproduce this, but the onus is on you to prove that it can't happen or that you're dealing with it adequately. It seems that the newly created node is passed to the callback; could you use that to look up the index in the model? https://codereview.chromium.org/795483004/diff/60001/chrome/browser/resources... File chrome/browser/resources/bookmark_manager/js/main.js (right): https://codereview.chromium.org/795483004/diff/60001/chrome/browser/resources... chrome/browser/resources/bookmark_manager/js/main.js:1032: var newIndex, selectedItem = bmm.list.selectedItem; Declare each variable on a separate line. https://codereview.chromium.org/795483004/diff/60001/chrome/browser/resources... chrome/browser/resources/bookmark_manager/js/main.js:1106: if (newIndex && newIndex <= index) You probably want to check for (newIndex != undefined) here, as 0 is also considered false in Javascript. Also, is the second check really necessary? newIndex is the index of the selected item in |dataModel|, so it should be less than its length.
On 2014/12/23 11:54:37, Bernhard Bauer wrote: > On 2014/12/22 11:58:21, Bernhard Bauer wrote: > > Your CL description states the current wrong behavior. That is context that is > > best placed in the linked bug. The CL description should instead contain the > > changes made in it. > > ^^^ > > https://codereview.chromium.org/795483004/diff/1/chrome/browser/resources/boo... > File chrome/browser/resources/bookmark_manager/js/main.js (right): > > https://codereview.chromium.org/795483004/diff/1/chrome/browser/resources/boo... > chrome/browser/resources/bookmark_manager/js/main.js:1036: if (selectedItem && > document.activeElement != bmm.tree) { > On 2014/12/23 04:27:47, Deepak wrote: > > On 2014/12/22 11:58:20, Bernhard Bauer wrote: > > > The indentation is way off in this CL. I would suggest you familiarize > > yourself > > > with the Chromium style guide > > > (http://dev.chromium.org/developers/coding-style/), specifically the web > > > development style > > > (http://dev.chromium.org/developers/web-development-style-guide). > > > > Can you please tell me in specific about this. > > The issues are fixed now. > > https://codereview.chromium.org/795483004/diff/1/chrome/browser/resources/boo... > chrome/browser/resources/bookmark_manager/js/main.js:1067: if (newIndex != '' && > newIndex <= length) > On 2014/12/23 04:27:47, Deepak wrote: > > On 2014/12/22 11:58:20, Bernhard Bauer wrote: > > > This callback will be called asynchronously. What if another item has been > > added > > > in the mean time, so |newIndex| is not the index of the newly created folder > > > anymore? > > I understand. > > Can you please tell me some way to reproduce this. > > I don't know how to reproduce this, but the onus is on you to prove that it > can't happen or that you're dealing with it adequately. > > It seems that the newly created node is passed to the callback; could you use > that to look up the index in the model? AFAIK We require index while creation of the bookmark node,And we get callback after creation of bookmark node. So we cannot use callback information. > https://codereview.chromium.org/795483004/diff/60001/chrome/browser/resources... > File chrome/browser/resources/bookmark_manager/js/main.js (right): > > https://codereview.chromium.org/795483004/diff/60001/chrome/browser/resources... > chrome/browser/resources/bookmark_manager/js/main.js:1032: var newIndex, > selectedItem = bmm.list.selectedItem; > Declare each variable on a separate line. > > https://codereview.chromium.org/795483004/diff/60001/chrome/browser/resources... > chrome/browser/resources/bookmark_manager/js/main.js:1106: if (newIndex && > newIndex <= index) > You probably want to check for (newIndex != undefined) here, as 0 is also > considered false in Javascript. Also, is the second check really necessary? > newIndex is the index of the selected item in |dataModel|, so it should be less > than its length.
PTAL at new changes. https://codereview.chromium.org/795483004/diff/60001/chrome/browser/resources... File chrome/browser/resources/bookmark_manager/js/main.js (right): https://codereview.chromium.org/795483004/diff/60001/chrome/browser/resources... chrome/browser/resources/bookmark_manager/js/main.js:1032: var newIndex, selectedItem = bmm.list.selectedItem; On 2014/12/23 11:54:36, Bernhard Bauer wrote: > Declare each variable on a separate line. Done. https://codereview.chromium.org/795483004/diff/60001/chrome/browser/resources... chrome/browser/resources/bookmark_manager/js/main.js:1106: if (newIndex && newIndex <= index) On 2014/12/23 11:54:36, Bernhard Bauer wrote: > You probably want to check for (newIndex != undefined) here, as 0 is also > considered false in Javascript. Also, is the second check really necessary? > newIndex is the index of the selected item in |dataModel|, so it should be less > than its length. I agree with you. Changes done as suggested.
PTAL.
xiyuan@chromium.org changed reviewers: + arv@chromium.org
Looks mostly good to me. But +arv to vet on the correctness of the CL since I have never touched this file before. https://codereview.chromium.org/795483004/diff/80001/chrome/browser/resources... File chrome/browser/resources/bookmark_manager/js/main.js (right): https://codereview.chromium.org/795483004/diff/80001/chrome/browser/resources... chrome/browser/resources/bookmark_manager/js/main.js:1060: index = newIndex; As Bernhard pointed out, |newIndex| does not necessary points at the correct item here. One possible case is when this callback happened after bookmark sync kicks in and brings in extra bookmarks in dataModel before the newly created item. We probably should use the index in the BookmarkTreeNode passed in chrome.bookmarks.create callback instead of |newIndex|. See "createFolder(function(newNode) {" at line 1048 of this patch.
@xiyuan PTAL. https://codereview.chromium.org/795483004/diff/80001/chrome/browser/resources... File chrome/browser/resources/bookmark_manager/js/main.js (right): https://codereview.chromium.org/795483004/diff/80001/chrome/browser/resources... chrome/browser/resources/bookmark_manager/js/main.js:1060: index = newIndex; On 2015/01/07 17:14:03, xiyuan wrote: > As Bernhard pointed out, |newIndex| does not necessary points at the correct > item here. One possible case is when this callback happened after bookmark sync > kicks in and brings in extra bookmarks in dataModel before the newly created > item. > > We probably should use the index in the BookmarkTreeNode passed in > chrome.bookmarks.create callback instead of |newIndex|. See > "createFolder(function(newNode) {" at line 1048 of this patch. @xiyuan Actually the code for bmm.tree and the bmm.list case is separate. editNewFolderInList() does not get called for bmm.tree case. and 'if ((opt_target || document.activeElement) == bmm.tree)' get executed in bmm.tree case only. This patch is for bmm.list case. I agree with you & Bernhard that I should not use newIndex here, As its value can be changed. So I am using the index that is present in the callback, as callback comes with the newly added node. I am making changes and uploading again. PTAL.
https://codereview.chromium.org/795483004/diff/100001/chrome/browser/resource... File chrome/browser/resources/bookmark_manager/js/main.js (right): https://codereview.chromium.org/795483004/diff/100001/chrome/browser/resource... chrome/browser/resources/bookmark_manager/js/main.js:1062: index = newIndex; I think we can replace line 1058-1062 to just var index = newNode.index; since |newNode| should always has the correct index that we want to use.
@xiyuan Thanks for review and suggestion. Changes done. PTAL. https://codereview.chromium.org/795483004/diff/100001/chrome/browser/resource... File chrome/browser/resources/bookmark_manager/js/main.js (right): https://codereview.chromium.org/795483004/diff/100001/chrome/browser/resource... chrome/browser/resources/bookmark_manager/js/main.js:1062: index = newIndex; On 2015/01/08 17:31:37, xiyuan wrote: > I think we can replace line 1058-1062 to just > var index = newNode.index; > > since |newNode| should always has the correct index that we want to use. Done.
lgtm
The CQ bit was checked by deepak.m1@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/795483004/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/829809ec98802b1d19d8f1793d90079833899560 Cr-Commit-Position: refs/heads/master@{#310719} |
