[MD History] Refactor history.html into a history-app element.

[MD History] Refactor history.html into a history-app element.

This CL refactors the top level structure of the MD History page so that
a top level app element exists which can take advantage of Polymer's
data binding.

BUG=

Committed: https://crrev.com/123c2cf29aa4890ce71e524b250ca21c7caff103
Cr-Commit-Position: refs/heads/master@{#391575}

[calamity, 2016-04-30 00:57:55]

Patchset #1 (id:1) has been deleted

[calamity, 2016-04-30 01:04:23]

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

[calamity, 2016-04-30 01:04:24]

Oracle is down so I can't download Java to run the closure compile so I'll deal
with that later. Can you take a look at the direction of this patch before I get
down and dirty with the closure stuff?

Thanks.

[calamity, 2016-04-30 01:04:33]

Patchset #1 (id:20001) has been deleted

[tsergeant, 2016-05-02 16:19:20]

You should also update suiteSetup in md_history_browsertest.js to reflect the
new page structure.

https://codereview.chromium.org/1932413003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_history/app.js (right):

https://codereview.chromium.org/1932413003/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/app.js:16: // TODO(calamity): Replace these
event listeners with data bound properties.
Move these event listeners into their own functions and declare them in a
listeners: {} block.

https://codereview.chromium.org/1932413003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_history/history.html (right):

https://codereview.chromium.org/1932413003/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history.html:73: <link rel="import"
href="chrome://history/elements.html" async id="bundle">
Remove elements.html entirely and just import app.html with async?

[calamity, 2016-05-02 21:31:12]

Also did the suitesetup thing

https://codereview.chromium.org/1932413003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_history/app.js (right):

https://codereview.chromium.org/1932413003/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/app.js:16: // TODO(calamity): Replace these
event listeners with data bound properties.
On 2016/05/02 16:19:20, tsergeant (MTV) wrote:
> Move these event listeners into their own functions and declare them in a
> listeners: {} block.

Done.

https://codereview.chromium.org/1932413003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_history/history.html (right):

https://codereview.chromium.org/1932413003/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history.html:73: <link rel="import"
href="chrome://history/elements.html" async id="bundle">
On 2016/05/02 16:19:20, tsergeant (MTV) wrote:
> Remove elements.html entirely and just import app.html with async?

Done.

[tsergeant, 2016-05-03 00:05:10]

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

https://codereview.chromium.org/1932413003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_history/history.js:66:
appElem.setForeignSessions(sessionList, isTabSyncEnabled);
Closure cast here

https://codereview.chromium.org/1932413003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_history/history.js:76: appElem.deleteComplete();
Closure cast here too

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

https://codereview.chromium.org/1932413003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_history/side_bar.html:40: <paper-item
id="synced-tabs-button" view-id="history-synced-device-manager">
Looks like this line is over 80 characters.

Can you get rid of the IDs from here? I don't think they're necessary and that
would save some space.

https://codereview.chromium.org/1932413003/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/md_history/md_history_browsertest.js (right):

https://codereview.chromium.org/1932413003/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/md_history_browsertest.js:46:
waitForUpgrade($('history-app')),
No need for Promise.all anymore, since there's just one promise

[calamity, 2016-05-03 23:07:19]

There's some weird junk happening with the tests... The synced tabs tests breaks
when calling window.setForeignSessions, I have to call app.setForeignSessions
before it works. I'm not sure what it could be =S

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

https://codereview.chromium.org/1932413003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_history/history.js:66:
appElem.setForeignSessions(sessionList, isTabSyncEnabled);
On 2016/05/03 00:05:10, tsergeant (MTV) wrote:
> Closure cast here

Done.

https://codereview.chromium.org/1932413003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_history/history.js:76: appElem.deleteComplete();
On 2016/05/03 00:05:10, tsergeant (MTV) wrote:
> Closure cast here too

Done.

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

https://codereview.chromium.org/1932413003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_history/side_bar.html:40: <paper-item
id="synced-tabs-button" view-id="history-synced-device-manager">
On 2016/05/03 00:05:10, tsergeant (MTV) wrote:
> Looks like this line is over 80 characters.
> 
> Can you get rid of the IDs from here? I don't think they're necessary and that
> would save some space.

Done.

https://codereview.chromium.org/1932413003/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/md_history/md_history_browsertest.js (right):

https://codereview.chromium.org/1932413003/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/md_history_browsertest.js:46:
waitForUpgrade($('history-app')),
On 2016/05/03 00:05:10, tsergeant (MTV) wrote:
> No need for Promise.all anymore, since there's just one promise

Done.

[tsergeant, 2016-05-03 23:30:21]

lgtm with a nit about closure.

The workaround for the synced tabs test seems fine, though it would be nice to
figure out why it happens.

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

https://codereview.chromium.org/1932413003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_history/history.js:33: /** @type {HistoryAppElement}
*/(appElem).historyResult(info, results);
Nit: This cast was the way it was because technically `appElem` isn't a
HistoryAppElement until after it has been upgraded. Doing it this way means that
closure will complain if you call a function on it outside of the
`waitForUpgrade` block.

It's a bit janky, but we should probably leave it that way.

[tsergeant, 2016-05-03 23:50:37]

Changes to the test look better, still lgtm

[calamity, 2016-05-04 00:02:42]

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

[calamity, 2016-05-04 00:02:43]

+dbeam for browser_resources.grd OWNERS.

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

https://codereview.chromium.org/1932413003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_history/history.js:33: /** @type {HistoryAppElement}
*/(appElem).historyResult(info, results);
On 2016/05/03 23:30:21, tsergeant (MTV) wrote:
> Nit: This cast was the way it was because technically `appElem` isn't a
> HistoryAppElement until after it has been upgraded. Doing it this way means
that
> closure will complain if you call a function on it outside of the
> `waitForUpgrade` block.
> 
> It's a bit janky, but we should probably leave it that way.

Done.

[Dan Beam, 2016-05-04 00:10:57]

i love stamping things
especially browser_
resources dot grd

lgtm

[calamity, 2016-05-04 17:12:22]

The CQ bit was checked by calamity@chromium.org

[calamity, 2016-05-04 17:12:23]

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/1932413003/#ps160001
(title: "address nit")

[commit-bot: I haz the power, 2016-05-04 17:12:45]

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

[calamity, 2016-05-04 17:13:25]

On 2016/05/04 00:10:57, Dan Beam wrote:
> i love stamping things
> especially browser_
> resources dot grd
> 
> lgtm

I will grudgingly count the underscore as a syllable.

[commit-bot: I haz the power, 2016-05-04 18:30:49]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2016-05-04 18:32:22]

Description was changed from

==========
[MD History] Refactor history.html into a history-app element.

This CL refactors the top level structure of the MD History page so that
a top level app element exists which can take advantage of Polymer's
data binding.

BUG=
==========

to

==========
[MD History] Refactor history.html into a history-app element.

This CL refactors the top level structure of the MD History page so that
a top level app element exists which can take advantage of Polymer's
data binding.

BUG=

Committed: https://crrev.com/123c2cf29aa4890ce71e524b250ca21c7caff103
Cr-Commit-Position: refs/heads/master@{#391575}
==========

[commit-bot: I haz the power, 2016-05-04 18:32:23]

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

[Dan Beam, 2016-05-05 16:29:48]

On 2016/05/04 17:13:25, calamity wrote:
> On 2016/05/04 00:10:57, Dan Beam wrote:
> > i love stamping things
> > especially browser_
> > resources dot grd
> > 
> > lgtm
> 
> I will grudgingly count the underscore as a syllable.

whoops