MD History: Replace app-route with a custom router

MD History: Replace app-route with a custom router

app-route is a fairly heavy-weight element, supporting complex routing
patterns designed for large pages. This is unnecessary for MD History,
which currently has one routing pattern and one query parameter.

This CL replaces app-route with a custom router, <history-router>, which
is responsible for two-way binding between the page state and the page
URL. This also refactors handling of search terms elsewhere in the app
to ensure that changes to the search term are reflected correctly across
the page.

This CL has a small positive impact on load time (~20ms on a Z620), code
health (all routing is handled in one place) and binary size (app-route
can be deleted from the binary).

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

Committed: https://crrev.com/73e5d0ee1756905fbb7d9ca932394735ac2a2f67
Cr-Commit-Position: refs/heads/master@{#420590}

[tsergeant, 2016-09-21 06:50:10]

Description was changed from

==========
MD History: Replace app-route with a custom router

app-route is a fairly heavy-weight element, supporting complex routing
patterns designed for large pages. This is unnecessary for MD History,
which has one routing pattern and one query parameter.

This CL replaces app-route with a custom router, <history-router>, which
is responsible for two-way binding between the page state and the page
URL. This also refactors handling of search terms elsewhere in the app
to ensure that changes to the search term are reflected correctly across
the page.

This CL has a small positive impact on load time (~20ms on a Z620), code
health (all routing is handled in one place) and binary size (app-route
is deleted from the binary).

BUG=629710
==========

to

==========
MD History: Replace app-route with a custom router

app-route is a fairly heavy-weight element, supporting complex routing
patterns designed for large pages. This is unnecessary for MD History,
which has one routing pattern and one query parameter.

This CL replaces app-route with a custom router, <history-router>, which
is responsible for two-way binding between the page state and the page
URL. This also refactors handling of search terms elsewhere in the app
to ensure that changes to the search term are reflected correctly across
the page.

This CL has a small positive impact on load time (~20ms on a Z620), code
health (all routing is handled in one place) and binary size (app-route
is deleted from the binary).

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

[tsergeant, 2016-09-21 06:52:35]

Description was changed from

==========
MD History: Replace app-route with a custom router

app-route is a fairly heavy-weight element, supporting complex routing
patterns designed for large pages. This is unnecessary for MD History,
which has one routing pattern and one query parameter.

This CL replaces app-route with a custom router, <history-router>, which
is responsible for two-way binding between the page state and the page
URL. This also refactors handling of search terms elsewhere in the app
to ensure that changes to the search term are reflected correctly across
the page.

This CL has a small positive impact on load time (~20ms on a Z620), code
health (all routing is handled in one place) and binary size (app-route
is deleted from the binary).

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

to

==========
MD History: Replace app-route with a custom router

app-route is a fairly heavy-weight element, supporting complex routing
patterns designed for large pages. This is unnecessary for MD History,
which currently has one routing pattern and one query parameter.

This CL replaces app-route with a custom router, <history-router>, which
is responsible for two-way binding between the page state and the page
URL. This also refactors handling of search terms elsewhere in the app
to ensure that changes to the search term are reflected correctly across
the page.

This CL has a small positive impact on load time (~20ms on a Z620), code
health (all routing is handled in one place) and binary size (app-route
is deleted from the binary).

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

[tsergeant, 2016-09-21 07:41:04]

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

[commit-bot: I haz the power, 2016-09-21 07:41:18]

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

[tsergeant, 2016-09-21 07:43:18]

Description was changed from

==========
MD History: Replace app-route with a custom router

app-route is a fairly heavy-weight element, supporting complex routing
patterns designed for large pages. This is unnecessary for MD History,
which currently has one routing pattern and one query parameter.

This CL replaces app-route with a custom router, <history-router>, which
is responsible for two-way binding between the page state and the page
URL. This also refactors handling of search terms elsewhere in the app
to ensure that changes to the search term are reflected correctly across
the page.

This CL has a small positive impact on load time (~20ms on a Z620), code
health (all routing is handled in one place) and binary size (app-route
is deleted from the binary).

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

to

==========
MD History: Replace app-route with a custom router

app-route is a fairly heavy-weight element, supporting complex routing
patterns designed for large pages. This is unnecessary for MD History,
which currently has one routing pattern and one query parameter.

This CL replaces app-route with a custom router, <history-router>, which
is responsible for two-way binding between the page state and the page
URL. This also refactors handling of search terms elsewhere in the app
to ensure that changes to the search term are reflected correctly across
the page.

This CL has a small positive impact on load time (~20ms on a Z620), code
health (all routing is handled in one place) and binary size (app-route
can be deleted from the binary).

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

[tsergeant, 2016-09-21 07:53:39]

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

[commit-bot: I haz the power, 2016-09-21 07:53:47]

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

[tsergeant, 2016-09-21 07:57:46]

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

[tsergeant, 2016-09-21 07:57:47]

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

https://codereview.chromium.org/2352293002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/app.js:273: if (this.$.content.selectedItem)
{
These guards are necessary because of tests.

I don't understand why, but I was getting a sequence of 4 calls to
historyViewChanged_ with `selectedItem` as `undefined` once and "history" every
other time. It was unclear why historyViewChanged_ was even being called -- I
couldn't reproduce anything like it in a regular browser.

I can try going back and debugging why this happened if you're concerned, but
I'm fairly certain that it wasn't anything which will affect the page outside of
tests.

[commit-bot: I haz the power, 2016-09-21 08:55:12]

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

[commit-bot: I haz the power, 2016-09-21 08:55:13]

Dry run: This issue passed the CQ dry run.

[calamity, 2016-09-22 05:12:26]

lgtm.

https://codereview.chromium.org/2352293002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/vulcanized.html (right):

https://codereview.chromium.org/2352293002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_downloads/vulcanized.html:3157: <g id="delete"><path
d="M6 19c0 1.1.9 2 2 2h8c1.1 0 2-.9 2-2V7H6v12zM19 4h-3.5l-1-1h-5l-1
1H5v2h14V4z"></path></g>
Why did this change?

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

https://codereview.chromium.org/2352293002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/app.html:8: <link rel="import"
href="chrome://history/history_router.html">
I thought we were going to stop prefixing everything with history_?

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

https://codereview.chromium.org/2352293002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/app.js:78: 'search-domain': 'searchDomain_',
Remove.

https://codereview.chromium.org/2352293002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/app.js:273: if (this.$.content.selectedItem)
{
On 2016/09/21 07:57:47, tsergeant wrote:
> These guards are necessary because of tests.
> 
> I don't understand why, but I was getting a sequence of 4 calls to
> historyViewChanged_ with `selectedItem` as `undefined` once and "history"
every
> other time. It was unclear why historyViewChanged_ was even being called -- I
> couldn't reproduce anything like it in a regular browser.
> 
> I can try going back and debugging why this happened if you're concerned, but
> I'm fairly certain that it wasn't anything which will affect the page outside
of
> tests.

Looks fine. Invert and early return with comment.

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

https://codereview.chromium.org/2352293002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_router.js:60: queryStateChanged_:
function() {
searchTermChanged_, perhaps?

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

https://codereview.chromium.org/2352293002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_toolbar.js:70: get searchBar() {
Might be a good time to rename this 'searchField'.

https://codereview.chromium.org/2352293002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_history/side_bar.js (left):

https://codereview.chromium.org/2352293002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/side_bar.js:55: getQueryString_:
function(route) { return window.location.search; }
Without the query string, middle click goes to a different place than left click
if there's a query.

[tsergeant, 2016-09-22 23:53:49]

Patchset #4 (id:60001) has been deleted

[tsergeant, 2016-09-22 23:57:42]

+dbeam@ for find_unused_elements.py.

I've got a follow-up patch to delete app-route ready to go, I just didn't want
to double the size of this CL.

https://codereview.chromium.org/2352293002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/vulcanized.html (right):

https://codereview.chromium.org/2352293002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_downloads/vulcanized.html:3157: <g id="delete"><path
d="M6 19c0 1.1.9 2 2 2h8c1.1 0 2-.9 2-2V7H6v12zM19 4h-3.5l-1-1h-5l-1
1H5v2h14V4z"></path></g>
On 2016/09/22 05:12:26, calamity wrote:
> Why did this change?

Everyone was clamoring to add the delete icon to downloads. michaelpg@ ended up
winning the race
(https://chromium.googlesource.com/chromium/src/+/b245b7d270fafab80c2eca0821a0...)

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

https://codereview.chromium.org/2352293002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/app.html:8: <link rel="import"
href="chrome://history/history_router.html">
On 2016/09/22 05:12:26, calamity wrote:
> I thought we were going to stop prefixing everything with history_?

True. Renamed to router.html

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

https://codereview.chromium.org/2352293002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/app.js:78: 'search-domain': 'searchDomain_',
On 2016/09/22 05:12:26, calamity wrote:
> Remove.

Good pickup, done.

https://codereview.chromium.org/2352293002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/app.js:273: if (this.$.content.selectedItem)
{
On 2016/09/22 05:12:26, calamity wrote:
> On 2016/09/21 07:57:47, tsergeant wrote:
> > These guards are necessary because of tests.
> > 
> > I don't understand why, but I was getting a sequence of 4 calls to
> > historyViewChanged_ with `selectedItem` as `undefined` once and "history"
> every
> > other time. It was unclear why historyViewChanged_ was even being called --
I
> > couldn't reproduce anything like it in a regular browser.
> > 
> > I can try going back and debugging why this happened if you're concerned,
but
> > I'm fairly certain that it wasn't anything which will affect the page
outside
> of
> > tests.
> 
> Looks fine. Invert and early return with comment.

Done.

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

https://codereview.chromium.org/2352293002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_router.js:60: queryStateChanged_:
function() {
On 2016/09/22 05:12:26, calamity wrote:
> searchTermChanged_, perhaps?

Done.

[tsergeant, 2016-09-22 23:58:01]

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

[tsergeant, 2016-09-22 23:58:02]

Actually +dbeam this time.

[tsergeant, 2016-09-23 00:37:44]

Whoops, I totally missed a couple of comments. I'm not having a good run with
this CL.

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

https://codereview.chromium.org/2352293002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_toolbar.js:70: get searchBar() {
On 2016/09/22 05:12:26, calamity wrote:
> Might be a good time to rename this 'searchField'.

Done.

https://codereview.chromium.org/2352293002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_history/side_bar.js (left):

https://codereview.chromium.org/2352293002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/side_bar.js:55: getQueryString_:
function(route) { return window.location.search; }
On 2016/09/22 05:12:26, calamity wrote:
> Without the query string, middle click goes to a different place than left
click
> if there's a query.

Yeah. This is a tradeoff I'm willing to make to simplify the code. Let me know
if you think we should keep this behavior.

[tsergeant, 2016-09-23 00:37:59]

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

[commit-bot: I haz the power, 2016-09-23 00:38:31]

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, 2016-09-23 02:03:09]

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

[commit-bot: I haz the power, 2016-09-23 02:03:11]

Dry run: This issue passed the CQ dry run.

[tsergeant, 2016-09-23 03:29:52]

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

[commit-bot: I haz the power, 2016-09-23 03:30:16]

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, 2016-09-23 04:32:42]

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

[commit-bot: I haz the power, 2016-09-23 04:32:43]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2016-09-23 05:50:46]

lgtm

https://codereview.chromium.org/2352293002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/md_history/list_container.js (right):

https://codereview.chromium.org/2352293002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_history/list_container.js:155: 
^H

[tsergeant, 2016-09-23 06:59:19]

Thanks!

https://codereview.chromium.org/2352293002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/md_history/list_container.js (right):

https://codereview.chromium.org/2352293002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_history/list_container.js:155: 
On 2016/09/23 05:50:46, Dan Beam wrote:
> ^H

^H-ed

[tsergeant, 2016-09-23 06:59:27]

The CQ bit was checked by tsergeant@chromium.org

[tsergeant, 2016-09-23 06:59:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
calamity@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2352293002/#ps160001
(title: "Delete stray newline")

[commit-bot: I haz the power, 2016-09-23 06:59:38]

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, 2016-09-23 11:27:57]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2016-09-23 11:29:56]

Description was changed from

==========
MD History: Replace app-route with a custom router

app-route is a fairly heavy-weight element, supporting complex routing
patterns designed for large pages. This is unnecessary for MD History,
which currently has one routing pattern and one query parameter.

This CL replaces app-route with a custom router, <history-router>, which
is responsible for two-way binding between the page state and the page
URL. This also refactors handling of search terms elsewhere in the app
to ensure that changes to the search term are reflected correctly across
the page.

This CL has a small positive impact on load time (~20ms on a Z620), code
health (all routing is handled in one place) and binary size (app-route
can be deleted from the binary).

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

to

==========
MD History: Replace app-route with a custom router

app-route is a fairly heavy-weight element, supporting complex routing
patterns designed for large pages. This is unnecessary for MD History,
which currently has one routing pattern and one query parameter.

This CL replaces app-route with a custom router, <history-router>, which
is responsible for two-way binding between the page state and the page
URL. This also refactors handling of search terms elsewhere in the app
to ensure that changes to the search term are reflected correctly across
the page.

This CL has a small positive impact on load time (~20ms on a Z620), code
health (all routing is handled in one place) and binary size (app-route
can be deleted from the binary).

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

Committed: https://crrev.com/73e5d0ee1756905fbb7d9ca932394735ac2a2f67
Cr-Commit-Position: refs/heads/master@{#420590}
==========

[commit-bot: I haz the power, 2016-09-23 11:29:57]

Patchset 8 (id:??) landed as
https://crrev.com/73e5d0ee1756905fbb7d9ca932394735ac2a2f67
Cr-Commit-Position: refs/heads/master@{#420590}