MD History: Display synced tabs history.

MD History: Display synced tabs history.

This CL adds a UI for displaying synced tabs in the material history page.
A sidebar has been added to navigate between history and synced tabs view.

Based on https://codereview.chromium.org/1607403004/.

BUG=425625
Committed: https://crrev.com/ffe1044406370947b780682e734840c6fc5ba711
Cr-Commit-Position: refs/heads/master@{#380922}

[calamity, 2016-02-29 02:03:03]

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

[tsergeant, 2016-03-01 03:58:23]

https://codereview.chromium.org/1729263005/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/history.js (right):

https://codereview.chromium.org/1729263005/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history.js:70:
$('synced-device-manager').hidden = e.detail.display != 'synced-tabs-button';
Did you experiment with using something other than 'hidden' to improve
performance?

https://codereview.chromium.org/1729263005/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history.js:94: * no foreign sessions, or tab
sync is disabled for this profile.
This means that it's possible to have no foreign sessions, but the sidebar will
still be enabled? Will this make it possible to switch to a totally empty page?

That's probably okay, but we should make sure we add a message in when there are
no synced tabs (follow-up is fine).

https://codereview.chromium.org/1729263005/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history.js:103: if (isTabSyncEnabled) {
Nit: {}

https://codereview.chromium.org/1729263005/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/side_bar.html (right):

https://codereview.chromium.org/1729263005/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/side_bar.html:5: <dom-module id="side-bar">
I think we should keep all of these element names 'namespaced' as
'history-side-bar' etc.

Downloads and setting both do this, although they make it a little more terse by
leaving the prefix out of filenames.

https://codereview.chromium.org/1729263005/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/side_bar.html:34: <div id="navigation-menu">
Can you get rid of #navigation-menu and just use :host instead?

Secondly, do you know if there's a reason why paper-menu wasn't used here? Seems
like it would add some nice things like keyboard control.

(md-settings uses a paper-menu with styling kinda similar to what we're doing)

https://codereview.chromium.org/1729263005/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/side_bar.html:41: <p>Open Tabs</p>
i18n this string

https://codereview.chromium.org/1729263005/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/synced_device_card.html (right):

https://codereview.chromium.org/1729263005/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/synced_device_card.html:72: #windowSeparator
{
Nit: #window-separator

https://codereview.chromium.org/1729263005/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/synced_device_card.js (right):

https://codereview.chromium.org/1729263005/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/synced_device_card.js:49:
this.$$('#dropdown-indicator').icon =
Can use $ instead of $$

https://codereview.chromium.org/1729263005/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/synced_device_manager.js (right):

https://codereview.chromium.org/1729263005/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/synced_device_manager.js:37: lastUpdateTime:
'- ' + sessionList[i].modifiedTime,
Typography nit: Make this an en-dash. Here's one ready to go: '–'.

IMO with this, it looks fine with the en-dash and no -webkit-padding-start

[tsergeant, 2016-03-01 04:00:41]

Oh, and one more thing:

You should make sure the page can never go behind the sidebar when the window is
very thin.

[calamity, 2016-03-08 02:44:09]

Patchset #3 (id:40001) has been deleted

[calamity, 2016-03-08 02:44:15]

Patchset #3 (id:60001) has been deleted

[calamity, 2016-03-08 02:46:52]

https://codereview.chromium.org/1729263005/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/history.js (right):

https://codereview.chromium.org/1729263005/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history.js:70:
$('synced-device-manager').hidden = e.detail.display != 'synced-tabs-button';
On 2016/03/01 03:58:22, tsergeant wrote:
> Did you experiment with using something other than 'hidden' to improve
> performance?

I looked into it. It didn't work out so well. This will do for now, I'd rather
fix this in a follow-up patch, especially if we have to do weird things to make
it work well.

https://codereview.chromium.org/1729263005/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history.js:94: * no foreign sessions, or tab
sync is disabled for this profile.
On 2016/03/01 03:58:22, tsergeant wrote:
> This means that it's possible to have no foreign sessions, but the sidebar
will
> still be enabled? Will this make it possible to switch to a totally empty
page?
> 
> That's probably okay, but we should make sure we add a message in when there
are
> no synced tabs (follow-up is fine).

Good point™.

https://codereview.chromium.org/1729263005/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history.js:103: if (isTabSyncEnabled) {
On 2016/03/01 03:58:22, tsergeant wrote:
> Nit: {}

Done.

https://codereview.chromium.org/1729263005/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/side_bar.html (right):

https://codereview.chromium.org/1729263005/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/side_bar.html:5: <dom-module id="side-bar">
On 2016/03/01 03:58:23, tsergeant wrote:
> I think we should keep all of these element names 'namespaced' as
> 'history-side-bar' etc.
> 
> Downloads and setting both do this, although they make it a little more terse
by
> leaving the prefix out of filenames.

Done.

https://codereview.chromium.org/1729263005/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/side_bar.html:34: <div id="navigation-menu">
On 2016/03/01 03:58:23, tsergeant wrote:
> Can you get rid of #navigation-menu and just use :host instead?
> 
> Secondly, do you know if there's a reason why paper-menu wasn't used here?
Seems
> like it would add some nice things like keyboard control.
> 
> (md-settings uses a paper-menu with styling kinda similar to what we're doing)

Done.

https://codereview.chromium.org/1729263005/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/side_bar.html:41: <p>Open Tabs</p>
On 2016/03/01 03:58:23, tsergeant wrote:
> i18n this string

Done.

https://codereview.chromium.org/1729263005/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/synced_device_card.html (right):

https://codereview.chromium.org/1729263005/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/synced_device_card.html:72: #windowSeparator
{
On 2016/03/01 03:58:23, tsergeant wrote:
> Nit: #window-separator

Done.

https://codereview.chromium.org/1729263005/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/synced_device_card.js (right):

https://codereview.chromium.org/1729263005/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/synced_device_card.js:49:
this.$$('#dropdown-indicator').icon =
On 2016/03/01 03:58:23, tsergeant wrote:
> Can use $ instead of $$

Done.

https://codereview.chromium.org/1729263005/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/synced_device_manager.js (right):

https://codereview.chromium.org/1729263005/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/synced_device_manager.js:37: lastUpdateTime:
'- ' + sessionList[i].modifiedTime,
On 2016/03/01 03:58:23, tsergeant wrote:
> Typography nit: Make this an en-dash. Here's one ready to go: '–'.
> 
> IMO with this, it looks fine with the en-dash and no -webkit-padding-start

Done.

[tsergeant, 2016-03-09 04:36:51]

Comments are split across two patch sets, sorry

https://codereview.chromium.org/1729263005/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_history/compiled_resources.gyp (right):

https://codereview.chromium.org/1729263005/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_history/compiled_resources.gyp:27:
'synced_device_card.js',
1) synced_device_card -> synced_device

2) dbeam@ has since added a compiled_resources2.gyp
(https://codereview.chromium.org/1767973002/), so you'll need to add the new
files to that as well.

https://codereview.chromium.org/1729263005/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_history/side_bar.js (right):

https://codereview.chromium.org/1729263005/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_history/side_bar.js:9: console.log(details.item.id);
Remove this log

https://codereview.chromium.org/1729263005/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_history/synced_device.html (right):

https://codereview.chromium.org/1729263005/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_history/synced_device.html:10: <dom-module
id="history-synced-device-card">
Were you going to rename this element to just

<history-synced-device> ?

https://codereview.chromium.org/1729263005/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/md_history_ui.cc (right):

https://codereview.chromium.org/1729263005/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/md_history_ui.cc:69:
source->AddResourcePath("synced_device_card.html",
We should make sure that our chrome:// paths match the files on disk, so rename

synced_device_card.{html,js} to synced_device.{html,js}

https://codereview.chromium.org/1729263005/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/md_history/synced_device.html (right):

https://codereview.chromium.org/1729263005/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_history/synced_device.html:82: {{device}}
It might be worth switching to single-way binding where appropriate here, since
we're doing that in history-list and history-item as well.

https://codereview.chromium.org/1729263005/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/md_history/synced_device.js (right):

https://codereview.chromium.org/1729263005/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_history/synced_device.js:29: cardOpen: {
Can this one be private?

https://codereview.chromium.org/1729263005/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/md_history/synced_device_manager.js (right):

https://codereview.chromium.org/1729263005/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_history/synced_device_manager.js:24: if
(this.syncedDevices.length > 0) {
Nit: {}

https://codereview.chromium.org/1729263005/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/history_synced_tabs_test.js (right):

https://codereview.chromium.org/1729263005/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_synced_tabs_test.js:8: collapsed:
false,
Nit: Deindent these to 6 spaces

[Dan Beam, 2016-03-09 04:56:10]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2016-03-09 04:56:11]

https://codereview.chromium.org/1729263005/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_history/compiled_resources.gyp (right):

https://codereview.chromium.org/1729263005/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_history/compiled_resources.gyp:27:
'synced_device_card.js',
On 2016/03/09 04:36:50, tsergeant wrote:
> 1) synced_device_card -> synced_device
> 
> 2) dbeam@ has since added a compiled_resources2.gyp
> (https://codereview.chromium.org/1767973002/), so you'll need to add the new
> files to that as well.

oops, I meant to remove this file as well :(

[Dan Beam, 2016-03-09 05:01:15]

Description was changed from

==========
MD History: Display synced tabs history.

This CL adds a UI for displaying synced tabs in the material history page.
A sidebar has been added to navigate between history and synced tabs view.

Based on https://codereview.chromium.org/1607403004/.

BUG=425625
==========

to

==========
MD History: Display synced tabs history.

This CL adds a UI for displaying synced tabs in the material history page.
A sidebar has been added to navigate between history and synced tabs view.

Based on https://codereview.chromium.org/1607403004/.

BUG=425625
==========

[Dan Beam, 2016-03-09 05:01:15]

dbeam@chromium.org changed reviewers:
- dbeam@chromium.org

[Dan Beam, 2016-03-09 06:54:49]

On 2016/03/09 04:56:11, Dan Beam wrote:
>
https://codereview.chromium.org/1729263005/diff/80001/chrome/browser/resource...
> File chrome/browser/resources/md_history/compiled_resources.gyp (right):
> 
>
https://codereview.chromium.org/1729263005/diff/80001/chrome/browser/resource...
> chrome/browser/resources/md_history/compiled_resources.gyp:27:
> 'synced_device_card.js',
> On 2016/03/09 04:36:50, tsergeant wrote:
> > 1) synced_device_card -> synced_device
> > 
> > 2) dbeam@ has since added a compiled_resources2.gyp
> > (https://codereview.chromium.org/1767973002/), so you'll need to add the new
> > files to that as well.
> 
> oops, I meant to remove this file as well :(

fixed: https://codereview.chromium.org/1773173004/

[calamity, 2016-03-10 02:49:43]

Patchset #5 (id:120001) has been deleted

[calamity, 2016-03-10 03:57:50]

Patchset #5 (id:120002) has been deleted

[calamity, 2016-03-10 04:18:25]

https://codereview.chromium.org/1729263005/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_history/compiled_resources.gyp (right):

https://codereview.chromium.org/1729263005/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_history/compiled_resources.gyp:27:
'synced_device_card.js',
On 2016/03/09 04:36:50, tsergeant wrote:
> 1) synced_device_card -> synced_device
> 
> 2) dbeam@ has since added a compiled_resources2.gyp
> (https://codereview.chromium.org/1767973002/), so you'll need to add the new
> files to that as well.

Done.

https://codereview.chromium.org/1729263005/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_history/side_bar.js (right):

https://codereview.chromium.org/1729263005/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_history/side_bar.js:9: console.log(details.item.id);
On 2016/03/09 04:36:50, tsergeant wrote:
> Remove this log

Oopz.

https://codereview.chromium.org/1729263005/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_history/synced_device.html (right):

https://codereview.chromium.org/1729263005/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_history/synced_device.html:10: <dom-module
id="history-synced-device-card">
On 2016/03/09 04:36:50, tsergeant wrote:
> Were you going to rename this element to just
> 
> <history-synced-device> ?

Hmm, actually, I think that it's kind of weird to mention what kind of UI thing
the other elements are (list, toolbar, sidebar) but call this a 'device'.

Reverted the file names and stuff.

https://codereview.chromium.org/1729263005/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/md_history_ui.cc (right):

https://codereview.chromium.org/1729263005/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/md_history_ui.cc:69:
source->AddResourcePath("synced_device_card.html",
On 2016/03/09 04:36:50, tsergeant wrote:
> We should make sure that our chrome:// paths match the files on disk, so
rename
> 
> synced_device_card.{html,js} to synced_device.{html,js}

N/A

https://codereview.chromium.org/1729263005/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/md_history/synced_device.html (right):

https://codereview.chromium.org/1729263005/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_history/synced_device.html:82: {{device}}
On 2016/03/09 04:36:50, tsergeant wrote:
> It might be worth switching to single-way binding where appropriate here,
since
> we're doing that in history-list and history-item as well.

Done.

https://codereview.chromium.org/1729263005/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/md_history/synced_device.js (right):

https://codereview.chromium.org/1729263005/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_history/synced_device.js:29: cardOpen: {
On 2016/03/09 04:36:50, tsergeant wrote:
> Can this one be private?

Done.

https://codereview.chromium.org/1729263005/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/md_history/synced_device_manager.js (right):

https://codereview.chromium.org/1729263005/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_history/synced_device_manager.js:24: if
(this.syncedDevices.length > 0) {
On 2016/03/09 04:36:50, tsergeant wrote:
> Nit: {}

Done.

https://codereview.chromium.org/1729263005/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/history_synced_tabs_test.js (right):

https://codereview.chromium.org/1729263005/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_synced_tabs_test.js:8: collapsed:
false,
On 2016/03/09 04:36:50, tsergeant wrote:
> Nit: Deindent these to 6 spaces

Done.

[tsergeant, 2016-03-10 06:02:52]

lgtm w/ one more nit

https://codereview.chromium.org/1729263005/diff/150001/chrome/browser/resourc...
File chrome/browser/resources/md_history/side_bar.js (right):

https://codereview.chromium.org/1729263005/diff/150001/chrome/browser/resourc...
chrome/browser/resources/md_history/side_bar.js:8: onSelect: function(e,
details) {
Nit: Make this private

[calamity, 2016-03-10 07:21:50]

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

[calamity, 2016-03-10 07:21:51]

+dbeam for browser_resources.grd and externs.js OWNERS.

[Dan Beam, 2016-03-10 18:57:15]

lgtm w/ a lot of nits (to not hold you up)

arguably, you should consider copying externs to md_history or adding yourselves
as owners of history/externs.js

https://codereview.chromium.org/1729263005/diff/170001/chrome/browser/resourc...
File chrome/browser/resources/history/externs.js (right):

https://codereview.chromium.org/1729263005/diff/170001/chrome/browser/resourc...
chrome/browser/resources/history/externs.js:54: var Session;
nit: ForeignSession, HistorySession?

https://codereview.chromium.org/1729263005/diff/170001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history.js (right):

https://codereview.chromium.org/1729263005/diff/170001/chrome/browser/resourc...
chrome/browser/resources/md_history/history.js:84: * @param {Array<Session>}
sessionList Array of objects describing the sessions
nit: unless this Array can be null or any of the sessions can be null,
!Array<!Session>

https://codereview.chromium.org/1729263005/diff/170001/chrome/browser/resourc...
chrome/browser/resources/md_history/history.js:92: var syncedDeviceManager = /**
@type {HistorySyncedDeviceManagerElement} */ (
nit: /** @type {HistorySyncedDeviceManagerElement} */(
                                                    ^^

https://codereview.chromium.org/1729263005/diff/170001/chrome/browser/resourc...
File chrome/browser/resources/md_history/side_bar.html (right):

https://codereview.chromium.org/1729263005/diff/170001/chrome/browser/resourc...
chrome/browser/resources/md_history/side_bar.html:27: color: #4285f4;
for any rgb() or #hex I see: is there a --paper-* or --google-* var you can use
here instead?

https://codereview.chromium.org/1729263005/diff/170001/chrome/browser/resourc...
File chrome/browser/resources/md_history/side_bar.js (right):

https://codereview.chromium.org/1729263005/diff/170001/chrome/browser/resourc...
chrome/browser/resources/md_history/side_bar.js:7: 
jsdoc (i.e. @private, @param)

https://codereview.chromium.org/1729263005/diff/170001/chrome/browser/resourc...
chrome/browser/resources/md_history/side_bar.js:9: this.fire('switch-display', {
display: details.item.id });
{display: details.item.id}
^^                      ^^

https://codereview.chromium.org/1729263005/diff/170001/chrome/browser/resourc...
File chrome/browser/resources/md_history/synced_device_card.html (right):

https://codereview.chromium.org/1729263005/diff/170001/chrome/browser/resourc...
chrome/browser/resources/md_history/synced_device_card.html:17: padding: 0 24px
20px 24px;
padding: 0 24px 20px;

https://codereview.chromium.org/1729263005/diff/170001/chrome/browser/resourc...
File chrome/browser/resources/md_history/synced_device_card.js (right):

https://codereview.chromium.org/1729263005/diff/170001/chrome/browser/resourc...
chrome/browser/resources/md_history/synced_device_card.js:21: // The list of
tabs open for this device.
can you parameterize this type?  i.e. @type {!Array<TabDataThinger>}
                                                    ^^^^^^^^^^^^^^

https://codereview.chromium.org/1729263005/diff/170001/chrome/browser/resourc...
chrome/browser/resources/md_history/synced_device_card.js:39: openAllTabs_:
function() {
nit: in other parts of the code (i.e. bookmarks manager) we have a warning if
there's over X tabs that will open (where X is like 10 or something)

maybe worth adding a todo or asking tbuckley/rpop/bettes (PMs and UX) what
should happen?

https://codereview.chromium.org/1729263005/diff/170001/chrome/browser/resourc...
File chrome/browser/resources/md_history/synced_device_manager.js (right):

https://codereview.chromium.org/1729263005/diff/170001/chrome/browser/resourc...
chrome/browser/resources/md_history/synced_device_manager.js:9: // An array of
synced devices with synced tab data.
can this be @typed as well?

https://codereview.chromium.org/1729263005/diff/170001/chrome/browser/resourc...
chrome/browser/resources/md_history/synced_device_manager.js:30: tabs =
tabs.concat(sessionList[i].windows[j].tabs);
nit: .tabs can never be empty, right?  otherwise the next line *might* throw

https://codereview.chromium.org/1729263005/diff/170001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/history_synced_tabs_test.js (right):

https://codereview.chromium.org/1729263005/diff/170001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_synced_tabs_test.js:19: for (var i in
tabUrls) {
fyi: new closure compiler versions don't allow iterating through arrays with
`in` iirc

https://codereview.chromium.org/1729263005/diff/170001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_synced_tabs_test.js:26: }
nit:

var tabs = tabUrls.map(function(tabUrl) {
  return {sessionId: 0, timestamp: 0, title: tabUrl, url: tabUrl};
});

https://codereview.chromium.org/1729263005/diff/170001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_synced_tabs_test.js:31: }
};

[calamity, 2016-03-14 02:25:14]

Patchset #7 (id:190001) has been deleted

[calamity, 2016-03-14 02:28:49]

Patchset #7 (id:210001) has been deleted

[calamity, 2016-03-14 02:29:40]

https://codereview.chromium.org/1729263005/diff/150001/chrome/browser/resourc...
File chrome/browser/resources/md_history/side_bar.js (right):

https://codereview.chromium.org/1729263005/diff/150001/chrome/browser/resourc...
chrome/browser/resources/md_history/side_bar.js:8: onSelect: function(e,
details) {
On 2016/03/10 06:02:52, tsergeant wrote:
> Nit: Make this private

Done.

https://codereview.chromium.org/1729263005/diff/170001/chrome/browser/resourc...
File chrome/browser/resources/history/externs.js (right):

https://codereview.chromium.org/1729263005/diff/170001/chrome/browser/resourc...
chrome/browser/resources/history/externs.js:54: var Session;
On 2016/03/10 18:57:14, Dan Beam wrote:
> nit: ForeignSession, HistorySession?

Done.

https://codereview.chromium.org/1729263005/diff/170001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history.js (right):

https://codereview.chromium.org/1729263005/diff/170001/chrome/browser/resourc...
chrome/browser/resources/md_history/history.js:84: * @param {Array<Session>}
sessionList Array of objects describing the sessions
On 2016/03/10 18:57:15, Dan Beam wrote:
> nit: unless this Array can be null or any of the sessions can be null,
> !Array<!Session>

Done.

https://codereview.chromium.org/1729263005/diff/170001/chrome/browser/resourc...
chrome/browser/resources/md_history/history.js:92: var syncedDeviceManager = /**
@type {HistorySyncedDeviceManagerElement} */ (
On 2016/03/10 18:57:14, Dan Beam wrote:
> nit: /** @type {HistorySyncedDeviceManagerElement} */(
>                                                     ^^

Done.

https://codereview.chromium.org/1729263005/diff/170001/chrome/browser/resourc...
File chrome/browser/resources/md_history/side_bar.html (right):

https://codereview.chromium.org/1729263005/diff/170001/chrome/browser/resourc...
chrome/browser/resources/md_history/side_bar.html:27: color: #4285f4;
On 2016/03/10 18:57:15, Dan Beam wrote:
> for any rgb() or #hex I see: is there a --paper-* or --google-* var you can
use
> here instead?

Hmm. Some.

https://codereview.chromium.org/1729263005/diff/170001/chrome/browser/resourc...
File chrome/browser/resources/md_history/side_bar.js (right):

https://codereview.chromium.org/1729263005/diff/170001/chrome/browser/resourc...
chrome/browser/resources/md_history/side_bar.js:7: 
On 2016/03/10 18:57:15, Dan Beam wrote:
> jsdoc (i.e. @private, @param)

Done.

https://codereview.chromium.org/1729263005/diff/170001/chrome/browser/resourc...
chrome/browser/resources/md_history/side_bar.js:9: this.fire('switch-display', {
display: details.item.id });
On 2016/03/10 18:57:15, Dan Beam wrote:
> {display: details.item.id}
> ^^                      ^^

Done.

https://codereview.chromium.org/1729263005/diff/170001/chrome/browser/resourc...
File chrome/browser/resources/md_history/synced_device_card.html (right):

https://codereview.chromium.org/1729263005/diff/170001/chrome/browser/resourc...
chrome/browser/resources/md_history/synced_device_card.html:17: padding: 0 24px
20px 24px;
On 2016/03/10 18:57:15, Dan Beam wrote:
> padding: 0 24px 20px;

Done.

https://codereview.chromium.org/1729263005/diff/170001/chrome/browser/resourc...
File chrome/browser/resources/md_history/synced_device_card.js (right):

https://codereview.chromium.org/1729263005/diff/170001/chrome/browser/resourc...
chrome/browser/resources/md_history/synced_device_card.js:21: // The list of
tabs open for this device.
On 2016/03/10 18:57:15, Dan Beam wrote:
> can you parameterize this type?  i.e. @type {!Array<TabDataThinger>}
>                                                     ^^^^^^^^^^^^^^

Done. Didn't realize closure did good things with Polymer annotations.

https://codereview.chromium.org/1729263005/diff/170001/chrome/browser/resourc...
chrome/browser/resources/md_history/synced_device_card.js:39: openAllTabs_:
function() {
On 2016/03/10 18:57:15, Dan Beam wrote:
> nit: in other parts of the code (i.e. bookmarks manager) we have a warning if
> there's over X tabs that will open (where X is like 10 or something)
> 
> maybe worth adding a todo or asking tbuckley/rpop/bettes (PMs and UX) what
> should happen?

Added a TODO. Will follow up with PMs/UX.

https://codereview.chromium.org/1729263005/diff/170001/chrome/browser/resourc...
File chrome/browser/resources/md_history/synced_device_manager.js (right):

https://codereview.chromium.org/1729263005/diff/170001/chrome/browser/resourc...
chrome/browser/resources/md_history/synced_device_manager.js:9: // An array of
synced devices with synced tab data.
On 2016/03/10 18:57:15, Dan Beam wrote:
> can this be @typed as well?

Done.

https://codereview.chromium.org/1729263005/diff/170001/chrome/browser/resourc...
chrome/browser/resources/md_history/synced_device_manager.js:30: tabs =
tabs.concat(sessionList[i].windows[j].tabs);
On 2016/03/10 18:57:15, Dan Beam wrote:
> nit: .tabs can never be empty, right?  otherwise the next line *might* throw

#BetterSafeThanSorry

https://codereview.chromium.org/1729263005/diff/170001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/history_synced_tabs_test.js (right):

https://codereview.chromium.org/1729263005/diff/170001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_synced_tabs_test.js:19: for (var i in
tabUrls) {
On 2016/03/10 18:57:15, Dan Beam wrote:
> fyi: new closure compiler versions don't allow iterating through arrays with
> `in` iirc

Acknowledged.

https://codereview.chromium.org/1729263005/diff/170001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_synced_tabs_test.js:26: }
On 2016/03/10 18:57:15, Dan Beam wrote:
> nit:
> 
> var tabs = tabUrls.map(function(tabUrl) {
>   return {sessionId: 0, timestamp: 0, title: tabUrl, url: tabUrl};
> });

Done.

https://codereview.chromium.org/1729263005/diff/170001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_synced_tabs_test.js:31: }
On 2016/03/10 18:57:15, Dan Beam wrote:
> };

Done.

[calamity, 2016-03-14 02:29:47]

The CQ bit was checked by calamity@chromium.org

[calamity, 2016-03-14 02:29:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
tsergeant@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1729263005/#ps230001
(title: "address_comments")

[commit-bot: I haz the power, 2016-03-14 02:29:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1729263005/230001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1729263005/230001

[commit-bot: I haz the power, 2016-03-14 03:50:35]

Description was changed from

==========
MD History: Display synced tabs history.

This CL adds a UI for displaying synced tabs in the material history page.
A sidebar has been added to navigate between history and synced tabs view.

Based on https://codereview.chromium.org/1607403004/.

BUG=425625
==========

to

==========
MD History: Display synced tabs history.

This CL adds a UI for displaying synced tabs in the material history page.
A sidebar has been added to navigate between history and synced tabs view.

Based on https://codereview.chromium.org/1607403004/.

BUG=425625
==========

[commit-bot: I haz the power, 2016-03-14 03:50:36]

Committed patchset #7 (id:230001)

[commit-bot: I haz the power, 2016-03-14 03:51:37]

Description was changed from

==========
MD History: Display synced tabs history.

This CL adds a UI for displaying synced tabs in the material history page.
A sidebar has been added to navigate between history and synced tabs view.

Based on https://codereview.chromium.org/1607403004/.

BUG=425625
==========

to

==========
MD History: Display synced tabs history.

This CL adds a UI for displaying synced tabs in the material history page.
A sidebar has been added to navigate between history and synced tabs view.

Based on https://codereview.chromium.org/1607403004/.

BUG=425625

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

[commit-bot: I haz the power, 2016-03-14 03:51:38]

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