Don't fire popstate event on initial document load

Don't fire popstate event on initial document load

According to the specification [1], a popstate event should only be fired
when the document has a "last entry" and the entry being navigated to isn't
it.  A document is created without a "last entry" and gets one just after
this check when it is first navigated to, so a popstate should be fired any
time a document is navigated to except for the first time after it has been
created.

[1] http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#traverse-the-history (step 12-14 in particular)

BUG=63040
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=165221

[Jens Widell, 2014-01-13 10:54:33]

Please have a look when possible.

Minimal code change, and a bunch of existing test cases could have (what
essentially looked like) workarounds for the fixed issue removed.

[arv (Not doing code reviews), 2014-01-13 14:26:43]

LGTM

https://codereview.chromium.org/136463002/diff/1/LayoutTests/fast/loader/stat...
File
LayoutTests/fast/loader/stateobjects/resources/replacestate-in-iframe-window-child.html
(right):

https://codereview.chromium.org/136463002/diff/1/LayoutTests/fast/loader/stat...
LayoutTests/fast/loader/stateobjects/resources/replacestate-in-iframe-window-child.html:2:
onload = function() {
window.onload = function() {

for clarity

https://codereview.chromium.org/136463002/diff/1/LayoutTests/fast/loader/stat...
LayoutTests/fast/loader/stateobjects/resources/replacestate-in-iframe-window-child.html:6:
onunload = function() {
do you mind changing this too?

[Jens Widell, 2014-01-13 14:57:04]

Many thanks for quick review!

https://codereview.chromium.org/136463002/diff/1/LayoutTests/fast/loader/stat...
File
LayoutTests/fast/loader/stateobjects/resources/replacestate-in-iframe-window-child.html
(right):

https://codereview.chromium.org/136463002/diff/1/LayoutTests/fast/loader/stat...
LayoutTests/fast/loader/stateobjects/resources/replacestate-in-iframe-window-child.html:2:
onload = function() {
On 2014/01/13 14:26:44, arv wrote:
> window.onload = function() {
> 
> for clarity

Prefix added here and below.

[commit-bot: I haz the power, 2014-01-13 14:57:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jl@opera.com/136463002/50001

[Jens Widell, 2014-01-13 16:02:49]

On 2014/01/13 14:26:43, arv wrote:
> LGTM

I had somehow missed some tests that needed adjusting.  Do you want to have look
at those new changes?

[arv (Not doing code reviews), 2014-01-13 16:06:33]

Still LGTM

[commit-bot: I haz the power, 2014-01-13 16:09:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jl@opera.com/136463002/190001

[commit-bot: I haz the power, 2014-01-13 18:45:27]

Retried try job too often on linux_blink for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[Jens Widell, 2014-01-14 11:02:05]

On 2014/01/13 18:45:27, I haz the power (commit-bot) wrote:
> Retried try job too often on linux_blink for step(s) webkit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

Ah, this is interesting.  My change triggers a (seemingly) debug-only crash in
the test fast/loader/stateobjects/pushstate-in-iframe.html, which indeed listens
to popstate events and thus would be affected by the patch.

However, that test is missing the type of "early popstate" workaround that I've
removed from lots of other tests here, so would be expected to start testing
what it was meant to test with my patch.  What's more, if I add an "early
popstate" workaround to that test, it starts crashing in a build without my fix.

[commit-bot: I haz the power, 2014-01-15 10:43:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jl@opera.com/136463002/350001

[commit-bot: I haz the power, 2014-01-15 17:30:49]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[eseidel, 2014-01-15 17:33:20]

Woh.  This is scary.  Are we sure that things like History.js still work?  I
remember it being a gigantic PITA to get pushState/popState working
cross-browser when I last tried to use it.  Changing any of these quirks scares
me. :)

That said, we definitely should bring ourselves closer to specs, rather than
further.

(Also, I trust Arv implicitly to understand this all waaay better than I.)

[Jens Widell, 2014-01-15 17:42:56]

On 2014/01/15 17:33:20, eseidel wrote:
> Woh.  This is scary.  Are we sure that things like History.js still work?  I
> remember it being a gigantic PITA to get pushState/popState working
> cross-browser when I last tried to use it.  Changing any of these quirks
scares
> me. :)

I seem to have enough trouble getting all the webkit tests to all pass on all
platforms.  :-)

I will do some History.js testing before queuing this again.

[eseidel, 2014-01-15 17:56:39]

In the end, we should strive to match the spec/other-browsers perfectly and
eventually libraries/apps will adapt to expect spec-compliant behavior on our
part.  My comments are simply out of fear that we might end up in an
intermediate state whereby we've broken existing webkit-dependent code and yet
are not fully spec compliant.

Again, I trust that Arv and you both know much more about this than I do!

[balupton, 2014-01-15 18:11:44]

On 2014/01/15 17:56:39, eseidel wrote:
> In the end, we should strive to match the spec/other-browsers perfectly and
> eventually libraries/apps will adapt to expect spec-compliant behavior on our
> part.  My comments are simply out of fear that we might end up in an
> intermediate state whereby we've broken existing webkit-dependent code and yet
> are not fully spec compliant.
> 
> Again, I trust that Arv and you both know much more about this than I do!

I did this up a few years ago when developing History.js, perhaps it is still
relevant:
https://github.com/browserstate/history.js/wiki/The-State-of-the-HTML5-Histor...

[Jens Widell, 2014-01-16 08:16:19]

https://codereview.chromium.org/136463002/diff/350001/LayoutTests/fast/loader...
File
LayoutTests/fast/loader/stateobjects/resources/replacestate-in-iframe-window-child.html
(right):

https://codereview.chromium.org/136463002/diff/350001/LayoutTests/fast/loader...
LayoutTests/fast/loader/stateobjects/resources/replacestate-in-iframe-window-child.html:3:
top.opener.windowLoaded();
Since the window is navigated to a different top-level document and back, this
onload handler is called twice, and thus incorrectly "starts" the test again in
the middle.  Should have copied the "sessionStorage.beganTest" mechanism from
the old onpopstate instead of just this call.

This test case bug appears to be involved in the Windows-only crash found by the
trybots, but doesn't (of course) explain why we'd crash.

[commit-bot: I haz the power, 2014-01-16 12:41:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jl@opera.com/136463002/570001

[commit-bot: I haz the power, 2014-01-16 13:53:36]

Change committed as 165221