Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2772003002:
[MD Bookmarks] Make drag and drop update data model. (Closed)
Description[MD Bookmarks] Make drag and drop update data model.
This CL makes the bookmark manager drag and drop actually send a message
to Chrome to update the bookmarks model.
BUG=692843
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2772003002
Cr-Commit-Position: refs/heads/master@{#461373}
Committed: https://chromium.googlesource.com/chromium/src/+/efa6e8b4bbc62a8f983a1be01415ae911338d293
Patch Set 1 : #
Total comments: 8
Patch Set 2 : rebase #Patch Set 3 : address_comments #Patch Set 4 : address_comments #
Total comments: 2
Patch Set 5 : rebase, nit #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 26 (17 generated)
Description was changed from ========== [MD Bookmarks] Make drag and drop update data model. This CL makes the bookmark manager drag and drop actually send a message to Chrome to update the bookmarks model. BUG=692843 ========== to ========== [MD Bookmarks] Make drag and drop update data model. This CL makes the bookmark manager drag and drop actually send a message to Chrome to update the bookmarks model. BUG=692843 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by calamity@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
calamity@chromium.org changed reviewers: + tsergeant@chromium.org
Cool, just a couple of small suggestions. https://codereview.chromium.org/2772003002/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/dnd_manager.js (right): https://codereview.chromium.org/2772003002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/dnd_manager.js:302: * @return {?{parentId: string, index: number}} Nit: This shouldn't be nullable, there's no early return here. https://codereview.chromium.org/2772003002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/dnd_manager.js:314: var listItems = bookmarks.util.getDisplayedList(state); I think that since you can't drop onto search results, it is sufficient to do: index = state.nodes[parentId].children.indexOf(node.id) This does feel kind of subtle, though, so I'm happy to stick with the current more explicit version if you prefer. https://codereview.chromium.org/2772003002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/md_bookmarks/dnd_manager_test.js (right): https://codereview.chromium.org/2772003002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/dnd_manager_test.js:344: // Drops onto the list. You could use a helper function here to shorten this, like function assertDropInfo(parentId, index, element, position){ assertDeepEquals(...) } Seems like it might be helpful if we ever need to refactor this.
Patchset #2 (id:60001) has been deleted
https://codereview.chromium.org/2772003002/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/dnd_manager.js (right): https://codereview.chromium.org/2772003002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/dnd_manager.js:302: * @return {?{parentId: string, index: number}} On 2017/03/29 00:32:48, tsergeant wrote: > Nit: This shouldn't be nullable, there's no early return here. Done. https://codereview.chromium.org/2772003002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/dnd_manager.js:314: var listItems = bookmarks.util.getDisplayedList(state); On 2017/03/29 00:32:48, tsergeant wrote: > I think that since you can't drop onto search results, it is sufficient to do: > > index = state.nodes[parentId].children.indexOf(node.id) > > This does feel kind of subtle, though, so I'm happy to stick with the current > more explicit version if you prefer. Changed, added a comment. https://codereview.chromium.org/2772003002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/md_bookmarks/dnd_manager_test.js (right): https://codereview.chromium.org/2772003002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/dnd_manager_test.js:344: // Drops onto the list. On 2017/03/29 00:32:48, tsergeant wrote: > You could use a helper function here to shorten this, like > > function assertDropInfo(parentId, index, element, position){ > assertDeepEquals(...) > } > > Seems like it might be helpful if we ever need to refactor this. Done.
https://codereview.chromium.org/2772003002/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/dnd_manager.js (right): https://codereview.chromium.org/2772003002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/dnd_manager.js:314: var listItems = bookmarks.util.getDisplayedList(state); On 2017/03/29 03:35:37, calamity wrote: > On 2017/03/29 00:32:48, tsergeant wrote: > > I think that since you can't drop onto search results, it is sufficient to do: > > > > index = state.nodes[parentId].children.indexOf(node.id) > > > > This does feel kind of subtle, though, so I'm happy to stick with the current > > more explicit version if you prefer. > > Changed, added a comment. I actually meant that you can replace the whole if/else block with that line: you can see that there's effectively no difference between the two branches in the latest patchset.
https://codereview.chromium.org/2772003002/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/dnd_manager.js (right): https://codereview.chromium.org/2772003002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/dnd_manager.js:314: var listItems = bookmarks.util.getDisplayedList(state); On 2017/03/29 04:28:01, tsergeant wrote: > On 2017/03/29 03:35:37, calamity wrote: > > On 2017/03/29 00:32:48, tsergeant wrote: > > > I think that since you can't drop onto search results, it is sufficient to > do: > > > > > > index = state.nodes[parentId].children.indexOf(node.id) > > > > > > This does feel kind of subtle, though, so I'm happy to stick with the > current > > > more explicit version if you prefer. > > > > Changed, added a comment. > > I actually meant that you can replace the whole if/else block with that line: > you can see that there's effectively no difference between the two branches in > the latest patchset. Ah yeah, I made this a bit more clear.
lgtm https://codereview.chromium.org/2772003002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/dnd_manager.js (right): https://codereview.chromium.org/2772003002/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/dnd_manager.js:315: parentId = node.parentId || ''; Nit: Maybe assert(node.parentId) here, instead of defaulting to the empty string. The end result is the same (since the empty string will just error on the line below), but the behavior is slightly nicer.
The CQ bit was checked by calamity@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2772003002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/dnd_manager.js (right): https://codereview.chromium.org/2772003002/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/dnd_manager.js:315: parentId = node.parentId || ''; On 2017/03/30 04:02:15, tsergeant wrote: > Nit: Maybe assert(node.parentId) here, instead of defaulting to the empty > string. > > The end result is the same (since the empty string will just error on the line > below), but the behavior is slightly nicer. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by calamity@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsergeant@chromium.org Link to the patchset: https://codereview.chromium.org/2772003002/#ps140001 (title: "rebase, nit")
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": 140001, "attempt_start_ts": 1491194394262380,
"parent_rev": "65f39693c549f42146b26cd35b13fa6a94fa0744", "commit_rev":
"efa6e8b4bbc62a8f983a1be01415ae911338d293"}
Message was sent while issue was closed.
Description was changed from ========== [MD Bookmarks] Make drag and drop update data model. This CL makes the bookmark manager drag and drop actually send a message to Chrome to update the bookmarks model. BUG=692843 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD Bookmarks] Make drag and drop update data model. This CL makes the bookmark manager drag and drop actually send a message to Chrome to update the bookmarks model. BUG=692843 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2772003002 Cr-Commit-Position: refs/heads/master@{#461373} Committed: https://chromium.googlesource.com/chromium/src/+/efa6e8b4bbc62a8f983a1be01415... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001) as https://chromium.googlesource.com/chromium/src/+/efa6e8b4bbc62a8f983a1be01415... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
