Options: maintain history entries on the parent frame.

Options: maintain history entries on the parent frame.

Currently, navigations within the child frame remain attached to the child
frame while it signals the parent to change path and title. This doesn't quite
work because location.replace and history.replaceState affect all back/forward
list entries which reference that frame's history entry, rather than just the
current one.

Chrome is currently inconsistent about which of the two behaviors it
implements. Fix the settings page to not rely on the bug so it is future-proof.
Instead, when embedded, all pushState and replaceState calls are proxied up to
the parent which manages history for the child. The child converts all
pushState calls to replaceState calls locally.

This adds uber.setTitle, uber.replaceState, and uber.pushState calls along with
a popState message to uber_utils.js. When running standalone, they call the
standard equivalents. When running within the uber page, they delegate to the
parent as appropriate.

In doing so, this fixes places where chrome://settings-frame and
chrome://settings do not set the same URL and title the same. Notably on the
search page and treating the default "settings" page as having a path of ""
rather than "settings".

BUG=317614
TEST=Go to chrome://settings. Open 'Manage search engines...'. Press back.
     Press refresh. Should stay on search page.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271538

[davidben, 2014-05-19 22:38:33]

I still need to write tests for the new uber page history management, but it
seems to work in manual testing. And, in theory, the behavior of
chrome://settings-iframe should actually match that of the uber page now, so the
options_browsertest tests are more useful now.

https://codereview.chromium.org/298553002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/content_settings.js (left):

https://codereview.chromium.org/298553002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/content_settings.js:50: {title:
loadTimeData.getString(hash + 'TabTitle')});
The setTitle call is replaced by the ContentSettingsExceptionArea updating
this.title as appropriate; right now the title isn't updated correctly if you
navigate to chrome://settings/contentExceptions#images directly.

https://codereview.chromium.org/298553002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/options_page.js (left):

https://codereview.chromium.org/298553002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/options_page.js:233:
this.setTitle_(page.title);
This is replaced by an explicit updateTitle_ call. Just so it's unified between
the updateHistory and non-updateHistory cases.

https://codereview.chromium.org/298553002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/options_page.js:248: // Override this with the
new page.
This special-case is replaced with the new replaceState parameter to
showDefaultPage. The old one relied on the child not getting the 'settings' ->
'' special-case while the parent did.

https://codereview.chromium.org/298553002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/options_page.js:285:
this.setTitle_(overlay.title);
This is replaced by the updateTitle_ call later. It needs to happen after
updateHistoryState_ (if it happens) otherwise the previous history item gets the
wrong title.

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/uber/uber.js (right):

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/resources...
chrome/browser/resources/uber/uber.js:30: var queuedInvokes = {};
This may not be strictly necessary? The only page that cares about state (the
settings page) makes all the state redundant with the URL. So we don't actually
need to follow "load" with a simulated "popstate". But this does match how Blink
does it. The queuing is kinda icky but there's a race otherwise. (Why didn't we
define it to work that way to begin with? I guess it's not clear whether you
want to signal the current page or the provisional load, but still...)

[Dan Beam, 2014-05-19 23:06:48]

generally this looks good

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/options_page.js (right):

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/options_page.js:79:
this.navigateToPage(this.getDefaultPage().name,
nit: just pass along the optional param in both cases and let showPageByName()
do the `opt_param || {}` voodoo e.g.:

  this.navigateToPage(this.getDefaultPage().name, opt_propertyBag);

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/uber/uber.js (right):

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/resources...
chrome/browser/resources/uber/uber.js:114: }
nit: no curlies

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/resources...
chrome/browser/resources/uber/uber.js:255: * @param {string} path The new
"path".
@param {string} path The new "path" (e.g. "/createProfile").

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/resources...
chrome/browser/resources/uber/uber.js:261: replace ?
HISTORY_STATE_OPTION.REPLACE : HISTORY_STATE_OPTION.PUSH;
nit: 4 \s indent

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/resources...
chrome/browser/resources/uber/uber.js:265: }
nit: no curlies

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/resources...
chrome/browser/resources/uber/uber.js:313: delete queuedInvokes[container.id];
arguably this should be

  queuedInvokes[container.id] = undefined;

because we're not supposed to use `delete` in chrome code.

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/resources...
chrome/browser/resources/uber/uber.js:356: var container = $(pageId);
nit: $ -> getRequiredElement

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/uber/uber_utils.js (right):

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/resources...
chrome/browser/resources/uber/uber_utils.js:97: window.dispatchEvent(new
PopStateEvent('popstate', {state: state}));
why do we simulate a popstate event here?

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/resources...
chrome/browser/resources/uber/uber_utils.js:101: * Returns true if this frame
has a parent.
@return {boolean} Whether this frame has parent.

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/resources...
chrome/browser/resources/uber/uber_utils.js:148: window.history.pushState;
indent 4\s instead of 2\s

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/options_browsertest.js (left):

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/options_browsertest.js:497:
this.verifyOpenPages_(['settings', 'search'], 'settings#query');
i'm generally confused, why s/settings//g ?

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/options_browsertest.js (right):

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/options_browsertest.js:390: verifyOpenPages_:
function(expectedPages, expectedUrl) {
nit: opt_expectedUrl

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/options_browsertest.js:404: expectedUrl =
(expectedUrl == undefined) ? lastExpected : expectedUrl;
can L395-404 be:

  expectedUrl = expectedUrl || expectedPages.slice(-1)[0];

?

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/options_browsertest.js:406:
assertEquals(fullExpectedUrl, window.location.href);
i think these should be expect*() rather than assert*()

[davidben, 2014-05-19 23:32:28]

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/options_page.js (right):

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/options_page.js:79:
this.navigateToPage(this.getDefaultPage().name,
On 2014/05/19 23:06:49, Dan Beam wrote:
> nit: just pass along the optional param in both cases and let showPageByName()
> do the `opt_param || {}` voodoo e.g.:
> 
>   this.navigateToPage(this.getDefaultPage().name, opt_propertyBag);

Done.

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/uber/uber.js (right):

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/resources...
chrome/browser/resources/uber/uber.js:114: }
On 2014/05/19 23:06:49, Dan Beam wrote:
> nit: no curlies

Done.

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/resources...
chrome/browser/resources/uber/uber.js:255: * @param {string} path The new
"path".
On 2014/05/19 23:06:49, Dan Beam wrote:
> @param {string} path The new "path" (e.g. "/createProfile").

Done.

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/resources...
chrome/browser/resources/uber/uber.js:261: replace ?
HISTORY_STATE_OPTION.REPLACE : HISTORY_STATE_OPTION.PUSH;
On 2014/05/19 23:06:49, Dan Beam wrote:
> nit: 4 \s indent

Done.

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/resources...
chrome/browser/resources/uber/uber.js:265: }
On 2014/05/19 23:06:49, Dan Beam wrote:
> nit: no curlies

Done.

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/resources...
chrome/browser/resources/uber/uber.js:313: delete queuedInvokes[container.id];
On 2014/05/19 23:06:49, Dan Beam wrote:
> arguably this should be
> 
>   queuedInvokes[container.id] = undefined;
> 
> because we're not supposed to use `delete` in chrome code.

Done.

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/resources...
chrome/browser/resources/uber/uber.js:356: var container = $(pageId);
On 2014/05/19 23:06:49, Dan Beam wrote:
> nit: $ -> getRequiredElement

Done.

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/uber/uber_utils.js (right):

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/resources...
chrome/browser/resources/uber/uber_utils.js:97: window.dispatchEvent(new
PopStateEvent('popstate', {state: state}));
On 2014/05/19 23:06:49, Dan Beam wrote:
> why do we simulate a popstate event here?

history.pushState and history.replaceState don't trigger popstate events, so we
have to simulate one.

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/resources...
chrome/browser/resources/uber/uber_utils.js:101: * Returns true if this frame
has a parent.
On 2014/05/19 23:06:49, Dan Beam wrote:
> @return {boolean} Whether this frame has parent.

Done.

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/resources...
chrome/browser/resources/uber/uber_utils.js:148: window.history.pushState;
On 2014/05/19 23:06:49, Dan Beam wrote:
> indent 4\s instead of 2\s

Done.

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/options_browsertest.js (left):

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/options_browsertest.js:497:
this.verifyOpenPages_(['settings', 'search'], 'settings#query');
On 2014/05/19 23:06:49, Dan Beam wrote:
> i'm generally confused, why s/settings//g ?

The settings OptionsPage is kind of weird in that, although page.name is
'settings', it appears in the top-level URL as empty. I.e., the URL is
chrome://settings/ and not chrome://settings/settings. This used to happen as a
special case in updateHistoryState_ where the inner path had the extra
"settings", but the outer path didn't; note how chrome://settings-frame/
rewrites to chrome://settings-frame/settings today, but chrome://settings
doesn't rewrite to chrome://settings/settings. This is the page ==
this.getDefaultPage() check.

Since the inner/outer syncing is now done within uber.pushState, that
special-case is gone. Probably not a bad plan anyway since this means that these
tests actually assert on URLs that (assuming uber_utils.js works correctly) are
analogous to what end up in the address bar.

(settings -> search is because that's another discrepancy between the two
frames. search is the version the outer frame sees, so the inner frame now gets
that too.)

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/options_browsertest.js (right):

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/options_browsertest.js:390: verifyOpenPages_:
function(expectedPages, expectedUrl) {
On 2014/05/19 23:06:49, Dan Beam wrote:
> nit: opt_expectedUrl

Done.

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/options_browsertest.js:404: expectedUrl =
(expectedUrl == undefined) ? lastExpected : expectedUrl;
On 2014/05/19 23:06:49, Dan Beam wrote:
> can L395-404 be:
> 
>   expectedUrl = expectedUrl || expectedPages.slice(-1)[0];
> 
> ?

Problem is the empty string is falsey.

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/options_browsertest.js:406:
assertEquals(fullExpectedUrl, window.location.href);
On 2014/05/19 23:06:49, Dan Beam wrote:
> i think these should be expect*() rather than assert*()

...that would explain why they failed by hanging rather aborting. Done.

[Dan Beam, 2014-05-19 23:49:09]

lgtm, we'll see how this works (*fingers crossed*)

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/uber/uber.js (right):

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/resources...
chrome/browser/resources/uber/uber.js:356: var container = $(pageId);
On 2014/05/19 23:32:28, David Benjamin wrote:
> On 2014/05/19 23:06:49, Dan Beam wrote:
> > nit: $ -> getRequiredElement
> 
> Done.

I think you did this on L325 instead of L354

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/options_browsertest.js (right):

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/options_browsertest.js:404: expectedUrl =
(expectedUrl == undefined) ? lastExpected : expectedUrl;
On 2014/05/19 23:32:28, David Benjamin wrote:
> On 2014/05/19 23:06:49, Dan Beam wrote:
> > can L395-404 be:
> > 
> >   expectedUrl = expectedUrl || expectedPages.slice(-1)[0];
> > 
> > ?
> 
> Problem is the empty string is falsey.

yeah, ok, can you use `=== undefined` or `typeof expectedUrl == 'undefined'`
here instead?

https://codereview.chromium.org/298553002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/uber/uber_utils.js (right):

https://codereview.chromium.org/298553002/diff/80001/chrome/browser/resources...
chrome/browser/resources/uber/uber_utils.js:101: * Returns true if this frame
has a parent.
^ and then remove this (seems duplicative)

https://codereview.chromium.org/298553002/diff/80001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/options_browsertest.js (right):

https://codereview.chromium.org/298553002/diff/80001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/options_browsertest.js:405: (opt_expectedUrl ==
undefined) ? lastExpected : opt_expectedUrl;
nit: rather than re-use this opt_ variable here, do something like:

  var blah = typeof opt_blah != 'undefined' ? blah : default;
  // the reader of this code is now sure `blah` is defined

[davidben, 2014-05-20 00:12:54]

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/uber/uber.js (right):

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/resources...
chrome/browser/resources/uber/uber.js:356: var container = $(pageId);
On 2014/05/19 23:49:10, Dan Beam wrote:
> On 2014/05/19 23:32:28, David Benjamin wrote:
> > On 2014/05/19 23:06:49, Dan Beam wrote:
> > > nit: $ -> getRequiredElement
> > 
> > Done.
> 
> I think you did this on L325 instead of L354

Oops, so I did. Done.

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/options_browsertest.js (right):

https://codereview.chromium.org/298553002/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/options_browsertest.js:404: expectedUrl =
(expectedUrl == undefined) ? lastExpected : expectedUrl;
On 2014/05/19 23:49:10, Dan Beam wrote:
> On 2014/05/19 23:32:28, David Benjamin wrote:
> > On 2014/05/19 23:06:49, Dan Beam wrote:
> > > can L395-404 be:
> > > 
> > >   expectedUrl = expectedUrl || expectedPages.slice(-1)[0];
> > > 
> > > ?
> > 
> > Problem is the empty string is falsey.
> 
> yeah, ok, can you use `=== undefined` or `typeof expectedUrl == 'undefined'`
> here instead?

Done.

https://codereview.chromium.org/298553002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/uber/uber_utils.js (right):

https://codereview.chromium.org/298553002/diff/80001/chrome/browser/resources...
chrome/browser/resources/uber/uber_utils.js:101: * Returns true if this frame
has a parent.
On 2014/05/19 23:49:10, Dan Beam wrote:
> ^ and then remove this (seems duplicative)

Done.

https://codereview.chromium.org/298553002/diff/80001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/options_browsertest.js (right):

https://codereview.chromium.org/298553002/diff/80001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/options_browsertest.js:405: (opt_expectedUrl ==
undefined) ? lastExpected : opt_expectedUrl;
On 2014/05/19 23:49:10, Dan Beam wrote:
> nit: rather than re-use this opt_ variable here, do something like:
> 
>   var blah = typeof opt_blah != 'undefined' ? blah : default;
>   // the reader of this code is now sure `blah` is defined

Done.

[davidben, 2014-05-20 00:40:09]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-05-20 00:40:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/davidben@chromium.org/298553002/140001

[commit-bot: I haz the power, 2014-05-20 02:22:55]

Change committed as 271538