[MD Bookmarks] Add a drag and drop indicator to bookmarks.

[MD Bookmarks] Add a drag and drop indicator to bookmarks.

This CL adds an indicator for dragging and dropping bookmarks. This
indicator will highlight folders that can be dragged onto and add a line
above/below items for dragging between items. This affects both the list
and the sidebar, and bookmark elements in both are now draggable.

BUG=692843
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2746363013
Cr-Commit-Position: refs/heads/master@{#460626}
Committed: https://chromium.googlesource.com/chromium/src/+/cc543e614e2362ae0e6881fbf5601c415e07c672

[calamity, 2017-03-17 05:27:52]

Description was changed from

==========
[MD Bookmarks] Add a drag and drop indicator to bookmarks.

This CL adds an indicator for dragging and dropping bookmarks. This
indicator will highlight folders that can be dragged onto and add a line
above/below items for dragging between items. This affects both the list
and the sidebar, and bookmark elements in both are now draggable.

BUG=692843
==========

to

==========
[MD Bookmarks] Add a drag and drop indicator to bookmarks.

This CL adds an indicator for dragging and dropping bookmarks. This
indicator will highlight folders that can be dragged onto and add a line
above/below items for dragging between items. This affects both the list
and the sidebar, and bookmark elements in both are now draggable.

BUG=692843
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[calamity, 2017-03-17 05:36:50]

calamity@chromium.org changed reviewers:
+ tsergeant@chromium.org

[calamity, 2017-03-17 05:36:51]

Okay... This didn't end up being much smaller.

The prototype is at https://codereview.chromium.org/2758703002/ .

This CL breaks out the drop functionality, but that's actually pretty small. If
you can think of a nicer way to split it, let me know? I think in general this
is a very end-to-end thing so it's gonna be a bit big.

[tsergeant, 2017-03-20 03:01:50]

First pass. At this stage I mostly understand what's going on, but I haven't
necessarily looked in-depth at everything.

https://codereview.chromium.org/2746363013/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/dnd_manager.html (right):

https://codereview.chromium.org/2746363013/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/dnd_manager.html:4: <dom-module
id="bookmarks-dnd-manager">
I believe you don't even need the dom-module if there's no template -- just the
JS call to Polymer() is enough

https://codereview.chromium.org/2746363013/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/dnd_manager.js (right):

https://codereview.chromium.org/2746363013/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/dnd_manager.js:5: /** @typedef {{element:
BookmarkElement, position: DropPosition}} */
You could put this, BookmarkElement and DragData into types.js to clean up this
file a bit and get them out of the build.

https://codereview.chromium.org/2746363013/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/dnd_manager.js:85: * @constructor
Maybe add a comment here explaining the purpose of this class? I was quite
confused about it until I realised that dragData was only set by the
bookmarkManagerPrivate listener.

https://codereview.chromium.org/2746363013/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/dnd_manager.js:230: Polymer({
I don't really think this needs to be a Polymer element. It doesn't do anything
Polymer-y: the only thing it makes use of is the lifecycle callbacks.

I'll leave it up to you, but this might be nicer as a regular JS class.

https://codereview.chromium.org/2746363013/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/dnd_manager.js:313: var draggedNodes =
This logic doesn't seem right:

1) If you drag a folder node while items are selected, you'll drag the selected
items instead of the folder

2) If you select some items then drag a different list item, you'll drag the
selection rather than the drag target.

https://codereview.chromium.org/2746363013/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/dnd_manager.js:325: 'copyMoveLink' :
I think 'copyMoveLink' is...bogus?

It's not in the MDN docs, and doesn't appear anywhere else in the codebase:

https://developer.mozilla.org/en-US/docs/Web/API/DataTransfer/effectAllowed
https://cs.chromium.org/search/?q=copyMoveLink&ssfr=1&type=cs

https://codereview.chromium.org/2746363013/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/dnd_manager.js:447:
calculateDropAboveBelow: function(overElement) {
I've noticed a couple of (minor) behavioral differences. Not sure why they are
different and whether they're actually bugs, but you should check anyway.

If I have a tree like

Bookmarks Bar
-> a
   -> b
      -> c

And I grab 'c', I am able to drop c:

- Below 'b'
- Onto 'a'

Neither of which are possible in the old bookmark manager.

https://codereview.chromium.org/2746363013/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/dnd_manager.js:452: if
(getBookmarkNode(overElement).parentId == ROOT_NODE_ID)
Maybe extract this into a util function like isRootNode(), which can be used
here and in folder_node

(it would probably also be handy in reducers)

https://codereview.chromium.org/2746363013/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/sidebar.html (right):

https://codereview.chromium.org/2746363013/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/sidebar.html:25: <bookmarks-folder-node
item-id="[[item]]" depth="0" draggable="true">
Is there any particular reason for this to be draggable? All it seems to do is
apply user-select: none, which we can just do in CSS.

https://codereview.chromium.org/2746363013/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/util.js (right):

https://codereview.chromium.org/2746363013/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/util.js:15: if (state.selectedFolder)
Call isShowingSearch from here, so we don't duplicate this logic

https://codereview.chromium.org/2746363013/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/util.js:70: function
isShowingSearch(state) {
Annotate this function

[calamity, 2017-03-22 04:30:02]

Patchset #3 (id:40001) has been deleted

[calamity, 2017-03-22 05:09:45]

Patchset #3 (id:60001) has been deleted

[calamity, 2017-03-22 05:29:53]

https://codereview.chromium.org/2746363013/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/dnd_manager.html (right):

https://codereview.chromium.org/2746363013/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/dnd_manager.html:4: <dom-module
id="bookmarks-dnd-manager">
On 2017/03/20 03:01:49, tsergeant wrote:
> I believe you don't even need the dom-module if there's no template -- just
the
> JS call to Polymer() is enough

Acknowledged.

https://codereview.chromium.org/2746363013/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/dnd_manager.js (right):

https://codereview.chromium.org/2746363013/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/dnd_manager.js:5: /** @typedef {{element:
BookmarkElement, position: DropPosition}} */
On 2017/03/20 03:01:49, tsergeant wrote:
> You could put this, BookmarkElement and DragData into types.js to clean up
this
> file a bit and get them out of the build.

Did some. Couldn't do this one since it depends on DropPosition which is a
forrealz enum and can't be moved just into types.js.

https://codereview.chromium.org/2746363013/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/dnd_manager.js:85: * @constructor
On 2017/03/20 03:01:50, tsergeant wrote:
> Maybe add a comment here explaining the purpose of this class? I was quite
> confused about it until I realised that dragData was only set by the
> bookmarkManagerPrivate listener.

Done.

https://codereview.chromium.org/2746363013/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/dnd_manager.js:230: Polymer({
On 2017/03/20 03:01:50, tsergeant wrote:
> I don't really think this needs to be a Polymer element. It doesn't do
anything
> Polymer-y: the only thing it makes use of is the lifecycle callbacks.
> 
> I'll leave it up to you, but this might be nicer as a regular JS class.

Done.

https://codereview.chromium.org/2746363013/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/dnd_manager.js:313: var draggedNodes =
On 2017/03/20 03:01:50, tsergeant wrote:
> This logic doesn't seem right:
> 
> 1) If you drag a folder node while items are selected, you'll drag the
selected
> items instead of the folder
> 
> 2) If you select some items then drag a different list item, you'll drag the
> selection rather than the drag target.

Good point. Fixed.

https://codereview.chromium.org/2746363013/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/dnd_manager.js:325: 'copyMoveLink' :
On 2017/03/20 03:01:50, tsergeant wrote:
> I think 'copyMoveLink' is...bogus?
> 
> It's not in the MDN docs, and doesn't appear anywhere else in the codebase:
> 
> https://developer.mozilla.org/en-US/docs/Web/API/DataTransfer/effectAllowed
> https://cs.chromium.org/search/?q=copyMoveLink&ssfr=1&type=cs

Lol.

https://codereview.chromium.org/2746363013/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/dnd_manager.js:447:
calculateDropAboveBelow: function(overElement) {
On 2017/03/20 03:01:49, tsergeant wrote:
> I've noticed a couple of (minor) behavioral differences. Not sure why they are
> different and whether they're actually bugs, but you should check anyway.
> 
> If I have a tree like
> 
> Bookmarks Bar
> -> a
>    -> b
>       -> c
> 
> And I grab 'c', I am able to drop c:
> 
> - Below 'b'
> - Onto 'a'
> 
> Neither of which are possible in the old bookmark manager.

Case 1 is a bug here, Case 2 is a bug with the old bookmark manager...

It uses the bounding rect of the entire node, including all it's children so the
ratio is bogus. Don't know if we want to explicitly cause the same behavior.

https://codereview.chromium.org/2746363013/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/dnd_manager.js:452: if
(getBookmarkNode(overElement).parentId == ROOT_NODE_ID)
On 2017/03/20 03:01:49, tsergeant wrote:
> Maybe extract this into a util function like isRootNode(), which can be used
> here and in folder_node
> 
> (it would probably also be handy in reducers)

Moved the constant to be part of bookmarks.util. Having a method seems weird
since the id is what we should check.

https://codereview.chromium.org/2746363013/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/sidebar.html (right):

https://codereview.chromium.org/2746363013/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/sidebar.html:25: <bookmarks-folder-node
item-id="[[item]]" depth="0" draggable="true">
On 2017/03/20 03:01:50, tsergeant wrote:
> Is there any particular reason for this to be draggable? All it seems to do is
> apply user-select: none, which we can just do in CSS.

Oops, forgot to remove.

https://codereview.chromium.org/2746363013/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/util.js (right):

https://codereview.chromium.org/2746363013/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/util.js:15: if (state.selectedFolder)
On 2017/03/20 03:01:50, tsergeant wrote:
> Call isShowingSearch from here, so we don't duplicate this logic

Done.

https://codereview.chromium.org/2746363013/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/util.js:70: function
isShowingSearch(state) {
On 2017/03/20 03:01:50, tsergeant wrote:
> Annotate this function

Done.

[tsergeant, 2017-03-23 00:28:23]

You'll need to do a rebase -- there are minor conflicts in a few places due to
files being added/removed.

Also, you should add 'user-select: none' to the sidebar css. It's currently
possible to highlight the root folders, but nothing else.

https://codereview.chromium.org/2746363013/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/app.js (right):

https://codereview.chromium.org/2746363013/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/app.js:27: dnd_manager_: null,
camelCase for member names

https://codereview.chromium.org/2746363013/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/dnd_manager.js (right):

https://codereview.chromium.org/2746363013/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/dnd_manager.js:161: this.synchronous_ =
false;
Hmmm, an alternative way to do this would be

this.setTimeout_ = window.setTimeout;

disableTimeoutForTesting: function() {
  this.setTimeout_ = function(fn, timeout) {
    fn();
  };
}

and then always call this.setTimeout_() in finish().

I kinda like this more since it gets rid of the branch in finish() without
affecting the rest of the logic too much. WDYT?

https://codereview.chromium.org/2746363013/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/dnd_manager.js:386: *       position - A
|DropPosition| relative to the |element|.
This and the methods below are all missing @private

https://codereview.chromium.org/2746363013/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/shared_style.html (right):

https://codereview.chromium.org/2746363013/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/shared_style.html:39: background-color:
rgb(66, 133, 244);
This is #4285F4, which is var(--google-blue-500) and also
var(--folder-active-color).

Pull these two colors out into a single shared variable like
--interactive-color?

https://codereview.chromium.org/2746363013/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/md_bookmarks/dnd_manager_test.js (right):

https://codereview.chromium.org/2746363013/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/dnd_manager_test.js:10: var dnd_manager;
underscoresArentReallyTheStyle

https://codereview.chromium.org/2746363013/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/dnd_manager_test.js:64: dragStyle =>
assertEquals(
This is pretty hard to read. Maybe add {} to the function so that it splits up
onto its own block.

https://codereview.chromium.org/2746363013/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/dnd_manager_test.js:71: function
createDragData(ids, sameProfile) {
You don't test sameProfile anywhere: seems like it would be good to test, since
it's entwined in all the logic.

https://codereview.chromium.org/2746363013/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/dnd_manager_test.js:187:
assertDragStyle(dragTarget, DRAG_STYLE.ON);
Should assert that it's not possible to drag above or below, since there's only
an item.

https://codereview.chromium.org/2746363013/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/dnd_manager_test.js:266: });
Maybe should have a test for dragging from the sidebar to the list?

Can drag a sidebar folder to the list, but not onto itself or any of its
descendants.

[calamity, 2017-03-23 05:45:49]

Patchset #4 (id:100001) has been deleted

[calamity, 2017-03-23 06:10:18]

Patchset #5 (id:140001) has been deleted

[calamity, 2017-03-23 06:10:24]

The CQ bit was checked by calamity@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 06:10:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[calamity, 2017-03-23 06:12:34]

The tests also picked up a few bugs which got fixed.

Sidebar user-select CSS added.

https://codereview.chromium.org/2746363013/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/app.js (right):

https://codereview.chromium.org/2746363013/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/app.js:27: dnd_manager_: null,
On 2017/03/23 00:28:23, tsergeant wrote:
> camelCase for member names

Done.

https://codereview.chromium.org/2746363013/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/dnd_manager.js (right):

https://codereview.chromium.org/2746363013/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/dnd_manager.js:161: this.synchronous_ =
false;
On 2017/03/23 00:28:23, tsergeant wrote:
> Hmmm, an alternative way to do this would be
> 
> this.setTimeout_ = window.setTimeout;
> 
> disableTimeoutForTesting: function() {
>   this.setTimeout_ = function(fn, timeout) {
>     fn();
>   };
> }
> 
> and then always call this.setTimeout_() in finish().
> 
> I kinda like this more since it gets rid of the branch in finish() without
> affecting the rest of the logic too much. WDYT?

Done.

https://codereview.chromium.org/2746363013/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/dnd_manager.js:386: *       position - A
|DropPosition| relative to the |element|.
On 2017/03/23 00:28:23, tsergeant wrote:
> This and the methods below are all missing @private

Done.

https://codereview.chromium.org/2746363013/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/shared_style.html (right):

https://codereview.chromium.org/2746363013/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/shared_style.html:39: background-color:
rgb(66, 133, 244);
On 2017/03/23 00:28:23, tsergeant wrote:
> This is #4285F4, which is var(--google-blue-500) and also
> var(--folder-active-color).
> 
> Pull these two colors out into a single shared variable like
> --interactive-color?

Done.

https://codereview.chromium.org/2746363013/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/md_bookmarks/dnd_manager_test.js (right):

https://codereview.chromium.org/2746363013/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/dnd_manager_test.js:10: var dnd_manager;
On 2017/03/23 00:28:23, tsergeant wrote:
> underscoresArentReallyTheStyle

but_im_a_hacker_

https://codereview.chromium.org/2746363013/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/dnd_manager_test.js:64: dragStyle =>
assertEquals(
On 2017/03/23 00:28:23, tsergeant wrote:
> This is pretty hard to read. Maybe add {} to the function so that it splits up
> onto its own block.

Done.

https://codereview.chromium.org/2746363013/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/dnd_manager_test.js:71: function
createDragData(ids, sameProfile) {
On 2017/03/23 00:28:23, tsergeant wrote:
> You don't test sameProfile anywhere: seems like it would be good to test,
since
> it's entwined in all the logic.

Done.

https://codereview.chromium.org/2746363013/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/dnd_manager_test.js:187:
assertDragStyle(dragTarget, DRAG_STYLE.ON);
On 2017/03/23 00:28:23, tsergeant wrote:
> Should assert that it's not possible to drag above or below, since there's
only
> an item.

Done.

[commit-bot: I haz the power, 2017-03-23 06:13:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-23 06:13:03]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[calamity, 2017-03-23 06:15:44]

Patchset #5 (id:160001) has been deleted

[calamity, 2017-03-23 06:16:01]

The CQ bit was checked by calamity@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 06:16:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 07:02:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-23 07:02:05]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[tsergeant, 2017-03-27 06:49:23]

review haiku:

big, complex CL
eyes begin to glaze over
here's a round of nits

https://codereview.chromium.org/2746363013/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/md_bookmarks/dnd_manager.js (right):

https://codereview.chromium.org/2746363013/diff/180001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/dnd_manager.js:158: * @type
{function((Function|null|string), number)}
Nit: @private

https://codereview.chromium.org/2746363013/diff/180001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/dnd_manager.js:182: * Clears the drop
indicator style from the last element was the drop target
nit: this sentence doesn't quite grammar

maybe just "Clears the drop indicator style from the last drop target"

https://codereview.chromium.org/2746363013/diff/180001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/dnd_manager.js:287: if
(this.dropDestination_) {
Nit: {}

https://codereview.chromium.org/2746363013/diff/180001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/dnd_manager.js:312: var draggedNodes =
Object.keys(state.selection.items);
Hrmm, at the moment we don't really guarantee from the store side that
everything in the `selection.items` map has the value `true`. It's possible
right now for a reducer to set `selection.items[id] = false` and everything else
will work.

I think it's fine to make the assumption that everything in the items map is
true, but we should probably document it somewhere. Maybe in types.js? That's
not a super visible place, but I'm not sure there's anywhere better.

https://codereview.chromium.org/2746363013/diff/180001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/dnd_manager.js:351: // the event will have
this set to 'none' already.
😕

https://codereview.chromium.org/2746363013/diff/180001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/dnd_manager.js:475: if (!previousElem ||
!dragInfo.isDraggingBookmark(previousElem.itemId)) {
Nit: {}

https://codereview.chromium.org/2746363013/diff/180001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/dnd_manager.js:486: // Cannot drop below
if the item below is already in the drag source.
Does this comment belong on the if statement on line 493?

https://codereview.chromium.org/2746363013/diff/180001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/dnd_manager.js:493: if (!nextElement ||
!dragInfo.isDraggingBookmark(nextElement.itemId)) {
{} here too

https://codereview.chromium.org/2746363013/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/md_bookmarks/shared_vars.html (right):

https://codereview.chromium.org/2746363013/diff/180001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/shared_vars.html:5: --interactive-color:
#4285f4;
Use var(--google-blue-500) directly

https://codereview.chromium.org/2746363013/diff/180001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/shared_vars.html:7:
--paper-input-container-focus-color: var(--google-blue-500);
And use var(--interactive-color) here

https://codereview.chromium.org/2746363013/diff/180001/chrome/test/data/webui...
File chrome/test/data/webui/md_bookmarks/dnd_manager_test.js (right):

https://codereview.chromium.org/2746363013/diff/180001/chrome/test/data/webui...
chrome/test/data/webui/md_bookmarks/dnd_manager_test.js:73: sameProfile:
sameProfile == undefined ? true : sameProfile,
nit: these should align

[calamity, 2017-03-28 04:15:03]

https://codereview.chromium.org/2746363013/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/md_bookmarks/dnd_manager.js (right):

https://codereview.chromium.org/2746363013/diff/180001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/dnd_manager.js:158: * @type
{function((Function|null|string), number)}
On 2017/03/27 06:49:24, tsergeant wrote:
> Nit: @private

Done. Also privated the rest of the things here.

https://codereview.chromium.org/2746363013/diff/180001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/dnd_manager.js:182: * Clears the drop
indicator style from the last element was the drop target
On 2017/03/27 06:49:24, tsergeant wrote:
> nit: this sentence doesn't quite grammar
> 
> maybe just "Clears the drop indicator style from the last drop target"

Uhhhhh. Done.

https://codereview.chromium.org/2746363013/diff/180001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/dnd_manager.js:287: if
(this.dropDestination_) {
On 2017/03/27 06:49:23, tsergeant wrote:
> Nit: {}

Done.

https://codereview.chromium.org/2746363013/diff/180001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/dnd_manager.js:312: var draggedNodes =
Object.keys(state.selection.items);
On 2017/03/27 06:49:24, tsergeant wrote:
> Hrmm, at the moment we don't really guarantee from the store side that
> everything in the `selection.items` map has the value `true`. It's possible
> right now for a reducer to set `selection.items[id] = false` and everything
else
> will work.
> 
> I think it's fine to make the assumption that everything in the items map is
> true, but we should probably document it somewhere. Maybe in types.js? That's
> not a super visible place, but I'm not sure there's anywhere better.

Done.

Remind me why we didn't use a Set again?

https://codereview.chromium.org/2746363013/diff/180001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/dnd_manager.js:351: // the event will have
this set to 'none' already.
On 2017/03/27 06:49:24, tsergeant wrote:
> 😕

Mate, you have no idea.

https://codereview.chromium.org/2746363013/diff/180001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/dnd_manager.js:475: if (!previousElem ||
!dragInfo.isDraggingBookmark(previousElem.itemId)) {
On 2017/03/27 06:49:24, tsergeant wrote:
> Nit: {}

Done.

https://codereview.chromium.org/2746363013/diff/180001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/dnd_manager.js:486: // Cannot drop below
if the item below is already in the drag source.
On 2017/03/27 06:49:23, tsergeant wrote:
> Does this comment belong on the if statement on line 493?

Ugh, I never know what to do with comments like these. I want to comment the
entire block below here, but I guess the conditional will do.

https://codereview.chromium.org/2746363013/diff/180001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/dnd_manager.js:493: if (!nextElement ||
!dragInfo.isDraggingBookmark(nextElement.itemId)) {
On 2017/03/27 06:49:23, tsergeant wrote:
> {} here too

Done.

https://codereview.chromium.org/2746363013/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/md_bookmarks/shared_vars.html (right):

https://codereview.chromium.org/2746363013/diff/180001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/shared_vars.html:5: --interactive-color:
#4285f4;
On 2017/03/27 06:49:24, tsergeant wrote:
> Use var(--google-blue-500) directly

Woops.

https://codereview.chromium.org/2746363013/diff/180001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/shared_vars.html:7:
--paper-input-container-focus-color: var(--google-blue-500);
On 2017/03/27 06:49:24, tsergeant wrote:
> And use var(--interactive-color) here

Done.

https://codereview.chromium.org/2746363013/diff/180001/chrome/test/data/webui...
File chrome/test/data/webui/md_bookmarks/dnd_manager_test.js (right):

https://codereview.chromium.org/2746363013/diff/180001/chrome/test/data/webui...
chrome/test/data/webui/md_bookmarks/dnd_manager_test.js:73: sameProfile:
sameProfile == undefined ? true : sameProfile,
On 2017/03/27 06:49:24, tsergeant wrote:
> nit: these should align

Obviously, it's because I forgot the semicolon on the return. (Like seriously,
clang-format did this.)

[tsergeant, 2017-03-28 23:52:22]

lgtm with one last nit

https://codereview.chromium.org/2746363013/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/md_bookmarks/dnd_manager.js (right):

https://codereview.chromium.org/2746363013/diff/180001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/dnd_manager.js:312: var draggedNodes =
Object.keys(state.selection.items);
On 2017/03/28 04:15:03, calamity wrote:
> On 2017/03/27 06:49:24, tsergeant wrote:
> > Hrmm, at the moment we don't really guarantee from the store side that
> > everything in the `selection.items` map has the value `true`. It's possible
> > right now for a reducer to set `selection.items[id] = false` and everything
> else
> > will work.
> > 
> > I think it's fine to make the assumption that everything in the items map is
> > true, but we should probably document it somewhere. Maybe in types.js?
That's
> > not a super visible place, but I'm not sure there's anywhere better.
> 
> Done.
> 
> Remind me why we didn't use a Set again?

Yeah, we could probably give it a go with a Set.

https://codereview.chromium.org/2746363013/diff/180001/chrome/test/data/webui...
File chrome/test/data/webui/md_bookmarks/dnd_manager_test.js (right):

https://codereview.chromium.org/2746363013/diff/180001/chrome/test/data/webui...
chrome/test/data/webui/md_bookmarks/dnd_manager_test.js:73: sameProfile:
sameProfile == undefined ? true : sameProfile,
On 2017/03/28 04:15:03, calamity wrote:
> On 2017/03/27 06:49:24, tsergeant wrote:
> > nit: these should align
> 
> Obviously, it's because I forgot the semicolon on the return. (Like seriously,
> clang-format did this.)

That's... interesting behavior.

https://codereview.chromium.org/2746363013/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/md_bookmarks/sidebar.html (right):

https://codereview.chromium.org/2746363013/diff/200001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/sidebar.html:15: user-select: none
Nit: Semicolon here

[calamity, 2017-03-29 00:50:44]

The CQ bit was checked by calamity@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 00:51:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[calamity, 2017-03-29 00:52:20]

https://codereview.chromium.org/2746363013/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/md_bookmarks/sidebar.html (right):

https://codereview.chromium.org/2746363013/diff/200001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/sidebar.html:15: user-select: none
On 2017/03/28 23:52:22, tsergeant wrote:
> Nit: Semicolon here

Done.

[commit-bot: I haz the power, 2017-03-29 02:40:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-29 02:40:10]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[calamity, 2017-03-29 06:45:05]

The CQ bit was checked by calamity@chromium.org

[calamity, 2017-03-29 06:45:06]

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/2746363013/#ps220001
(title: "fix nit")

[commit-bot: I haz the power, 2017-03-29 06:45:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 07:48:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-29 07:48:14]

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_...)

[calamity, 2017-03-30 00:51:30]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2017-03-30 00:53:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 02:15:04]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1490835090813730,
"parent_rev": "e4a64eab960f6b5a25df0270b03e86311206d802", "commit_rev":
"cc543e614e2362ae0e6881fbf5601c415e07c672"}

[commit-bot: I haz the power, 2017-03-30 02:15:52]

Description was changed from

==========
[MD Bookmarks] Add a drag and drop indicator to bookmarks.

This CL adds an indicator for dragging and dropping bookmarks. This
indicator will highlight folders that can be dragged onto and add a line
above/below items for dragging between items. This affects both the list
and the sidebar, and bookmark elements in both are now draggable.

BUG=692843
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD Bookmarks] Add a drag and drop indicator to bookmarks.

This CL adds an indicator for dragging and dropping bookmarks. This
indicator will highlight folders that can be dragged onto and add a line
above/below items for dragging between items. This affects both the list
and the sidebar, and bookmark elements in both are now draggable.

BUG=692843
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2746363013
Cr-Commit-Position: refs/heads/master@{#460626}
Committed:
https://chromium.googlesource.com/chromium/src/+/cc543e614e2362ae0e6881fbf560...
==========

[commit-bot: I haz the power, 2017-03-30 02:15:54]

Committed patchset #7 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/cc543e614e2362ae0e6881fbf560...