[MD Bookmarks] Implement adding folders and bookmarks from toolbar menu.

[MD Bookmarks] Implement adding folders and bookmarks from toolbar menu.

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

[angelayang, 2017-02-01 07:53:29]

Description was changed from

==========
[MD Bookmarks] Implement adding folders and bookmarks from toolbar menu.

BUG=658980
==========

to

==========
[MD Bookmarks] Implement adding folders and bookmarks from toolbar menu.

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

[angelayang, 2017-02-08 07:25:05]

Patchset #1 (id:1) has been deleted

[angelayang, 2017-02-08 07:33:14]

Patchset #2 (id:40001) has been deleted

[angelayang, 2017-02-08 07:34:03]

angelayang@google.com changed reviewers:
+ calamity@chromium.org, jiaxi@google.com, tsergeant@chromium.org

[angelayang, 2017-02-08 07:34:03]

CL that implements adding folder and bookmark.

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

https://codereview.chromium.org/2670473002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store.js:337: var parent =
this.idToNodeMap_[bookmarkNode.parentId || null];
Closure complains that bookmarkNode.parentId can be undefined whilst the
idToNodeMap expects ?string. I'm unsure of what should be done for this.

[jiaxi, 2017-02-09 01:13:12]

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

https://codereview.chromium.org/2670473002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store.js:337: var parent =
this.idToNodeMap_[bookmarkNode.parentId || null];
On 2017/02/08 07:34:03, angelayang wrote:
> Closure complains that bookmarkNode.parentId can be undefined whilst the
> idToNodeMap expects ?string. I'm unsure of what should be done for this.

I used /** @type {?string} */ in onSelectedFolderChanged. If you come across any
better options can you change the code there as well?

https://codereview.chromium.org/2670473002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store.js:338: this.push(parent.path +
'.children', bookmarkNode);
According to my understanding, this adds the new BookmarkTreeNode at the end of
the parent node. If you create an item using the bar the new node will actually
gets 'inserted' into the children.
We might want to check if the BookmarkTreeNode in the callback has an index in
it or not. If it does and there's some function similar to 'insert' in Polymer
we can do that, otherwise we have to getSubTree instead. (Do we have enough time
for finishing this cl?)

[jiaxi, 2017-02-09 01:20:24]

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

https://codereview.chromium.org/2670473002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store.js:338: this.push(parent.path +
'.children', bookmarkNode);
Take a second thought, Polymer's splice could probably insert elements into the
specific index of the array. Make sure all the indexes are correct after you
insert the new element. (Check out how it's done in onBookmarkRemoved_)

This listener handles create event very well from the toolbar. But since we are
only going to use this listener for all onCreate events I think it would be
better to write a listener that works for all cases.

[angelayang, 2017-02-09 03:12:24]

I made changes in regards to Charlotte's suggestions :)

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

https://codereview.chromium.org/2670473002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store.js:337: var parent =
this.idToNodeMap_[bookmarkNode.parentId || null];
On 2017/02/09 01:13:09, jiaxi wrote:
> On 2017/02/08 07:34:03, angelayang wrote:
> > Closure complains that bookmarkNode.parentId can be undefined whilst the
> > idToNodeMap expects ?string. I'm unsure of what should be done for this.
> 
> I used /** @type {?string} */ in onSelectedFolderChanged. If you come across
any
> better options can you change the code there as well?

I'll to do the same for consistency sake for now and then see.

https://codereview.chromium.org/2670473002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store.js:338: this.push(parent.path +
'.children', bookmarkNode);
On 2017/02/09 01:20:24, jiaxi wrote:
> Take a second thought, Polymer's splice could probably insert elements into
the
> specific index of the array. Make sure all the indexes are correct after you
> insert the new element. (Check out how it's done in onBookmarkRemoved_)
> 
> This listener handles create event very well from the toolbar. But since we
are
> only going to use this listener for all onCreate events I think it would be
> better to write a listener that works for all cases.

I think what you suggested is good, thanks

[jiaxi, 2017-02-09 04:09:45]

https://codereview.chromium.org/2670473002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/store.js (right):

https://codereview.chromium.org/2670473002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store.js:342:
BookmarksStore.generatePaths(parent, 0);
The index inside the tree doesn't get updated properly here.

Suppose a children array:
[{id:1, index:0}, {id:2, index:1}]

after splice:
[{id:1, index:0}, {id:3, index:1}, {id:2, index:1}]

This is because Polymer doesn't know the rest of the BookmarkTreeNode changed as
well. Use getSubTree to update the index would be nice. (We still need to use
splice to let Polymer know the tree has a new element tho)