Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2613683002:
[MD Bookmarks] Add Delete and Copy URL for Material Bookmarks. (Closed)
Description[MD Bookmarks] Add Delete and Copy URL for Material Bookmarks.
This CL adds delete bookmarks and folders function and copy URL function.
BUG=658980
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2613683002
Cr-Commit-Position: refs/heads/master@{#441864}
Committed: https://chromium.googlesource.com/chromium/src/+/98f40af6044a40ba8a10899efb11d75ab122dffd
Patch Set 1 #
Total comments: 19
Patch Set 2 : style fix #
Total comments: 4
Patch Set 3 : closure and style fix #Patch Set 4 : add an early return #Patch Set 5 : fix a merge problem #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 22 (11 generated)
Description was changed from ========== [MD Bookmarks] Add Delete and Copy URL for Material Bookmarks This CL adds delete bookmarks and folders function and copy URL function. BUG=651980 ========== to ========== [MD Bookmarks] Add Delete and Copy URL for Material Bookmarks This CL adds delete bookmarks and folders function and copy URL function. BUG=651980 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
jiaxi@google.com changed reviewers: + angelayang@google.com, calamity@chromium.org, tsergeant@chromium.org
Description was changed from ========== [MD Bookmarks] Add Delete and Copy URL for Material Bookmarks This CL adds delete bookmarks and folders function and copy URL function. BUG=651980 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD Bookmarks] Add Delete and Copy URL for Material Bookmarks. This CL adds delete bookmarks and folders function and copy URL function. BUG=651980 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2613683002/diff/50001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/item.js (right): https://codereview.chromium.org/2613683002/diff/50001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/item.js:34: onItemChanged_: function() { this.isFolder_ = !(this.item.url); }, This format is wrong. Try running git cl format, it should make everything how it should be. https://codereview.chromium.org/2613683002/diff/50001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/list.js (right): https://codereview.chromium.org/2613683002/diff/50001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/list.js:46: this.closeDropdownMenu_(); For consistency, move this to the end of the method. https://codereview.chromium.org/2613683002/diff/50001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2613683002/diff/50001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:44: Move this back to where it was. https://codereview.chromium.org/2613683002/diff/50001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:128: this.fire('selected-folder-changed', removeInfo.parentId); nit: newline https://codereview.chromium.org/2613683002/diff/50001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:164: if (bookmarkNode.children) { Invert and early return. https://codereview.chromium.org/2613683002/diff/50001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:172: /** This method deserves a comment. https://codereview.chromium.org/2613683002/diff/50001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:176: removeChildrenFromMap_: function(id) { Since this is all recursive children, I would prefer removeDescendantsFromMap_. https://codereview.chromium.org/2613683002/diff/50001/chrome/test/data/webui/... File chrome/test/data/webui/md_bookmarks/store_test.js (right): https://codereview.chromium.org/2613683002/diff/50001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/store_test.js:126: // paths have been updated. Capitalization. https://codereview.chromium.org/2613683002/diff/50001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/store_test.js:151: // paths have been updated. Capitalization.
https://codereview.chromium.org/2613683002/diff/50001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/item.js (right): https://codereview.chromium.org/2613683002/diff/50001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/item.js:34: onItemChanged_: function() { this.isFolder_ = !(this.item.url); }, On 2017/01/04 06:57:44, calamity wrote: > This format is wrong. Try running git cl format, it should make everything how > it should be. Done. https://codereview.chromium.org/2613683002/diff/50001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/list.js (right): https://codereview.chromium.org/2613683002/diff/50001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/list.js:46: this.closeDropdownMenu_(); On 2017/01/04 06:57:44, calamity wrote: > For consistency, move this to the end of the method. Done. https://codereview.chromium.org/2613683002/diff/50001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2613683002/diff/50001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:44: On 2017/01/04 06:57:44, calamity wrote: > Move this back to where it was. Done. https://codereview.chromium.org/2613683002/diff/50001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:128: this.fire('selected-folder-changed', removeInfo.parentId); On 2017/01/04 06:57:44, calamity wrote: > nit: newline Done. https://codereview.chromium.org/2613683002/diff/50001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:164: if (bookmarkNode.children) { On 2017/01/04 06:57:44, calamity wrote: > Invert and early return. I think the early return is in this(https://codereview.appspot.com/315240043/) patch, do you think I should do it in the current patch as well? https://codereview.chromium.org/2613683002/diff/50001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:172: /** On 2017/01/04 06:57:44, calamity wrote: > This method deserves a comment. Done. https://codereview.chromium.org/2613683002/diff/50001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:176: removeChildrenFromMap_: function(id) { On 2017/01/04 06:57:44, calamity wrote: > Since this is all recursive children, I would prefer removeDescendantsFromMap_. Done. https://codereview.chromium.org/2613683002/diff/50001/chrome/test/data/webui/... File chrome/test/data/webui/md_bookmarks/store_test.js (right): https://codereview.chromium.org/2613683002/diff/50001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/store_test.js:126: // paths have been updated. On 2017/01/04 06:57:44, calamity wrote: > Capitalization. Done. https://codereview.chromium.org/2613683002/diff/50001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/store_test.js:151: // paths have been updated. On 2017/01/04 06:57:44, calamity wrote: > Capitalization. Done.
https://codereview.chromium.org/2613683002/diff/70001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/list.js (right): https://codereview.chromium.org/2613683002/diff/70001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/list.js:33: onEditTap_: function() { this.closeDropdownMenu_(); }, This still isn't fixed. Let me know if clang-format isn't working here. https://codereview.chromium.org/2613683002/diff/70001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2613683002/diff/70001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:133: this.generatePaths_(parentNode, Number(removeInfo.index)); Is this Number() call only here to satisfy Closure? If so, it's probably better to remove it and define the removeInfo type exactly in the method definition. Something like this should work: @param {!{index: number, parentId: string, node: BookmarkTreeNode}} removeInfo
https://codereview.chromium.org/2613683002/diff/70001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/list.js (right): https://codereview.chromium.org/2613683002/diff/70001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/list.js:33: onEditTap_: function() { this.closeDropdownMenu_(); }, On 2017/01/04 23:30:35, tsergeant wrote: > This still isn't fixed. Let me know if clang-format isn't working here. It works now :) https://codereview.chromium.org/2613683002/diff/70001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2613683002/diff/70001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:133: this.generatePaths_(parentNode, Number(removeInfo.index)); On 2017/01/04 23:30:36, tsergeant wrote: > Is this Number() call only here to satisfy Closure? > > If so, it's probably better to remove it and define the removeInfo type exactly > in the method definition. Something like this should work: > > @param {!{index: number, parentId: string, node: BookmarkTreeNode}} removeInfo Done.
lgtm
https://codereview.chromium.org/2613683002/diff/50001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2613683002/diff/50001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:164: if (bookmarkNode.children) { On 2017/01/04 22:44:00, jiaxi wrote: > On 2017/01/04 06:57:44, calamity wrote: > > Invert and early return. > > I think the early return is in this(https://codereview.appspot.com/315240043/) > patch, do you think I should do it in the current patch as well? You might as well, the next patch will just pull it cleanly.
On 2017/01/05 07:07:08, calamity wrote: > https://codereview.chromium.org/2613683002/diff/50001/chrome/browser/resource... > File chrome/browser/resources/md_bookmarks/store.js (right): > > https://codereview.chromium.org/2613683002/diff/50001/chrome/browser/resource... > chrome/browser/resources/md_bookmarks/store.js:164: if (bookmarkNode.children) { > On 2017/01/04 22:44:00, jiaxi wrote: > > On 2017/01/04 06:57:44, calamity wrote: > > > Invert and early return. > > > > I think the early return is in this(https://codereview.appspot.com/315240043/) > > patch, do you think I should do it in the current patch as well? > > You might as well, the next patch will just pull it cleanly. It's done now :)
lgtm.
The CQ bit was checked by jiaxi@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from tsergeant@chromium.org, calamity@chromium.org Link to the patchset: https://codereview.chromium.org/2613683002/#ps130001 (title: "fix a merge problem")
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": 130001, "attempt_start_ts": 1483670409782640,
"parent_rev": "fc29407956cd9349e12be316b7f0007c9b2471ab", "commit_rev":
"98f40af6044a40ba8a10899efb11d75ab122dffd"}
Message was sent while issue was closed.
Description was changed from ========== [MD Bookmarks] Add Delete and Copy URL for Material Bookmarks. This CL adds delete bookmarks and folders function and copy URL function. BUG=651980 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD Bookmarks] Add Delete and Copy URL for Material Bookmarks. This CL adds delete bookmarks and folders function and copy URL function. BUG=651980 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2613683002 Cr-Commit-Position: refs/heads/master@{#441864} Committed: https://chromium.googlesource.com/chromium/src/+/98f40af6044a40ba8a10899efb11... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:130001) as https://chromium.googlesource.com/chromium/src/+/98f40af6044a40ba8a10899efb11...
Message was sent while issue was closed.
Description was changed from ========== [MD Bookmarks] Add Delete and Copy URL for Material Bookmarks. This CL adds delete bookmarks and folders function and copy URL function. BUG=651980 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2613683002 Cr-Commit-Position: refs/heads/master@{#441864} Committed: https://chromium.googlesource.com/chromium/src/+/98f40af6044a40ba8a10899efb11... ========== to ========== [MD Bookmarks] Add Delete and Copy URL for Material Bookmarks. This CL adds delete bookmarks and folders function and copy URL function. BUG=658980 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2613683002 Cr-Commit-Position: refs/heads/master@{#441864} Committed: https://chromium.googlesource.com/chromium/src/+/98f40af6044a40ba8a10899efb11... ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
