[MD Bookmarks] Add UI for Material Bookmarks.

[MD Bookmarks] Add UI for Material Bookmarks.

This CL adds the basic UI for the material design bookmark manager.
- list.html is the list of bookmarks.
- sidebar.html contains the folder tree.

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

Committed: https://crrev.com/ab33b38dd1014114876adbcd70bcea76d5b1e614
Cr-Commit-Position: refs/heads/master@{#441325}

[jiaxi, 2017-01-03 02:31:34]

Description was changed from

==========
[MD Bookmarks] Add UI for Material Bookmarks.

This CL adds the basic UI for the material design bookmark manager.
- list.html is the list of bookmarks.
- sidebar.html contains the folder tree.

BUG=651980
==========

to

==========
[MD Bookmarks] Add UI for Material Bookmarks.

This CL adds the basic UI for the material design bookmark manager.
- list.html is the list of bookmarks.
- sidebar.html contains the folder tree.

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

[jiaxi, 2017-01-03 02:33:21]

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

[tsergeant, 2017-01-03 23:31:17]

🙆🙆🙆

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_bookmarks/app.html (right):

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/app.html:3: <link rel="import"
href="chrome://bookmarks/sidebar.html">
Sort these: Sidebar comes after shared.

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_bookmarks/folder_node.html (right):

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/folder_node.html:24: color:
var(--inactive-folder-color);
Angela's CL renamed these variables to

--folder-active-color
--folder-inactive-color

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_bookmarks/folder_node.js (right):

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/folder_node.js:19: /** @private */
@return annotation here and on the next function.

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/folder_node.js:26: return this.item.isOpen
? 'cr:arrow-drop-up' : 'cr:arrow-drop-down';
Did you deliberately leave out the change to cr_elements/icons.html from this
CL? (Chris?) It means that cr:arrow-drop-up doesn't exist and no icon appears.

If you want to avoid changing the shared file right now, add `arrow-drop-up` to
md_bookmarks/icons.html with a TODO to move it into the shared file.

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/folder_node.js:45: /** @private */
@return annotation

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/folder_node.js:48: if
(!this.item.children[i].url) {
Remove {}

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_bookmarks/item.html (right):

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/item.html:52: <iron-icon
icon="bookmarks:folder"
This is wrapped a bit weirdly. We also normally keep ID before other things, to
help readability. Try:

<iron-icon id="folder-icon icon="bookmarks:folder"
    hidden$="[!!isFolder_]]">

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_bookmarks/item.js (right):

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/item.js:15: isFolder_: {
Use the short form here:

isFolder_: Boolean

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/item.js:39: updateFavicon_: function(url)
{
@private

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_bookmarks/list.js (right):

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/list.js:15: 'toggle-menu': 'toggleMenu_'
Prefer to name event listeners as `onEventName`, so in this case `onToggleMenu_`

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_bookmarks/sidebar.js (right):

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/sidebar.js:9: rootFolders: {
Short form here as well.

https://codereview.chromium.org/2603303002/diff/1/chrome/test/data/webui/md_b...
File chrome/test/data/webui/md_bookmarks/md_bookmarks_browsertest.js (right):

https://codereview.chromium.org/2603303002/diff/1/chrome/test/data/webui/md_b...
chrome/test/data/webui/md_bookmarks/md_bookmarks_browsertest.js:24: /**
@override */
Get rid of this again (we removed it in the last review)

https://codereview.chromium.org/2603303002/diff/1/chrome/test/data/webui/md_b...
File chrome/test/data/webui/md_bookmarks/store_test.js (right):

https://codereview.chromium.org/2603303002/diff/1/chrome/test/data/webui/md_b...
chrome/test/data/webui/md_bookmarks/store_test.js:29: test('mapNodes inserts
nodes into idToNodeMap', function(){
Undo all changes to this file. These were comments from Angela's code review.

https://codereview.chromium.org/2603303002/diff/1/chrome/test/data/webui/md_b...
File chrome/test/data/webui/md_bookmarks/test_util.js (right):

https://codereview.chromium.org/2603303002/diff/1/chrome/test/data/webui/md_b...
chrome/test/data/webui/md_bookmarks/test_util.js:6: * Replace the current
primary element of the test with a new element. Useful
As above, undo all changes to this file.

[calamity, 2017-01-04 00:43:00]

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_bookmarks/folder_node.js (right):

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/folder_node.js:26: return this.item.isOpen
? 'cr:arrow-drop-up' : 'cr:arrow-drop-down';
On 2017/01/03 23:31:17, tsergeant wrote:
> Did you deliberately leave out the change to cr_elements/icons.html from this
> CL? (Chris?) It means that cr:arrow-drop-up doesn't exist and no icon appears.
> 
> If you want to avoid changing the shared file right now, add `arrow-drop-up`
to
> md_bookmarks/icons.html with a TODO to move it into the shared file.

Yeah, I'd rather not do all the settings stuff in this CL. Add the TODO for
folder too.

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_bookmarks/list.js (right):

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/list.js:11: },
selectedNode: Object,

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/list.js:28: // TODO(jiaxi): change these
dummy click event holders later.
handlers?

[jiaxi, 2017-01-04 02:47:28]

Patchset #2 (id:20001) has been deleted

[jiaxi, 2017-01-04 02:50:16]

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_bookmarks/app.html (right):

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/app.html:3: <link rel="import"
href="chrome://bookmarks/sidebar.html">
On 2017/01/03 23:31:17, tsergeant wrote:
> Sort these: Sidebar comes after shared.

Done.

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_bookmarks/folder_node.html (right):

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/folder_node.html:24: color:
var(--inactive-folder-color);
On 2017/01/03 23:31:17, tsergeant wrote:
> Angela's CL renamed these variables to
> 
> --folder-active-color
> --folder-inactive-color

Done.

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_bookmarks/folder_node.js (right):

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/folder_node.js:19: /** @private */
On 2017/01/03 23:31:17, tsergeant wrote:
> @return annotation here and on the next function.

Done.

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/folder_node.js:26: return this.item.isOpen
? 'cr:arrow-drop-up' : 'cr:arrow-drop-down';
On 2017/01/03 23:31:17, tsergeant wrote:
> Did you deliberately leave out the change to cr_elements/icons.html from this
> CL? (Chris?) It means that cr:arrow-drop-up doesn't exist and no icon appears.
> 
> If you want to avoid changing the shared file right now, add `arrow-drop-up`
to
> md_bookmarks/icons.html with a TODO to move it into the shared file.

Done.

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/folder_node.js:26: return this.item.isOpen
? 'cr:arrow-drop-up' : 'cr:arrow-drop-down';
On 2017/01/03 23:31:17, tsergeant wrote:
> Did you deliberately leave out the change to cr_elements/icons.html from this
> CL? (Chris?) It means that cr:arrow-drop-up doesn't exist and no icon appears.
> 
> If you want to avoid changing the shared file right now, add `arrow-drop-up`
to
> md_bookmarks/icons.html with a TODO to move it into the shared file.

Done.

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/folder_node.js:45: /** @private */
On 2017/01/03 23:31:17, tsergeant wrote:
> @return annotation

Done.

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/folder_node.js:48: if
(!this.item.children[i].url) {
On 2017/01/03 23:31:17, tsergeant wrote:
> Remove {}

Done.

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_bookmarks/item.html (right):

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/item.html:52: <iron-icon
icon="bookmarks:folder"
On 2017/01/03 23:31:17, tsergeant wrote:
> This is wrapped a bit weirdly. We also normally keep ID before other things,
to
> help readability. Try:
> 
> <iron-icon id="folder-icon icon="bookmarks:folder"
>     hidden$="[!!isFolder_]]">

Done.

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_bookmarks/item.js (right):

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/item.js:15: isFolder_: {
On 2017/01/03 23:31:17, tsergeant wrote:
> Use the short form here:
> 
> isFolder_: Boolean

Done.

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/item.js:39: updateFavicon_: function(url)
{
On 2017/01/03 23:31:17, tsergeant wrote:
> @private

Done.

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_bookmarks/list.js (right):

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/list.js:11: },
On 2017/01/04 00:43:00, calamity wrote:
> selectedNode: Object,

Done.

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/list.js:15: 'toggle-menu': 'toggleMenu_'
On 2017/01/03 23:31:17, tsergeant wrote:
> Prefer to name event listeners as `onEventName`, so in this case
`onToggleMenu_`

Done.

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/list.js:28: // TODO(jiaxi): change these
dummy click event holders later.
On 2017/01/04 00:43:00, calamity wrote:
> handlers?

Done.

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_bookmarks/sidebar.js (right):

https://codereview.chromium.org/2603303002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/sidebar.js:9: rootFolders: {
On 2017/01/03 23:31:17, tsergeant wrote:
> Short form here as well.

Done.

https://codereview.chromium.org/2603303002/diff/1/chrome/test/data/webui/md_b...
File chrome/test/data/webui/md_bookmarks/md_bookmarks_browsertest.js (right):

https://codereview.chromium.org/2603303002/diff/1/chrome/test/data/webui/md_b...
chrome/test/data/webui/md_bookmarks/md_bookmarks_browsertest.js:24: /**
@override */
On 2017/01/03 23:31:17, tsergeant wrote:
> Get rid of this again (we removed it in the last review)

Done.

https://codereview.chromium.org/2603303002/diff/1/chrome/test/data/webui/md_b...
File chrome/test/data/webui/md_bookmarks/store_test.js (right):

https://codereview.chromium.org/2603303002/diff/1/chrome/test/data/webui/md_b...
chrome/test/data/webui/md_bookmarks/store_test.js:29: test('mapNodes inserts
nodes into idToNodeMap', function(){
On 2017/01/03 23:31:17, tsergeant wrote:
> Undo all changes to this file. These were comments from Angela's code review.

Done.

https://codereview.chromium.org/2603303002/diff/1/chrome/test/data/webui/md_b...
File chrome/test/data/webui/md_bookmarks/test_util.js (right):

https://codereview.chromium.org/2603303002/diff/1/chrome/test/data/webui/md_b...
chrome/test/data/webui/md_bookmarks/test_util.js:6: * Replace the current
primary element of the test with a new element. Useful
On 2017/01/03 23:31:17, tsergeant wrote:
> As above, undo all changes to this file.

Done.

[tsergeant, 2017-01-04 03:44:12]

https://codereview.chromium.org/2603303002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/list.js (right):

https://codereview.chromium.org/2603303002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/list.js:6: is: 'bookmarks-list',
Blank line after this.

https://codereview.chromium.org/2603303002/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/md_bookmarks/item_test.js (right):

https://codereview.chromium.org/2603303002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/item_test.js:13: item.isTesting_ = true;
This doesn't do anything anymore, get rid of it.

[jiaxi, 2017-01-04 04:12:34]

https://codereview.chromium.org/2603303002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/list.js (right):

https://codereview.chromium.org/2603303002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/list.js:6: is: 'bookmarks-list',
On 2017/01/04 03:44:12, tsergeant wrote:
> Blank line after this.

Done.

https://codereview.chromium.org/2603303002/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/md_bookmarks/item_test.js (right):

https://codereview.chromium.org/2603303002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/item_test.js:13: item.isTesting_ = true;
On 2017/01/04 03:44:12, tsergeant wrote:
> This doesn't do anything anymore, get rid of it.

Done.

[tsergeant, 2017-01-04 04:21:14]

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

[tsergeant, 2017-01-04 04:21:29]

lgtm

[commit-bot: I haz the power, 2017-01-04 04:21:32]

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

[calamity, 2017-01-04 04:33:07]

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

https://codereview.chromium.org/2603303002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/folder_node.js:33: //TODO(jiaxi): Move
these icons to shared file.
// TODO


But also, these TODOs should really be in the icons.html.

[jiaxi, 2017-01-04 04:41:06]

Patchset #4 (id:80001) has been deleted

[jiaxi, 2017-01-04 04:41:31]

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

https://codereview.chromium.org/2603303002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/folder_node.js:33: //TODO(jiaxi): Move
these icons to shared file.
On 2017/01/04 04:33:07, calamity wrote:
> // TODO
> 
> 
> But also, these TODOs should really be in the icons.html.

Done.

[calamity, 2017-01-04 05:01:20]

lgtm

[calamity, 2017-01-04 05:01:24]

The CQ bit was checked by calamity@chromium.org

[calamity, 2017-01-04 05:01:25]

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/2603303002/#ps100001
(title: "change TODO position")

[commit-bot: I haz the power, 2017-01-04 05:01:45]

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-01-04 05:51:20]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1483506084903870,
"parent_rev": "0f2953d64a93ebde6324e88ed92f8646b1848cd6", "commit_rev":
"bc3f7b84c63f10e4e19b7349d7359e6683313189"}

[commit-bot: I haz the power, 2017-01-04 05:52:24]

Description was changed from

==========
[MD Bookmarks] Add UI for Material Bookmarks.

This CL adds the basic UI for the material design bookmark manager.
- list.html is the list of bookmarks.
- sidebar.html contains the folder tree.

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

to

==========
[MD Bookmarks] Add UI for Material Bookmarks.

This CL adds the basic UI for the material design bookmark manager.
- list.html is the list of bookmarks.
- sidebar.html contains the folder tree.

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

Review-Url: https://codereview.chromium.org/2603303002
==========

[commit-bot: I haz the power, 2017-01-04 05:52:25]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2017-01-04 05:54:40]

Description was changed from

==========
[MD Bookmarks] Add UI for Material Bookmarks.

This CL adds the basic UI for the material design bookmark manager.
- list.html is the list of bookmarks.
- sidebar.html contains the folder tree.

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

Review-Url: https://codereview.chromium.org/2603303002
==========

to

==========
[MD Bookmarks] Add UI for Material Bookmarks.

This CL adds the basic UI for the material design bookmark manager.
- list.html is the list of bookmarks.
- sidebar.html contains the folder tree.

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

Committed: https://crrev.com/ab33b38dd1014114876adbcd70bcea76d5b1e614
Cr-Commit-Position: refs/heads/master@{#441325}
==========

[commit-bot: I haz the power, 2017-01-04 05:54:41]

Patchset 4 (id:??) landed as
https://crrev.com/ab33b38dd1014114876adbcd70bcea76d5b1e614
Cr-Commit-Position: refs/heads/master@{#441325}

[jiaxi, 2017-01-10 23:02:22]

Description was changed from

==========
[MD Bookmarks] Add UI for Material Bookmarks.

This CL adds the basic UI for the material design bookmark manager.
- list.html is the list of bookmarks.
- sidebar.html contains the folder tree.

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

Committed: https://crrev.com/ab33b38dd1014114876adbcd70bcea76d5b1e614
Cr-Commit-Position: refs/heads/master@{#441325}
==========

to

==========
[MD Bookmarks] Add UI for Material Bookmarks.

This CL adds the basic UI for the material design bookmark manager.
- list.html is the list of bookmarks.
- sidebar.html contains the folder tree.

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

Committed: https://crrev.com/ab33b38dd1014114876adbcd70bcea76d5b1e614
Cr-Commit-Position: refs/heads/master@{#441325}
==========