[MD Extensions] Add support for URL navigation

[MD Extensions] Add support for URL navigation

The chrome://extensions page uses different urls for a few things,
primarily to highlight a given extension or to display the extension's
options. In the MD version, we have separate pages for these, which
makes these more like navigations.

Add basic page and history manipulation to the chrome://extensions page.
Navigating to a subpage updates the location and pushes a new state to
the history stack, so that clicking back will return you to the previous
page. Dialogs are contained within a page, and do not push a new
history state.

Examples:
- Navigating from the main view to the details view updates the
  location, and 'back' would return to the main view.
- Opening the options dialog from the details view does not push new state
  (since otherwise 'back' would just close the dialog).

Add tests for the same.

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

Review-Url: https://codereview.chromium.org/2811993004
Cr-Commit-Position: refs/heads/master@{#469216}
Committed: https://chromium.googlesource.com/chromium/src/+/8904a23861a4ec7ef5f8851a55d68c198c3ce829

[Devlin, 2017-04-12 16:23:01]

Description was changed from

==========
wip page state

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

to

==========
wip page state

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

[Devlin, 2017-04-12 20:46:13]

Description was changed from

==========
wip page state

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

to

==========
[MD Extensions] Add support for URL navigation

The chrome://extensions page uses different urls for a few things,
primarily to highlight a given extension or to display the extension's
options. In the MD version, we have separate pages for these, which
makes these more like navigations.

Add basic page and history manipulation to the chrome://extensions page.
Navigating to a subpage updates the location and pushes a new state to
the history stack, so that clicking back will return you to the previous
page. Subpages are contained within a page, and do not push a new
history state.

Examples:
- Navigating from the main view to the details view updates the
  location, and 'back' would return to the main view.
- Opening the options page from the details view does not push new state
  (since otherwise 'back' would just close the dialog).

Add tests for the same.

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

[Devlin, 2017-04-14 15:13:10]

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

[commit-bot: I haz the power, 2017-04-14 15:13:20]

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

[Devlin, 2017-04-14 16:04:10]

rdevlin.cronin@chromium.org changed reviewers:
+ michaelpg@chromium.org

[Devlin, 2017-04-14 16:04:12]

Michael, mind taking a look?

If you wanna see how this works in person, feel free to swing by my desk.

[commit-bot: I haz the power, 2017-04-14 17:06:17]

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

[commit-bot: I haz the power, 2017-04-14 17:06:18]

Dry run: This issue passed the CQ dry run.

[michaelpg, 2017-04-14 20:23:53]

> Navigating to a subpage updates the location and pushes a new state to
> the history stack, so that clicking back will return you to the previous
> page. Subpages are contained within a page, and do not push a new
> history state.

Err, which is it?

https://codereview.chromium.org/2811993004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_extensions/manager.js (right):

https://codereview.chromium.org/2811993004/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_extensions/manager.js:127: this.changePage({page:
Page.DETAILS, id: this.currentPage_.id});
this seems to contradict the comment: if we're on the details page, change to
the details page? Are we trying to update the URL here and nothing else?

https://codereview.chromium.org/2811993004/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_extensions/manager.js:315: * @param {boolean=}
isSilent
document isSilent

https://codereview.chromium.org/2811993004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_extensions/page_state.js (right):

https://codereview.chromium.org/2811993004/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_extensions/page_state.js:11: LIST: '0',
could these values be IDs? neon-animated-pages should be able to key off those,
and then you wouldn't have to worry about order (which should be irrelevant),
and you could assert that each ID exists in the DOM to self-enforce this.

https://codereview.chromium.org/2811993004/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_extensions/page_state.js:30: /** @constructor */
document what PageState is for.
also type and document the changePage param.

https://codereview.chromium.org/2811993004/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_extensions/page_state.js:31: function
PageState(changePage) {
should this be a singleton, since it manipulates history? Or is it intended as
an auxiliary helper class that basically notifies its owner of a page change?

https://codereview.chromium.org/2811993004/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_extensions/page_state.js:39: var state =
this.getCurrentPage();
nit: skip variable declaration

https://codereview.chromium.org/2811993004/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_extensions/page_state.js:58: var id =
search.get('errors');
remove "var"

[Devlin, 2017-04-17 23:29:03]

Description was changed from

==========
[MD Extensions] Add support for URL navigation

The chrome://extensions page uses different urls for a few things,
primarily to highlight a given extension or to display the extension's
options. In the MD version, we have separate pages for these, which
makes these more like navigations.

Add basic page and history manipulation to the chrome://extensions page.
Navigating to a subpage updates the location and pushes a new state to
the history stack, so that clicking back will return you to the previous
page. Subpages are contained within a page, and do not push a new
history state.

Examples:
- Navigating from the main view to the details view updates the
  location, and 'back' would return to the main view.
- Opening the options page from the details view does not push new state
  (since otherwise 'back' would just close the dialog).

Add tests for the same.

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

to

==========
[MD Extensions] Add support for URL navigation

The chrome://extensions page uses different urls for a few things,
primarily to highlight a given extension or to display the extension's
options. In the MD version, we have separate pages for these, which
makes these more like navigations.

Add basic page and history manipulation to the chrome://extensions page.
Navigating to a subpage updates the location and pushes a new state to
the history stack, so that clicking back will return you to the previous
page. Dialogs are contained within a page, and do not push a new
history state.

Examples:
- Navigating from the main view to the details view updates the
  location, and 'back' would return to the main view.
- Opening the options dialog from the details view does not push new state
  (since otherwise 'back' would just close the dialog).

Add tests for the same.

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

[Devlin, 2017-04-17 23:29:25]

renamed PageState -> NavigationHelper, PageEntry -> PageState per our
discussion.

https://codereview.chromium.org/2811993004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_extensions/manager.js (right):

https://codereview.chromium.org/2811993004/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_extensions/manager.js:127: this.changePage({page:
Page.DETAILS, id: this.currentPage_.id});
On 2017/04/14 20:23:53, michaelpg wrote:
> this seems to contradict the comment: if we're on the details page, change to
> the details page? Are we trying to update the URL here and nothing else?

Tried to clear it up with some comments.

https://codereview.chromium.org/2811993004/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_extensions/manager.js:315: * @param {boolean=}
isSilent
On 2017/04/14 20:23:53, michaelpg wrote:
> document isSilent

Done.

https://codereview.chromium.org/2811993004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_extensions/page_state.js (right):

https://codereview.chromium.org/2811993004/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_extensions/page_state.js:11: LIST: '0',
On 2017/04/14 20:23:53, michaelpg wrote:
> could these values be IDs? neon-animated-pages should be able to key off
those,
> and then you wouldn't have to worry about order (which should be irrelevant),
> and you could assert that each ID exists in the DOM to self-enforce this.

Using ids doesn't seem to work...  I thought neon-animated-pages.selected always
used index?

https://codereview.chromium.org/2811993004/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_extensions/page_state.js:30: /** @constructor */
On 2017/04/14 20:23:53, michaelpg wrote:
> document what PageState is for.
> also type and document the changePage param.

Done.

https://codereview.chromium.org/2811993004/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_extensions/page_state.js:31: function
PageState(changePage) {
On 2017/04/14 20:23:53, michaelpg wrote:
> should this be a singleton, since it manipulates history? Or is it intended as
> an auxiliary helper class that basically notifies its owner of a page change?

Conceptually, it should be a singleton (just as manager should be), but it
seemed unnecessary to strictly enforce that.

https://codereview.chromium.org/2811993004/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_extensions/page_state.js:39: var state =
this.getCurrentPage();
On 2017/04/14 20:23:53, michaelpg wrote:
> nit: skip variable declaration

Done.

https://codereview.chromium.org/2811993004/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_extensions/page_state.js:58: var id =
search.get('errors');
On 2017/04/14 20:23:53, michaelpg wrote:
> remove "var"

Done.

[michaelpg, 2017-04-18 22:03:12]

This is making a lot more sense to me now. Maybe I was just confused by the
names before.

You may want to add tommycli@ as he has more insights related to page routing
from Settings.

https://codereview.chromium.org/2811993004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_extensions/manager.js (right):

https://codereview.chromium.org/2811993004/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_extensions/manager.js:127: this.changePage({page:
Page.DETAILS, id: this.currentPage_.id});
On 2017/04/17 23:29:24, Devlin wrote:
> On 2017/04/14 20:23:53, michaelpg wrote:
> > this seems to contradict the comment: if we're on the details page, change
to
> > the details page? Are we trying to update the URL here and nothing else?
> 
> Tried to clear it up with some comments.

Thanks.

https://codereview.chromium.org/2811993004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_extensions/page_state.js (right):

https://codereview.chromium.org/2811993004/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_extensions/page_state.js:11: LIST: '0',
On 2017/04/17 23:29:24, Devlin wrote:
> On 2017/04/14 20:23:53, michaelpg wrote:
> > could these values be IDs? neon-animated-pages should be able to key off
> those,
> > and then you wouldn't have to worry about order (which should be
irrelevant),
> > and you could assert that each ID exists in the DOM to self-enforce this.
> 
> Using ids doesn't seem to work...  I thought neon-animated-pages.selected
always
> used index?

it's using iron-selector, which has the "attrForSelected" property:

    <neon-animated-pages attr-for-selected="id"> <foo id="..."> ...

I *assume* "id" would work there.
https://github.com/PolymerElements/iron-selector/blob/master/iron-selectable....

https://codereview.chromium.org/2811993004/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_extensions/page_state.js:31: function
PageState(changePage) {
On 2017/04/17 23:29:24, Devlin wrote:
> On 2017/04/14 20:23:53, michaelpg wrote:
> > should this be a singleton, since it manipulates history? Or is it intended
as
> > an auxiliary helper class that basically notifies its owner of a page
change?
> 
> Conceptually, it should be a singleton (just as manager should be), but it
> seemed unnecessary to strictly enforce that.

Sure. The new naming makes it clearer, too.

https://codereview.chromium.org/2811993004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_extensions/navigation_helper.js (right):

https://codereview.chromium.org/2811993004/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_extensions/navigation_helper.js:34: * @param
{!function(!PageState:void)} changePage A function to call when the
It also takes a boolean (maybe)

https://codereview.chromium.org/2811993004/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_extensions/navigation_helper.js:38: function
NavigationHelper(changePage) {
nit: changePageObserver? onChangePage?

https://codereview.chromium.org/2811993004/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_extensions/navigation_helper.js:46:
this.changePage_(this.getCurrentPage(), false);
Is |false| unused here?

I think the interface is clearer if this function is only called with the new
page state. We already know it means that the user navigates the browser
somewhere; it should be up to the manager how to handle that.

And I think that's actually what you already have, from where Manager constructs
NavigationHelper -- it's a detail the Manager should, and already does, keep
internally (by currying a |true| argument)

https://codereview.chromium.org/2811993004/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_extensions/navigation_helper.js:78: onPageChange:
function(entry) {
If you don't mind some more naming bikeshedding, how about "updateHistory"? It's
not actually observing an event (unlike onPopState_), it's a public method to be
called by clients to do a thing.

That would also make me less likely to confuse the purpose of this.changePage_
and this.onPageChange.

https://codereview.chromium.org/2811993004/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_extensions/navigation_helper.js:97: }
instead of `if (relative)`, maybe `default: return;` to make the control flow
clearer. also something the C++ style guide mandates, but that's not directly
relevant because we don't have a real compiler.

https://codereview.chromium.org/2811993004/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_extensions/navigation_helper.js:112: 
remove line

[Devlin, 2017-04-27 00:51:01]

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

[commit-bot: I haz the power, 2017-04-27 00:51:37]

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, 2017-04-27 03:56:27]

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

[commit-bot: I haz the power, 2017-04-27 03:56:28]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-04-27 13:41:26]

Sorry for the delay in getting this back

https://codereview.chromium.org/2811993004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_extensions/navigation_helper.js (right):

https://codereview.chromium.org/2811993004/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_extensions/navigation_helper.js:34: * @param
{!function(!PageState:void)} changePage A function to call when the
On 2017/04/18 22:03:11, michaelpg wrote:
> It also takes a boolean (maybe)

It doesn't.  Below was a mistake.

https://codereview.chromium.org/2811993004/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_extensions/navigation_helper.js:38: function
NavigationHelper(changePage) {
On 2017/04/18 22:03:12, michaelpg wrote:
> nit: changePageObserver? onChangePage?

onHistoryChange, to match below?

https://codereview.chromium.org/2811993004/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_extensions/navigation_helper.js:46:
this.changePage_(this.getCurrentPage(), false);
On 2017/04/18 22:03:12, michaelpg wrote:
> Is |false| unused here?
> 
> I think the interface is clearer if this function is only called with the new
> page state. We already know it means that the user navigates the browser
> somewhere; it should be up to the manager how to handle that.
> 
> And I think that's actually what you already have, from where Manager
constructs
> NavigationHelper -- it's a detail the Manager should, and already does, keep
> internally (by currying a |true| argument)

Yes, false is unused and shouldn't be there.  Removed.

https://codereview.chromium.org/2811993004/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_extensions/navigation_helper.js:78: onPageChange:
function(entry) {
On 2017/04/18 22:03:12, michaelpg wrote:
> If you don't mind some more naming bikeshedding, how about "updateHistory"?
It's
> not actually observing an event (unlike onPopState_), it's a public method to
be
> called by clients to do a thing.
> 
> That would also make me less likely to confuse the purpose of this.changePage_
> and this.onPageChange.

Sure!  Done.

https://codereview.chromium.org/2811993004/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_extensions/navigation_helper.js:97: }
On 2017/04/18 22:03:12, michaelpg wrote:
> instead of `if (relative)`, maybe `default: return;` to make the control flow
> clearer. also something the C++ style guide mandates, but that's not directly
> relevant because we don't have a real compiler.

I think we can actually assert(relative) here since all Page.* options are
handled, and C++ wouldn't require it for the same reason.

https://codereview.chromium.org/2811993004/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_extensions/navigation_helper.js:112: 
On 2017/04/18 22:03:12, michaelpg wrote:
> remove line

Whoops, done.

[michaelpg, 2017-05-01 19:19:59]

Sorry, had a busier time than usual as CrOS gardener last week. Getting to this
today.

[michaelpg, 2017-05-01 23:41:26]

I like where this is. I have some suggestions/minor fixes, but it's nicely put
together.

https://codereview.chromium.org/2811993004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_extensions/manager.js (right):

https://codereview.chromium.org/2811993004/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_extensions/manager.js:343: assert(newPage.id);
throughout this whole function: can you assert on data instead of newPage.id,
since data is what needs to be defined? that way the newPage.id detail doesn't
have to keep popping up. Just my opinion.

also you could do fancy things like:

  this.detailViewItem_ = assert(data);

etc. to condense the code a bit.

https://codereview.chromium.org/2811993004/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_extensions/manager.js:351: var entry;
optional: extract this block into a standalone function

https://codereview.chromium.org/2811993004/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_extensions/manager.js:355: assert(newPage.id);
duplicate assertion?

https://codereview.chromium.org/2811993004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_extensions/navigation_helper.js (right):

https://codereview.chromium.org/2811993004/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_extensions/navigation_helper.js:7: * Note: This must
remain in sync with the order in manager.html!
with the page ids

https://codereview.chromium.org/2811993004/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_extensions/navigation_helper.js:23: id:
(string|undefined),
You probably have this problem all throughout extensions, but "id" is ambiguous,
and since the neon-animated-pages uses "id" (which is Page here), maybe this
would be clearer as extensionID?

https://codereview.chromium.org/2811993004/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_extensions/navigation_helper.js:54: if
(location.search == '' && location.pathname == '/')
i'd be in favor of removing this, as it's already the base case

https://codereview.chromium.org/2811993004/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_extensions/navigation_helper.js:79: var relative;
absolute? :-\

https://codereview.chromium.org/2811993004/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_extensions/navigation_helper.js:96: relative =
'/?errors=' + entry.id;
add a break; so adding another case doesn't cause unintentional fallthrough

unless our style guides say not to do that.... I hate having five style guides
to refer to for chrome JS.

https://codereview.chromium.org/2811993004/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/extensions/cr_extensions_browsertest.js (right):

https://codereview.chromium.org/2811993004/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/extensions/cr_extensions_browsertest.js:347: // Extension
Page State Tests
navigation helper

https://codereview.chromium.org/2811993004/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/extensions/extension_navigation_helper_test.js
(right):

https://codereview.chromium.org/2811993004/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/extensions/extension_navigation_helper_test.js:56:
waitForPop.then(() => {
I think returning this Promise will let you eliminate the done() calls.

https://codereview.chromium.org/2811993004/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/extensions/extension_navigation_helper_test.js:60:
waitForPop = getOnPopState();
setting a variable in a function passed as an argument to a function called on
that variable gives me the chills...

https://codereview.chromium.org/2811993004/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/extensions/extension_navigation_helper_test.js:100: var
currentPage = navigationHelper.getCurrentPage();
unused? also, mixing var/let?

[Devlin, 2017-05-02 01:04:07]

https://codereview.chromium.org/2811993004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_extensions/manager.js (right):

https://codereview.chromium.org/2811993004/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_extensions/manager.js:343: assert(newPage.id);
On 2017/05/01 23:41:25, michaelpg wrote:
> throughout this whole function: can you assert on data instead of newPage.id,
> since data is what needs to be defined? that way the newPage.id detail doesn't
> have to keep popping up. Just my opinion.
> 
> also you could do fancy things like:
> 
>   this.detailViewItem_ = assert(data);
> 
> etc. to condense the code a bit.

Done.

https://codereview.chromium.org/2811993004/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_extensions/manager.js:351: var entry;
On 2017/05/01 23:41:25, michaelpg wrote:
> optional: extract this block into a standalone function

I think that since it's only used here, relies on the same inputs, and has
either multiple outputs or mutates the state of this, it's better being inlined.
 So if you don't feel strongly, I'll keep it like that.

https://codereview.chromium.org/2811993004/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_extensions/manager.js:355: assert(newPage.id);
On 2017/05/01 23:41:25, michaelpg wrote:
> duplicate assertion?

Removed.

https://codereview.chromium.org/2811993004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_extensions/navigation_helper.js (right):

https://codereview.chromium.org/2811993004/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_extensions/navigation_helper.js:7: * Note: This must
remain in sync with the order in manager.html!
On 2017/05/01 23:41:25, michaelpg wrote:
> with the page ids

Done.

https://codereview.chromium.org/2811993004/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_extensions/navigation_helper.js:23: id:
(string|undefined),
On 2017/05/01 23:41:25, michaelpg wrote:
> You probably have this problem all throughout extensions, but "id" is
ambiguous,
> and since the neon-animated-pages uses "id" (which is Page here), maybe this
> would be clearer as extensionID?

done.

https://codereview.chromium.org/2811993004/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_extensions/navigation_helper.js:54: if
(location.search == '' && location.pathname == '/')
On 2017/05/01 23:41:25, michaelpg wrote:
> i'd be in favor of removing this, as it's already the base case

Done.

https://codereview.chromium.org/2811993004/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_extensions/navigation_helper.js:79: var relative;
On 2017/05/01 23:41:25, michaelpg wrote:
> absolute? :-\

s/relative/path

https://codereview.chromium.org/2811993004/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_extensions/navigation_helper.js:96: relative =
'/?errors=' + entry.id;
On 2017/05/01 23:41:25, michaelpg wrote:
> add a break; so adding another case doesn't cause unintentional fallthrough
> 
> unless our style guides say not to do that.... I hate having five style guides
> to refer to for chrome JS.

Done.

https://codereview.chromium.org/2811993004/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/extensions/cr_extensions_browsertest.js (right):

https://codereview.chromium.org/2811993004/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/extensions/cr_extensions_browsertest.js:347: // Extension
Page State Tests
On 2017/05/01 23:41:26, michaelpg wrote:
> navigation helper

Done.

https://codereview.chromium.org/2811993004/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/extensions/extension_navigation_helper_test.js
(right):

https://codereview.chromium.org/2811993004/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/extensions/extension_navigation_helper_test.js:56:
waitForPop.then(() => {
On 2017/05/01 23:41:26, michaelpg wrote:
> I think returning this Promise will let you eliminate the done() calls.

Done.

https://codereview.chromium.org/2811993004/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/extensions/extension_navigation_helper_test.js:60:
waitForPop = getOnPopState();
On 2017/05/01 23:41:26, michaelpg wrote:
> setting a variable in a function passed as an argument to a function called on
> that variable gives me the chills...

waitFor*Next*Pop? :)

https://codereview.chromium.org/2811993004/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/extensions/extension_navigation_helper_test.js:100: var
currentPage = navigationHelper.getCurrentPage();
On 2017/05/01 23:41:26, michaelpg wrote:
> unused? also, mixing var/let?

Removed.

[michaelpg, 2017-05-02 20:58:10]

nice, couple cleanups then lgtm

https://codereview.chromium.org/2811993004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_extensions/manager.js (right):

https://codereview.chromium.org/2811993004/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_extensions/manager.js:351: var entry;
On 2017/05/02 01:04:06, Devlin wrote:
> On 2017/05/01 23:41:25, michaelpg wrote:
> > optional: extract this block into a standalone function
> 
> I think that since it's only used here, relies on the same inputs, and has
> either multiple outputs or mutates the state of this, it's better being
inlined.
>  So if you don't feel strongly, I'll keep it like that.

Acknowledged.

https://codereview.chromium.org/2811993004/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/extensions/extension_navigation_helper_test.js
(right):

https://codereview.chromium.org/2811993004/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/extensions/extension_navigation_helper_test.js:60:
waitForNextPop = getOnPopState();
missing "var"

https://codereview.chromium.org/2811993004/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/extensions/extension_navigation_helper_test.js:64:
console.warn('VERIFYING MOCK');
yes, I promise it runs :-P

[Devlin, 2017-05-03 21:42:19]

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

[commit-bot: I haz the power, 2017-05-03 21:42:49]

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, 2017-05-03 23:47:50]

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

[commit-bot: I haz the power, 2017-05-03 23:47:51]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-05-04 00:15:55]

https://codereview.chromium.org/2811993004/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/extensions/extension_navigation_helper_test.js
(right):

https://codereview.chromium.org/2811993004/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/extensions/extension_navigation_helper_test.js:60:
waitForNextPop = getOnPopState();
On 2017/05/02 20:58:10, michaelpg wrote:
> missing "var"

Done.

https://codereview.chromium.org/2811993004/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/extensions/extension_navigation_helper_test.js:64:
console.warn('VERIFYING MOCK');
On 2017/05/02 20:58:10, michaelpg wrote:
> yes, I promise it runs :-P

Grr... I wonder if there's a good enough heuristic that we could somehow have a
presubmit warning...

"Hey, it looks like you have debug logs!  Clean them up."

[Devlin, 2017-05-04 00:16:00]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2017-05-04 00:16:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2811993004/#ps100001
(title: "nits")

[commit-bot: I haz the power, 2017-05-04 00:16:41]

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-05-04 00:25:02]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493856960688740,
"parent_rev": "1d11530a771298ce260f024b118864f66cdace4c", "commit_rev":
"8904a23861a4ec7ef5f8851a55d68c198c3ce829"}

[commit-bot: I haz the power, 2017-05-04 00:25:51]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493856960688740,
"parent_rev": "1d11530a771298ce260f024b118864f66cdace4c", "commit_rev":
"8904a23861a4ec7ef5f8851a55d68c198c3ce829"}

[commit-bot: I haz the power, 2017-05-04 00:25:56]

Description was changed from

==========
[MD Extensions] Add support for URL navigation

The chrome://extensions page uses different urls for a few things,
primarily to highlight a given extension or to display the extension's
options. In the MD version, we have separate pages for these, which
makes these more like navigations.

Add basic page and history manipulation to the chrome://extensions page.
Navigating to a subpage updates the location and pushes a new state to
the history stack, so that clicking back will return you to the previous
page. Dialogs are contained within a page, and do not push a new
history state.

Examples:
- Navigating from the main view to the details view updates the
  location, and 'back' would return to the main view.
- Opening the options dialog from the details view does not push new state
  (since otherwise 'back' would just close the dialog).

Add tests for the same.

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

to

==========
[MD Extensions] Add support for URL navigation

The chrome://extensions page uses different urls for a few things,
primarily to highlight a given extension or to display the extension's
options. In the MD version, we have separate pages for these, which
makes these more like navigations.

Add basic page and history manipulation to the chrome://extensions page.
Navigating to a subpage updates the location and pushes a new state to
the history stack, so that clicking back will return you to the previous
page. Dialogs are contained within a page, and do not push a new
history state.

Examples:
- Navigating from the main view to the details view updates the
  location, and 'back' would return to the main view.
- Opening the options dialog from the details view does not push new state
  (since otherwise 'back' would just close the dialog).

Add tests for the same.

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

Review-Url: https://codereview.chromium.org/2811993004
Cr-Commit-Position: refs/heads/master@{#469216}
Committed:
https://chromium.googlesource.com/chromium/src/+/8904a23861a4ec7ef5f8851a55d6...
==========

[commit-bot: I haz the power, 2017-05-04 00:25:58]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/8904a23861a4ec7ef5f8851a55d6...

[wOxxOm, 2017-09-10 19:09:16]

@rdevlin, will the documented [1] URL chrome://extensions/configureCommands to
invoke the keyboard shortcuts dialog be used when MD-extensions page is enabled
by default?

I'm assuming a different URL is used temporarily because otherwise it would
cause navigation conflicts internally. The thing is, if a different URL goes
into production, the extensions that open this dialog (which is effectively a
buried treasure even on the new MD page) will have to check userAgent for build
number or do other weird things like chrome.webNavigation.getAllFrames which
produces a different number of frames for classic/MD page.

  1: https://developer.chrome.com/extensions/commands