|
|
Description[MD Bookmarks] Add Select for Bookmarks.
This CL enables single click to select items in the list.
- Shift select.
- Ctrl select.
BUG=658980
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2639453002
Cr-Commit-Position: refs/heads/master@{#448209}
Committed: https://chromium.googlesource.com/chromium/src/+/feacdc993b7e6657fb5daa957de6e50a0fc6970b
Patch Set 1 #
Total comments: 9
Patch Set 2 : some fix #
Total comments: 1
Patch Set 3 : comment #
Total comments: 34
Patch Set 4 : selection after deletion #
Total comments: 19
Patch Set 5 : clean up #
Total comments: 20
Patch Set 6 : some more clean up #
Total comments: 22
Patch Set 7 : merge #Patch Set 8 : restructure #
Total comments: 17
Patch Set 9 : clean up #
Total comments: 20
Patch Set 10 : fix closure #
Total comments: 2
Patch Set 11 : add shift+ctrl behaviours #
Total comments: 2
Patch Set 12 : add removeChild #Patch Set 13 : add removeChild #
Total comments: 4
Patch Set 14 : minor fix #
Total comments: 7
Patch Set 15 : fixed some comments #Dependent Patchsets: Messages
Total messages: 45 (13 generated)
Description was changed from ========== [MD Bookmarks] Add Select for Bookmarks. This CL enables single click to select items in the list. - Shift select. - Ctrl select. BUG=658980 ========== to ========== [MD Bookmarks] Add Select for Bookmarks. This CL enables single click to select items in the list. - Shift select. - Ctrl select. BUG=658980 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
jiaxi@google.com changed reviewers: + angelayang@google.com
https://codereview.chromium.org/2639453002/diff/1/chrome/browser/resources/md... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2639453002/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_bookmarks/store.js:275: this.idToNodeMap_[this.selectedId].path + '.isSelectedFolder', false); ?
https://codereview.chromium.org/2639453002/diff/1/chrome/browser/resources/md... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2639453002/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_bookmarks/store.js:275: this.idToNodeMap_[this.selectedId].path + '.isSelectedFolder', false); On 2017/01/16 23:48:39, jiaxi wrote: > ? Oops wrong click, ignore me.
nice work! https://codereview.chromium.org/2639453002/diff/1/chrome/browser/resources/md... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2639453002/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_bookmarks/store.js:56: 'folder-open-changed': this.onFolderOpenChanged_.bind(this), Nice, since you've got yours in alphabetical order, we might as well move folder-open-changed and search-term-changed into order too https://codereview.chromium.org/2639453002/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_bookmarks/store.js:134: if (results) { Can we remove this outer if? or does no results give null? https://codereview.chromium.org/2639453002/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_bookmarks/store.js:185: if (this.displayedList[i].isSelected == true) { can we make this, if (this.displayedList[i].isSelected) ? https://codereview.chromium.org/2639453002/diff/1/chrome/test/data/webui/md_b... File chrome/test/data/webui/md_bookmarks/store_test.js (right): https://codereview.chromium.org/2639453002/diff/1/chrome/test/data/webui/md_b... chrome/test/data/webui/md_bookmarks/store_test.js:202: callback(EMPTY_RESULT); cool we can make this: callback([]) and remove EMPTY_RESULT
https://codereview.chromium.org/2639453002/diff/1/chrome/browser/resources/md... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2639453002/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_bookmarks/store.js:56: 'folder-open-changed': this.onFolderOpenChanged_.bind(this), On 2017/01/17 00:46:51, angelayang wrote: > Nice, since you've got yours in alphabetical order, we might as well move > folder-open-changed and search-term-changed into order too Done. https://codereview.chromium.org/2639453002/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_bookmarks/store.js:134: if (results) { On 2017/01/17 00:46:51, angelayang wrote: > Can we remove this outer if? or does no results give null? It returns an empty array [] https://codereview.chromium.org/2639453002/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_bookmarks/store.js:185: if (this.displayedList[i].isSelected == true) { On 2017/01/17 00:46:51, angelayang wrote: > can we make this, if (this.displayedList[i].isSelected) ? Done.
lgtm https://codereview.chromium.org/2639453002/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2639453002/diff/20001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:305: * Selected multiple items based on |prevSelectedItemIndex_| and the selected nit. Select
jiaxi@google.com changed reviewers: + calamity@chromium.org, tsergeant@chromium.org
Cool, here's a few comments. https://codereview.chromium.org/2639453002/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/item.html (right): https://codereview.chromium.org/2639453002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/item.html:21: background-color: rgb(216, 223, 240); Is this the right color? The spec says it should be rgba(66, 133, 244, 0.16) on a white background, which ends up being 225, 235, 253. https://codereview.chromium.org/2639453002/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/item.js (right): https://codereview.chromium.org/2639453002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/item.js:53: onSingleClick_: function(e) { nit: Just onClick_ https://codereview.chromium.org/2639453002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/item.js:65: }); Braces go around all multiline clauses. But in any case, you should just fire the same event for all 3 cases that has the shift and ctrl key inside the detail object. Also, I'd feel more comfortable firing item ids rather than the actual item. Any opinions Tim? https://codereview.chromium.org/2639453002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/item.js:72: onDoubleClick_: function(e) { onDblClick_ https://codereview.chromium.org/2639453002/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2639453002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:128: this.prevSelectedItemIndex_ = undefined; Can this and the if clause above move into this.clearSelectedItems_? https://codereview.chromium.org/2639453002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:198: getSelectedIndex_: function(item) { It would be better to add the index to the BookmarkTreeNode when the search comes back. Do it under a different field so that it doesn't interfere with other code that uses item.index. Then this code becomes return item.searchResultIndex || item.index; Also, getSelectedIndex_ sounds like something that gets the currently selected index. getIndexInList_ is probably better. https://codereview.chromium.org/2639453002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:208: //////// bookmarks-store, bookmarks API event listeners: clang-format janked this up. Delete back to 2 slashes. Check the others too. https://codereview.chromium.org/2639453002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:301: this.prevSelectedItemIndex_ = this.getSelectedIndex_(e.detail.item); What happens if the item at prevSelectedItemIndex_ gets deleted? https://codereview.chromium.org/2639453002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:322: for (var i = startIndex; i <= endIndex; i++) Braces around non-single line clauses. https://codereview.chromium.org/2639453002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:368: bookmarkNode.isSelected = false; Maybe isSelectedItem so it's more obvious which is which. https://codereview.chromium.org/2639453002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/md_bookmarks/item_test.js (right): https://codereview.chromium.org/2639453002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/item_test.js:51: }); Add all 3 event listeners at the beginning and assert for all values each time. Consider that the 'single-select-item' action might trigger all the other event listeners. That wouldn't get caught by this test. You can use map to make this a little nicer (check comment in other test). https://codereview.chromium.org/2639453002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/md_bookmarks/store_test.js (right): https://codereview.chromium.org/2639453002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/store_test.js:180: var SEARCH_RESULTS = TEST_TREE.children[0].children; Can we just make these the same results as before, except as items? https://codereview.chromium.org/2639453002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/store_test.js:294: assertFalse(store.idToNodeMap_['7'].isSelected); Consider doing what happens in history_list_test.js:83 for conciseness. (i.e use map) https://codereview.chromium.org/2639453002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/md_bookmarks/test_util.js (right): https://codereview.chromium.org/2639453002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/test_util.js:78: function customClick(element, config) { I would fill out config with a useful set of defaults just in case and then copy things from the arg. See history's test_util.js:shiftClick for an example of sane defaults.
https://codereview.chromium.org/2639453002/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/item.js (right): https://codereview.chromium.org/2639453002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/item.js:65: }); On 2017/01/17 06:12:22, calamity wrote: > Braces go around all multiline clauses. > > But in any case, you should just fire the same event for all 3 cases that has > the shift and ctrl key inside the detail object. > > Also, I'd feel more comfortable firing item ids rather than the actual item. Any > opinions Tim? Yeah, just fire IDs unless there's a good reason not to.
https://codereview.chromium.org/2639453002/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2639453002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:138: } Can we just set isSelected on the nodes inside results instead? https://codereview.chromium.org/2639453002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:183: if (this.displayedList[i].isSelected) { Invert and early continue.
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
Hi, massive patch https://codereview.chromium.org/2639453002/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/item.html (right): https://codereview.chromium.org/2639453002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/item.html:21: background-color: rgb(216, 223, 240); On 2017/01/17 06:12:22, calamity wrote: > Is this the right color? The spec says it should be rgba(66, 133, 244, 0.16) on > a white background, which ends up being 225, 235, 253. Done. https://codereview.chromium.org/2639453002/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/item.js (right): https://codereview.chromium.org/2639453002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/item.js:53: onSingleClick_: function(e) { On 2017/01/17 06:12:22, calamity wrote: > nit: Just onClick_ Done. https://codereview.chromium.org/2639453002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/item.js:65: }); On 2017/01/17 06:12:22, calamity wrote: > Braces go around all multiline clauses. > > But in any case, you should just fire the same event for all 3 cases that has > the shift and ctrl key inside the detail object. > > Also, I'd feel more comfortable firing item ids rather than the actual item. Any > opinions Tim? Done. https://codereview.chromium.org/2639453002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/item.js:72: onDoubleClick_: function(e) { On 2017/01/17 06:12:22, calamity wrote: > onDblClick_ Done. https://codereview.chromium.org/2639453002/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2639453002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:128: this.prevSelectedItemIndex_ = undefined; On 2017/01/17 06:12:22, calamity wrote: > Can this and the if clause above move into this.clearSelectedItems_? The prevSelectedItemIndex_ cannot be reset in this.clearSelectedItems_, this.clearSelectedItems is ran by all select events at the beginning but we want to preserve the prevSelectedItemIndex_. The only time prevSelectedItemIndex_ should be reset is when the displayedList is changed either by a search or by changing selected folder. https://codereview.chromium.org/2639453002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:138: } On 2017/01/18 03:25:37, calamity wrote: > Can we just set isSelected on the nodes inside results instead? There's a bunch of changes around this part of the code and it is no longer needed to set the isSelectedItem here. https://codereview.chromium.org/2639453002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:183: if (this.displayedList[i].isSelected) { On 2017/01/18 03:25:37, calamity wrote: > Invert and early continue. Done. https://codereview.chromium.org/2639453002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:198: getSelectedIndex_: function(item) { On 2017/01/17 06:12:22, calamity wrote: > It would be better to add the index to the BookmarkTreeNode when the search > comes back. Do it under a different field so that it doesn't interfere with > other code that uses item.index. Then this code becomes > > return item.searchResultIndex || item.index; > > Also, getSelectedIndex_ sounds like something that gets the currently selected > index. getIndexInList_ is probably better. Done. https://codereview.chromium.org/2639453002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:208: //////// bookmarks-store, bookmarks API event listeners: On 2017/01/17 06:12:22, calamity wrote: > clang-format janked this up. Delete back to 2 slashes. Check the others too. Done. https://codereview.chromium.org/2639453002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:301: this.prevSelectedItemIndex_ = this.getSelectedIndex_(e.detail.item); On 2017/01/17 06:12:22, calamity wrote: > What happens if the item at prevSelectedItemIndex_ gets deleted? The prevSelectedItemIndex_ will not change unless it's the last item in the displayedList, then it will turn into prevSelectedItem_--. Behaviors for selected item under deletion has been added. https://codereview.chromium.org/2639453002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:322: for (var i = startIndex; i <= endIndex; i++) On 2017/01/17 06:12:22, calamity wrote: > Braces around non-single line clauses. Done. https://codereview.chromium.org/2639453002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:368: bookmarkNode.isSelected = false; On 2017/01/17 06:12:22, calamity wrote: > Maybe isSelectedItem so it's more obvious which is which. Done. https://codereview.chromium.org/2639453002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/md_bookmarks/item_test.js (right): https://codereview.chromium.org/2639453002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/item_test.js:51: }); On 2017/01/17 06:12:22, calamity wrote: > Add all 3 event listeners at the beginning and assert for all values each time. > Consider that the 'single-select-item' action might trigger all the other event > listeners. That wouldn't get caught by this test. > > You can use map to make this a little nicer (check comment in other test). Done. https://codereview.chromium.org/2639453002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/md_bookmarks/store_test.js (right): https://codereview.chromium.org/2639453002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/store_test.js:180: var SEARCH_RESULTS = TEST_TREE.children[0].children; On 2017/01/17 06:12:22, calamity wrote: > Can we just make these the same results as before, except as items? Done. https://codereview.chromium.org/2639453002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/store_test.js:294: assertFalse(store.idToNodeMap_['7'].isSelected); On 2017/01/17 06:12:22, calamity wrote: > Consider doing what happens in history_list_test.js:83 for conciseness. > > (i.e use map) Done. https://codereview.chromium.org/2639453002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/md_bookmarks/test_util.js (right): https://codereview.chromium.org/2639453002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/test_util.js:78: function customClick(element, config) { On 2017/01/17 06:12:22, calamity wrote: > I would fill out config with a useful set of defaults just in case and then copy > things from the arg. See history's test_util.js:shiftClick for an example of > sane defaults. Done. https://codereview.chromium.org/2639453002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/folder_node.js (right): https://codereview.chromium.org/2639453002/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/folder_node.js:59: This helps when a folder has been removed but the parent still thinks there's a folder.
https://codereview.chromium.org/2639453002/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2639453002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:128: this.prevSelectedItemIndex_ = undefined; On 2017/01/20 04:51:09, jiaxi wrote: > On 2017/01/17 06:12:22, calamity wrote: > > Can this and the if clause above move into this.clearSelectedItems_? > > The prevSelectedItemIndex_ cannot be reset in this.clearSelectedItems_, > this.clearSelectedItems is ran by all select events at the beginning but we want > to preserve the prevSelectedItemIndex_. The only time prevSelectedItemIndex_ > should be reset is when the displayedList is changed either by a search or by > changing selected folder. Acknowledged. https://codereview.chromium.org/2639453002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/folder_node.js (right): https://codereview.chromium.org/2639453002/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/folder_node.js:59: On 2017/01/20 04:51:09, jiaxi wrote: > This helps when a folder has been removed but the parent still thinks there's a > folder. Does this get recalculated properly? You may need to set the computed binding to depend on item.children.splices or item.children.length for this to recalculate at delete time. https://codereview.chromium.org/2639453002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/item.html (right): https://codereview.chromium.org/2639453002/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/item.html:66: on-click="onMenuButtonOpenClick_"> Why did this change? https://codereview.chromium.org/2639453002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2639453002/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:128: this.idToNodeMap_[results[i].id].searchResultIndex = i; Why do we have to update the nodes inside the tree still? Ah. After reading more of the patch, I realize it's because I told you to send the id through the event. This was a mistake. You should send the item up so that you can directly deal with them and not have to access things through the idToNodeMap_. From a data model perspective, we want a clear separation of things in the tree and things in the search results. Trying to match the search results with the tree is messy and error-prone, and we should only do it when absolutely necessary. This property should be getting set on the search results. Not on the tree. https://codereview.chromium.org/2639453002/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:208: chrome.bookmarks.getSubTree(removeInfo.parentId, function(parentNode) { Does this cancel all existing selections? https://codereview.chromium.org/2639453002/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:210: // Check if the removed item is from the list. This only checks if the removed item is the previously selected item? Is this intentional? https://codereview.chromium.org/2639453002/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:213: if (id == this.displayedList[this.prevSelectedItemIndex_].id) { Use && for these 2 conditions? https://codereview.chromium.org/2639453002/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:222: parentNode[0].path = this.idToNodeMap_[parentNode[0].id].path; Change the arg to the function to parentNodes and then make a var parentNode = parentNodes[0]. I don't understand why the API gives back an array. https://codereview.chromium.org/2639453002/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:248: this._setDisplayedList(parentNode[0].children); This assumes that removed bookmarks are always being displayed. As mentioned, this gets called if bookmarks are removed from the bookmarks bar and should be responsive. https://codereview.chromium.org/2639453002/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:254: true); I'm not sure what this does?
Patchset #5 (id:120001) has been deleted
https://codereview.chromium.org/2639453002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/folder_node.js (right): https://codereview.chromium.org/2639453002/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/folder_node.js:59: On 2017/01/23 00:44:27, calamity wrote: > On 2017/01/20 04:51:09, jiaxi wrote: > > This helps when a folder has been removed but the parent still thinks there's > a > > folder. > > Does this get recalculated properly? You may need to set the computed binding to > depend on item.children.splices or item.children.length for this to recalculate > at delete time. Done. https://codereview.chromium.org/2639453002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/item.html (right): https://codereview.chromium.org/2639453002/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/item.html:66: on-click="onMenuButtonOpenClick_"> On 2017/01/23 00:44:27, calamity wrote: > Why did this change? To stop event from bubbling up and select the item. Do you think we need the item to get selected to match the old bmm behaviour? https://codereview.chromium.org/2639453002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2639453002/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:128: this.idToNodeMap_[results[i].id].searchResultIndex = i; On 2017/01/23 00:44:27, calamity wrote: > Why do we have to update the nodes inside the tree still? > > Ah. After reading more of the patch, I realize it's because I told you to send > the id through the event. This was a mistake. You should send the item up so > that you can directly deal with them and not have to access things through the > idToNodeMap_. > > From a data model perspective, we want a clear separation of things in the tree > and things in the search results. Trying to match the search results with the > tree is messy and error-prone, and we should only do it when absolutely > necessary. > > This property should be getting set on the search results. Not on the tree. Done. https://codereview.chromium.org/2639453002/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:208: chrome.bookmarks.getSubTree(removeInfo.parentId, function(parentNode) { On 2017/01/23 00:44:27, calamity wrote: > Does this cancel all existing selections? All existing selections will be canceled when the displayedList gets set. https://codereview.chromium.org/2639453002/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:210: // Check if the removed item is from the list. On 2017/01/23 00:44:27, calamity wrote: > This only checks if the removed item is the previously selected item? Is this > intentional? Now it checks if the removed item is in the displayedList or not instead. In old bmm no matter how many items we removed, there's only one item selected after the removal. https://codereview.chromium.org/2639453002/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:213: if (id == this.displayedList[this.prevSelectedItemIndex_].id) { On 2017/01/23 00:44:27, calamity wrote: > Use && for these 2 conditions? This part has been changed a lot so... https://codereview.chromium.org/2639453002/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:222: parentNode[0].path = this.idToNodeMap_[parentNode[0].id].path; On 2017/01/23 00:44:27, calamity wrote: > Change the arg to the function to parentNodes and then make a var parentNode = > parentNodes[0]. I don't understand why the API gives back an array. Done. https://codereview.chromium.org/2639453002/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:248: this._setDisplayedList(parentNode[0].children); On 2017/01/23 00:44:27, calamity wrote: > This assumes that removed bookmarks are always being displayed. As mentioned, > this gets called if bookmarks are removed from the bookmarks bar and should be > responsive. Done. https://codereview.chromium.org/2639453002/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:254: true); On 2017/01/23 00:44:27, calamity wrote: > I'm not sure what this does? This selects the item in the displayedList since we cleared everything selected.
https://codereview.chromium.org/2639453002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2639453002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:41: prevSelectedItemIndex_: Number, Hmm. I got confused by this thinking it was the actual previous selection index. This is probably better called anchorIndex as in list_selection_controller.js. https://codereview.chromium.org/2639453002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:192: return item.index; I think this can just be return item.searchResultIndex || item.index; https://codereview.chromium.org/2639453002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:202: } You may want to think about caching this information in a map for fast lookup instead of iterating. https://codereview.chromium.org/2639453002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:204: return (this.idToNodeMap_[id].parentId == this.selectedId); nit: No parens. https://codereview.chromium.org/2639453002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:219: chrome.bookmarks.getSubTree(removeInfo.parentId, function(parentNode) { Call this parentNodes so it's not confused with the var below. https://codereview.chromium.org/2639453002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:225: this.removeDescendantsFromMap_(id); Should this be replacing the parent node? Is this even necessary since you're setting a new parentNode later anyway? https://codereview.chromium.org/2639453002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:238: if (isInDisplayedList) { Invert and early return. https://codereview.chromium.org/2639453002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:245: results[i].searchResultIndex = i; This code should really be shared with the searching code. https://codereview.chromium.org/2639453002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:252: true); What if prevSelectedItemIndex is null? https://codereview.chromium.org/2639453002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:352: var currIndex = this.getIndexInList_(item); nit: selectedIndex.
Patchset #6 (id:160001) has been deleted
https://codereview.chromium.org/2639453002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2639453002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:41: prevSelectedItemIndex_: Number, On 2017/01/24 06:39:17, calamity wrote: > Hmm. I got confused by this thinking it was the actual previous selection index. > This is probably better called anchorIndex as in list_selection_controller.js. Done. https://codereview.chromium.org/2639453002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:192: return item.index; On 2017/01/24 06:39:17, calamity wrote: > I think this can just be return item.searchResultIndex || item.index; This will return undefined if we select the first item in the displayedList while searching (item.searchResultIndex = 0 = false in js). https://codereview.chromium.org/2639453002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:202: } On 2017/01/24 06:39:17, calamity wrote: > You may want to think about caching this information in a map for fast lookup > instead of iterating. Done. https://codereview.chromium.org/2639453002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:204: return (this.idToNodeMap_[id].parentId == this.selectedId); On 2017/01/24 06:39:17, calamity wrote: > nit: No parens. Done. https://codereview.chromium.org/2639453002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:219: chrome.bookmarks.getSubTree(removeInfo.parentId, function(parentNode) { On 2017/01/24 06:39:17, calamity wrote: > Call this parentNodes so it's not confused with the var below. Done. https://codereview.chromium.org/2639453002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:225: this.removeDescendantsFromMap_(id); On 2017/01/24 06:39:17, calamity wrote: > Should this be replacing the parent node? Is this even necessary since you're > setting a new parentNode later anyway? Discussed offline. This will remove all the extra nodes in the idToNodeMap_. https://codereview.chromium.org/2639453002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:238: if (isInDisplayedList) { On 2017/01/24 06:39:17, calamity wrote: > Invert and early return. Done. https://codereview.chromium.org/2639453002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:245: results[i].searchResultIndex = i; On 2017/01/24 06:39:17, calamity wrote: > This code should really be shared with the searching code. Done. https://codereview.chromium.org/2639453002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:252: true); On 2017/01/24 06:39:17, calamity wrote: > What if prevSelectedItemIndex is null? The prevSelectedItemIndex(anchorIndex) can only be either undefined or a number. When it is undefined it will not select anything. https://codereview.chromium.org/2639453002/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:352: var currIndex = this.getIndexInList_(item); On 2017/01/24 06:39:17, calamity wrote: > nit: selectedIndex. Done.
https://codereview.chromium.org/2639453002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/folder_node.html (right): https://codereview.chromium.org/2639453002/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/folder_node.html:77: <template is="dom-repeat" items="[[getFolders_(item.children)]]" dom-repeat has a built-in feature for filtering the list: https://www.polymer-project.org/1.0/docs/devguide/templates#filtering-and-sor... Using that is probably better for performance and should work properly with data binding notifications. As it is, this breaks paths and prevents the isSelectedFolder highlught from showing up for nested folders. https://codereview.chromium.org/2639453002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/item.js (right): https://codereview.chromium.org/2639453002/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/item.js:70: * @param {Event} e This annotation is wrong https://codereview.chromium.org/2639453002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2639453002/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:43: idToSearchResultMap_: Object, I recently discovered how to closure annotate something like this: /** @type{Object<string, !BookmarkTreeNode>} */ https://codereview.chromium.org/2639453002/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:198: return this.searchTerm ? this.idToSearchResultMap_[id] : This doesn't always return a boolean? Sometimes it returns an Element? If you want to convert that to a boolean, you can do !! eg, !!this.idToSearchResultMap_[id] https://codereview.chromium.org/2639453002/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:207: * @param {array<BookmarkTreeNode>} item The Array type has an uppercase 'A' https://codereview.chromium.org/2639453002/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:214: } Can you set `isSelectedItem = false` in here? It's always set for regular results, so it seems strange that it might not happen for search results. https://codereview.chromium.org/2639453002/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:237: // Update the tree. Updates the tree...for what reason? The fact that you've gone to all this trouble to do it this way makes me suspicious that something subtle but important is happening. It would be good to make sure that comments explain anything unusual that's going on. https://codereview.chromium.org/2639453002/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:258: chrome.bookmarks.search(this.searchTerm, function(results) { This spooks me a little because there are now two places where a search can happen. It seems like it would be easy to add some logic to updateSearchDisplay_() that doesn't get called when this version of search happens. Is it possible to just call updateSearchDisplay here? Is there a good reason not to? https://codereview.chromium.org/2639453002/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:385: onItemSelected_: function(e) { Since store is supposed to be isolated from UI, I would be happier to name these functions and variables according to what they do to the data rather than what keys they correspond to. "ctrl" means "add to the selection" and "shift" means "select a range". So, the Event structure could become {add: Boolean, range: Boolean} Reframing it in this way means that we're just thinking about the data modifications that need to be made, which means we can simplify the code. If `e.add` is *false*, then we need to clear the selection before doing anything else. If `e.range` is true, then we need to do a range selection. Otherwise, we select just a single item. So the whole onItemSelected_ function becomes something like: if (!e.add) this.clearSelectedItems_() if (e.range) this.selectRange_(index) else this.selectItem_(index) Hooray! Now there's one fewer function needed, all of the functions have names which describe how they modify the data, and it's possible to use Ctrl and Shift and the same time. https://codereview.chromium.org/2639453002/diff/180001/chrome/test/data/webui... File chrome/test/data/webui/md_bookmarks/store_test.js (right): https://codereview.chromium.org/2639453002/diff/180001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/store_test.js:157: DELETE_RESULTS = [createFolder('0', [ Overwriting this is a bit strange -- the ALLCAPS_NAME is meant to indicate that it's a constant. Perhaps do what Angela does now and add a convenience method for overwriting the getSubtree function? https://cs.chromium.org/chromium/src/chrome/test/data/webui/md_bookmarks/stor... https://codereview.chromium.org/2639453002/diff/180001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/store_test.js:217: chrome.bookmarks.search = function(searchTerm, callback) { You're gonna need to rebase to pick up Angela's change to this test. It's gonna be nasty. Maybe do it after the rest of the changes here?
Patchset #8 (id:220001) has been deleted
https://codereview.chromium.org/2639453002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/folder_node.html (right): https://codereview.chromium.org/2639453002/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/folder_node.html:77: <template is="dom-repeat" items="[[getFolders_(item.children)]]" On 2017/01/25 06:02:13, tsergeant wrote: > dom-repeat has a built-in feature for filtering the list: > > https://www.polymer-project.org/1.0/docs/devguide/templates#filtering-and-sor... > > Using that is probably better for performance and should work properly with data > binding notifications. As it is, this breaks paths and prevents the > isSelectedFolder highlught from showing up for nested folders. Done. https://codereview.chromium.org/2639453002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/item.js (right): https://codereview.chromium.org/2639453002/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/item.js:70: * @param {Event} e On 2017/01/25 06:02:13, tsergeant wrote: > This annotation is wrong Done. https://codereview.chromium.org/2639453002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2639453002/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:43: idToSearchResultMap_: Object, On 2017/01/25 06:02:13, tsergeant wrote: > I recently discovered how to closure annotate something like this: > > /** @type{Object<string, !BookmarkTreeNode>} */ Done. https://codereview.chromium.org/2639453002/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:198: return this.searchTerm ? this.idToSearchResultMap_[id] : On 2017/01/25 06:02:13, tsergeant wrote: > This doesn't always return a boolean? Sometimes it returns an Element? > > If you want to convert that to a boolean, you can do !! > eg, > > !!this.idToSearchResultMap_[id] Discussed offline, changing this map into a set. https://codereview.chromium.org/2639453002/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:207: * @param {array<BookmarkTreeNode>} item On 2017/01/25 06:02:13, tsergeant wrote: > The Array type has an uppercase 'A' Done. https://codereview.chromium.org/2639453002/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:214: } On 2017/01/25 06:02:13, tsergeant wrote: > Can you set `isSelectedItem = false` in here? It's always set for regular > results, so it seems strange that it might not happen for search results. Done. https://codereview.chromium.org/2639453002/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:237: // Update the tree. On 2017/01/25 06:02:13, tsergeant wrote: > Updates the tree...for what reason? > > The fact that you've gone to all this trouble to do it this way makes me > suspicious that something subtle but important is happening. It would be good to > make sure that comments explain anything unusual that's going on. Done. https://codereview.chromium.org/2639453002/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:258: chrome.bookmarks.search(this.searchTerm, function(results) { On 2017/01/25 06:02:13, tsergeant wrote: > This spooks me a little because there are now two places where a search can > happen. It seems like it would be easy to add some logic to > updateSearchDisplay_() that doesn't get called when this version of search > happens. > > Is it possible to just call updateSearchDisplay here? Is there a good reason not > to? I can't think of a good way to using promise to reuse updateSearchDisplay. The newest patch uses tricky way that updateSearchDisplay doesn't have a parameter when been called not as an observer to reuse the code. https://codereview.chromium.org/2639453002/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:385: onItemSelected_: function(e) { On 2017/01/25 06:02:13, tsergeant wrote: > Since store is supposed to be isolated from UI, I would be happier to name these > functions and variables according to what they do to the data rather than what > keys they correspond to. > > "ctrl" means "add to the selection" and "shift" means "select a range". > > So, the Event structure could become > > {add: Boolean, > range: Boolean} > > Reframing it in this way means that we're just thinking about the data > modifications that need to be made, which means we can simplify the code. > > If `e.add` is *false*, then we need to clear the selection before doing anything > else. > > If `e.range` is true, then we need to do a range selection. Otherwise, we select > just a single item. > > So the whole onItemSelected_ function becomes something like: > > if (!e.add) > this.clearSelectedItems_() > if (e.range) > this.selectRange_(index) > else > this.selectItem_(index) > > Hooray! Now there's one fewer function needed, all of the functions have names > which describe how they modify the data, and it's possible to use Ctrl and Shift > and the same time. The old bmm doesn't support Ctrl and Shift operation at the same time. The Shift will overwrite everything. I've made the change so it matches the bahaviours of the old bmm. Also, should we do tests for the rangeSelect and addSelect in the store-test instead of testing single select, shift select and ctrl select? https://codereview.chromium.org/2639453002/diff/180001/chrome/test/data/webui... File chrome/test/data/webui/md_bookmarks/store_test.js (right): https://codereview.chromium.org/2639453002/diff/180001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/store_test.js:157: DELETE_RESULTS = [createFolder('0', [ On 2017/01/25 06:02:13, tsergeant wrote: > Overwriting this is a bit strange -- the ALLCAPS_NAME is meant to indicate that > it's a constant. > > Perhaps do what Angela does now and add a convenience method for overwriting the > getSubtree function? > > https://cs.chromium.org/chromium/src/chrome/test/data/webui/md_bookmarks/stor... Done. https://codereview.chromium.org/2639453002/diff/180001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/store_test.js:217: chrome.bookmarks.search = function(searchTerm, callback) { On 2017/01/25 06:02:13, tsergeant wrote: > You're gonna need to rebase to pick up Angela's change to this test. It's gonna > be nasty. Maybe do it after the rest of the changes here? I've merged Angela's patch and it took a while to fix all these tests.
Thanks for the patience~! https://codereview.chromium.org/2639453002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/folder_node.html (right): https://codereview.chromium.org/2639453002/diff/240001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/folder_node.html:78: filter="isFolder_" observe="isSelectedFolder" as="child"> I think you want observe="url", since the isFolder_ method looks specifically at item.url https://codereview.chromium.org/2639453002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/folder_node.js (right): https://codereview.chromium.org/2639453002/diff/240001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/folder_node.js:64: * @private Add an @param annotation https://codereview.chromium.org/2639453002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2639453002/diff/240001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:40: /** @type{Object<string, !BookmarkTreeNode>} */ Space after type: @type {} (sorry, I typoed this in my comments last time) https://codereview.chromium.org/2639453002/diff/240001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:45: /** @type{Object<string, !boolean>} */ Space after type here, too https://codereview.chromium.org/2639453002/diff/240001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:126: updateSearchDisplay_: function(searchObserver) { - Rename `searchObserver` to `searchTerm`, since that's what it actually is - Add an `@param {?string} searchTerm` annotation to mark searchTerm as optional - Add a comment explaining why this is necessary. https://codereview.chromium.org/2639453002/diff/240001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:134: if (searchObserver) This is an interesting way to solve this problem. @Chris -- I'm happy to go ahead with 'creative solution' in the interest of making forward progress on this CL and the things it is blocking. Do you agree? https://codereview.chromium.org/2639453002/diff/240001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:137: this.searchResultSet_ = {}; There's a Set class that you should use for this: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje... Which lets you do add(), clear() and has() https://codereview.chromium.org/2639453002/diff/240001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:254: // correct index. Update this comment to explain that since Polymer doesn't update the indexes, you need to do it manually.
https://codereview.chromium.org/2639453002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/folder_node.html (right): https://codereview.chromium.org/2639453002/diff/240001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/folder_node.html:78: filter="isFolder_" observe="isSelectedFolder" as="child"> On 2017/01/30 06:39:47, tsergeant wrote: > I think you want observe="url", since the isFolder_ method looks specifically at > item.url Done. https://codereview.chromium.org/2639453002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/folder_node.js (right): https://codereview.chromium.org/2639453002/diff/240001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/folder_node.js:64: * @private On 2017/01/30 06:39:47, tsergeant wrote: > Add an @param annotation Done. https://codereview.chromium.org/2639453002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2639453002/diff/240001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:40: /** @type{Object<string, !BookmarkTreeNode>} */ On 2017/01/30 06:39:47, tsergeant wrote: > Space after type: > > @type {} > > (sorry, I typoed this in my comments last time) Done. https://codereview.chromium.org/2639453002/diff/240001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:45: /** @type{Object<string, !boolean>} */ On 2017/01/30 06:39:47, tsergeant wrote: > Space after type here, too Since this is a set object now do we need closure for it? https://codereview.chromium.org/2639453002/diff/240001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:126: updateSearchDisplay_: function(searchObserver) { On 2017/01/30 06:39:47, tsergeant wrote: > - Rename `searchObserver` to `searchTerm`, since that's what it actually is > > - Add an `@param {?string} searchTerm` annotation to mark searchTerm as optional > > - Add a comment explaining why this is necessary. We're using this.searchTerm in this function. Will renaming the param to searchTerm be a bit confusing? https://codereview.chromium.org/2639453002/diff/240001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:137: this.searchResultSet_ = {}; On 2017/01/30 06:39:47, tsergeant wrote: > There's a Set class that you should use for this: > > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje... > > Which lets you do add(), clear() and has() Done. https://codereview.chromium.org/2639453002/diff/240001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:254: // correct index. On 2017/01/30 06:39:47, tsergeant wrote: > Update this comment to explain that since Polymer doesn't update the indexes, > you need to do it manually. Done. I moved selectRange and selectItem to the right section so this looks a bit messy. The only thing changed in this part of the code is the comment.
https://codereview.chromium.org/2639453002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2639453002/diff/240001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:45: /** @type{Object<string, !boolean>} */ On 2017/01/31 00:37:42, jiaxi wrote: > On 2017/01/30 06:39:47, tsergeant wrote: > > Space after type here, too > > Since this is a set object now do we need closure for it? You should be able to add /** @type {Set<string>} */
Patchset #10 (id:280001) has been deleted
https://codereview.chromium.org/2639453002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2639453002/diff/240001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:45: /** @type{Object<string, !boolean>} */ On 2017/01/31 01:55:35, tsergeant wrote: > On 2017/01/31 00:37:42, jiaxi wrote: > > On 2017/01/30 06:39:47, tsergeant wrote: > > > Space after type here, too > > > > Since this is a set object now do we need closure for it? > > You should be able to add /** @type {Set<string>} */ Done.
https://codereview.chromium.org/2639453002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/item.js (right): https://codereview.chromium.org/2639453002/diff/260001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/item.js:64: } Can this just be range: e.shiftKey, add: e.ctrlKey, without the if? This provides a better Ctrl + Shift click behavior. https://codereview.chromium.org/2639453002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2639453002/diff/260001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:288: var isInDisplayedList = this.isInDisplayedList_(id); Maybe call this 'wasInDisplayedList' to indicate that it's a value that's invalidated by the update. https://codereview.chromium.org/2639453002/diff/260001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:291: // the indexes. What does indexes mean? How about 'Refresh the parent node's data from the backend as its children's indexes will have changed.' s/Manually updates/Manually update/g https://codereview.chromium.org/2639453002/diff/260001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:309: this.anchorIndex_--; I'm pretty happy for everything to just be deselected and reset to something reasonable if any selected item is deleted. I'm a bit worried about the consequences of messing around with indexes like this. https://codereview.chromium.org/2639453002/diff/260001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:311: if (this.searchTerm) { Comment above here: // Update the currently displayed list. https://codereview.chromium.org/2639453002/diff/260001/chrome/test/data/webui... File chrome/test/data/webui/md_bookmarks/store_test.js (right): https://codereview.chromium.org/2639453002/diff/260001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/store_test.js:169: ])]); Can you splice the removed value out of the TEST_TREE instead? This is prone to not being updated properly if the TEST_TREE ever changes. https://codereview.chromium.org/2639453002/diff/260001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/store_test.js:362: } nit: remove braces. https://codereview.chromium.org/2639453002/diff/260001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/store_test.js:453: Add a test for Ctrl + Shift click behavior. Let me know if you want to see what kind of behavior I'm looking for. https://codereview.chromium.org/2639453002/diff/300001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2639453002/diff/300001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:43: /** @type {?number} */ Is this necessary?
https://codereview.chromium.org/2639453002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/item.js (right): https://codereview.chromium.org/2639453002/diff/260001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/item.js:64: } On 2017/01/31 05:05:15, calamity wrote: > Can this just be range: e.shiftKey, add: e.ctrlKey, without the if? This > provides a better Ctrl + Shift click behavior. Done. https://codereview.chromium.org/2639453002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2639453002/diff/260001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:288: var isInDisplayedList = this.isInDisplayedList_(id); On 2017/01/31 05:05:15, calamity wrote: > Maybe call this 'wasInDisplayedList' to indicate that it's a value that's > invalidated by the update. Done. https://codereview.chromium.org/2639453002/diff/260001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:291: // the indexes. On 2017/01/31 05:05:15, calamity wrote: > What does indexes mean? How about 'Refresh the parent node's data from the > backend as its children's indexes will have changed.' > > s/Manually updates/Manually update/g Done. https://codereview.chromium.org/2639453002/diff/260001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:309: this.anchorIndex_--; On 2017/01/31 05:05:15, calamity wrote: > I'm pretty happy for everything to just be deselected and reset to something > reasonable if any selected item is deleted. I'm a bit worried about the > consequences of messing around with indexes like this. Done. https://codereview.chromium.org/2639453002/diff/260001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:311: if (this.searchTerm) { On 2017/01/31 05:05:15, calamity wrote: > Comment above here: // Update the currently displayed list. Done. https://codereview.chromium.org/2639453002/diff/260001/chrome/test/data/webui... File chrome/test/data/webui/md_bookmarks/store_test.js (right): https://codereview.chromium.org/2639453002/diff/260001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/store_test.js:169: ])]); On 2017/01/31 05:05:15, calamity wrote: > Can you splice the removed value out of the TEST_TREE instead? This is prone to > not being updated properly if the TEST_TREE ever changes. I've changed everything that can be changed by splicing TEST_TREE. Using splice has the same issue in Polymer, the indexes don't get updated so I have to change the order of some tests. (Testing select after deletion is not going to work because of the messed up indexes). https://codereview.chromium.org/2639453002/diff/260001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/store_test.js:362: } On 2017/01/31 05:05:15, calamity wrote: > nit: remove braces. Done. https://codereview.chromium.org/2639453002/diff/260001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/store_test.js:453: On 2017/01/31 05:05:15, calamity wrote: > Add a test for Ctrl + Shift click behavior. Let me know if you want to see what > kind of behavior I'm looking for. Done. https://codereview.chromium.org/2639453002/diff/300001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2639453002/diff/300001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:43: /** @type {?number} */ On 2017/01/31 05:05:15, calamity wrote: > Is this necessary? We're setting anchorIndex_ to null in some situations so it is necessary.
I can't patch this in locally -- can you please do a rebase? (I think you're missing the routing CL). https://codereview.chromium.org/2639453002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2639453002/diff/260001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:288: var isInDisplayedList = this.isInDisplayedList_(id); On 2017/02/01 03:19:25, jiaxi wrote: > On 2017/01/31 05:05:15, calamity wrote: > > Maybe call this 'wasInDisplayedList' to indicate that it's a value that's > > invalidated by the update. > > Done. It makes sense to call the var `wasInDisplayedList`, but the function probably should stay as `isInDisplayedList`, right Chris? I think the idea is that at the time you assign the variable, present tense is appropriate, but when you use the variable later, the list has changed and so past tense makes more sense. https://codereview.chromium.org/2639453002/diff/260001/chrome/test/data/webui... File chrome/test/data/webui/md_bookmarks/store_test.js (right): https://codereview.chromium.org/2639453002/diff/260001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/store_test.js:169: ])]); On 2017/02/01 03:19:25, jiaxi wrote: > On 2017/01/31 05:05:15, calamity wrote: > > Can you splice the removed value out of the TEST_TREE instead? This is prone > to > > not being updated properly if the TEST_TREE ever changes. > > I've changed everything that can be changed by splicing TEST_TREE. Using splice > has the same issue in Polymer, the indexes don't get updated so I have to change > the order of some tests. (Testing select after deletion is not going to work > because of the messed up indexes). In that case, maybe it would be good to define a new test helper method like removeChild(tree, index) which splices out the item/folder at `index` and then adjusts the indices of all the items after that. https://codereview.chromium.org/2639453002/diff/320001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2639453002/diff/320001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:144: this.set( Does this line ever do anything useful? Can it be deleted?
Hi, I've done the rebase and hopefully this time it works https://codereview.chromium.org/2639453002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2639453002/diff/260001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:288: var isInDisplayedList = this.isInDisplayedList_(id); On 2017/02/02 00:16:02, tsergeant wrote: > On 2017/02/01 03:19:25, jiaxi wrote: > > On 2017/01/31 05:05:15, calamity wrote: > > > Maybe call this 'wasInDisplayedList' to indicate that it's a value that's > > > invalidated by the update. > > > > Done. > > It makes sense to call the var `wasInDisplayedList`, but the function probably > should stay as `isInDisplayedList`, right Chris? > > I think the idea is that at the time you assign the variable, present tense is > appropriate, but when you use the variable later, the list has changed and so > past tense makes more sense. Done. https://codereview.chromium.org/2639453002/diff/260001/chrome/test/data/webui... File chrome/test/data/webui/md_bookmarks/store_test.js (right): https://codereview.chromium.org/2639453002/diff/260001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/store_test.js:169: ])]); On 2017/02/02 00:16:02, tsergeant wrote: > On 2017/02/01 03:19:25, jiaxi wrote: > > On 2017/01/31 05:05:15, calamity wrote: > > > Can you splice the removed value out of the TEST_TREE instead? This is prone > > to > > > not being updated properly if the TEST_TREE ever changes. > > > > I've changed everything that can be changed by splicing TEST_TREE. Using > splice > > has the same issue in Polymer, the indexes don't get updated so I have to > change > > the order of some tests. (Testing select after deletion is not going to work > > because of the messed up indexes). > > In that case, maybe it would be good to define a new test helper method like > > removeChild(tree, index) > > which splices out the item/folder at `index` and then adjusts the indices of all > the items after that. Done. https://codereview.chromium.org/2639453002/diff/320001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2639453002/diff/320001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:144: this.set( On 2017/02/02 00:16:03, tsergeant wrote: > Does this line ever do anything useful? Can it be deleted? Done.
Looks good! Just a couple more minor things https://codereview.chromium.org/2639453002/diff/360001/chrome/test/data/webui... File chrome/test/data/webui/md_bookmarks/store_test.js (right): https://codereview.chromium.org/2639453002/diff/360001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/store_test.js:20: function removeChild(tree, index) { I think this should go in test_util.js with the rest of the tree helpers. https://codereview.chromium.org/2639453002/diff/360001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/store_test.js:428: Nit: Extra line here
https://codereview.chromium.org/2639453002/diff/360001/chrome/test/data/webui... File chrome/test/data/webui/md_bookmarks/store_test.js (right): https://codereview.chromium.org/2639453002/diff/360001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/store_test.js:20: function removeChild(tree, index) { On 2017/02/02 04:35:21, tsergeant wrote: > I think this should go in test_util.js with the rest of the tree helpers. Done. https://codereview.chromium.org/2639453002/diff/360001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/store_test.js:428: On 2017/02/02 04:35:21, tsergeant wrote: > Nit: Extra line here Done.
lgtm!
https://codereview.chromium.org/2639453002/diff/380001/chrome/test/data/webui... File chrome/test/data/webui/md_bookmarks/store_test.js (right): https://codereview.chromium.org/2639453002/diff/380001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/store_test.js:217: removeChild(TEST_TREE, 3); Shouldn't this test be removing the same node as in selectedId? https://codereview.chromium.org/2639453002/diff/380001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/store_test.js:461: // Delete the selected item will unselect everything. nit: Deleting https://codereview.chromium.org/2639453002/diff/380001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/store_test.js:476: // Shift+Ctrl select selects the right item. s/select // s/item/items/
https://codereview.chromium.org/2639453002/diff/380001/chrome/test/data/webui... File chrome/test/data/webui/md_bookmarks/store_test.js (right): https://codereview.chromium.org/2639453002/diff/380001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/store_test.js:217: removeChild(TEST_TREE, 3); On 2017/02/03 03:34:59, calamity wrote: > Shouldn't this test be removing the same node as in selectedId? This is removing the same node as in selectedId, selectedId is '8' and it has index 3 in TEST_TREE.children. I changed the selectedId from '2' to '8' because '2' is not a folder. https://codereview.chromium.org/2639453002/diff/380001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/store_test.js:461: // Delete the selected item will unselect everything. On 2017/02/03 03:34:59, calamity wrote: > nit: Deleting Done. https://codereview.chromium.org/2639453002/diff/380001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/store_test.js:476: // Shift+Ctrl select selects the right item. On 2017/02/03 03:34:59, calamity wrote: > s/select // > s/item/items/ Done.
lgtm https://codereview.chromium.org/2639453002/diff/380001/chrome/test/data/webui... File chrome/test/data/webui/md_bookmarks/store_test.js (right): https://codereview.chromium.org/2639453002/diff/380001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/store_test.js:217: removeChild(TEST_TREE, 3); On 2017/02/06 02:01:01, jiaxi wrote: > On 2017/02/03 03:34:59, calamity wrote: > > Shouldn't this test be removing the same node as in selectedId? > > This is removing the same node as in selectedId, selectedId is '8' and it has > index 3 in TEST_TREE.children. I changed the selectedId from '2' to '8' because > '2' is not a folder. Ah, removeChild works by index. My bad.
The CQ bit was checked by jiaxi@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from angelayang@google.com, tsergeant@chromium.org Link to the patchset: https://codereview.chromium.org/2639453002/#ps400001 (title: "fixed some comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 400001, "attempt_start_ts": 1486355777736530, "parent_rev": "f67d6bb3f50d878ab42f797042eacb184b7d803e", "commit_rev": "feacdc993b7e6657fb5daa957de6e50a0fc6970b"}
Message was sent while issue was closed.
Description was changed from ========== [MD Bookmarks] Add Select for Bookmarks. This CL enables single click to select items in the list. - Shift select. - Ctrl select. BUG=658980 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD Bookmarks] Add Select for Bookmarks. This CL enables single click to select items in the list. - Shift select. - Ctrl select. BUG=658980 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2639453002 Cr-Commit-Position: refs/heads/master@{#448209} Committed: https://chromium.googlesource.com/chromium/src/+/feacdc993b7e6657fb5daa957de6... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:400001) as https://chromium.googlesource.com/chromium/src/+/feacdc993b7e6657fb5daa957de6... |