Summary: | history.pushState doesn't work for the first page in a window. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jędrzej Nowacki <jedrzej.nowacki> | ||||||||
Component: | History | Assignee: | Jędrzej Nowacki <jedrzej.nowacki> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ap, beidson, commit-queue, darin, fishd, jedrzej.nowacki, tonikitoo | ||||||||
Priority: | P1 | Keywords: | Regression | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Jędrzej Nowacki
2010-05-20 04:26:20 PDT
Created attachment 56585 [details] Fix v1 Apparently it fix the bug 38754 too at least on my Linux machine. Comment on attachment 56585 [details] Fix v1 Clearing flags on attachment: 56585 Committed r59933: <http://trac.webkit.org/changeset/59933> All reviewed patches have been landed. Closing bug. This would've been easy to add a layouttest for, wouldn't it have? (In reply to comment #4) > This would've been easy to add a layouttest for, wouldn't it have? It will be a bit redundant, but easy :-) Created attachment 57020 [details]
layout test v1
New test, after load event, calls the pushState a few times and check if it change the history length property.
(In reply to comment #5) > (In reply to comment #4) > > This would've been easy to add a layouttest for, wouldn't it have? > It will be a bit redundant, but easy :-) I don't understand this comment. There was a bug - you couldn't call pushState after the initial load in a new window. If a layouttest was catching this case, then that layout test should've been failing. If a layouttest was NOT catching this case, then adding one would not be redundant. Comment on attachment 57020 [details]
layout test v1
I'm r-'ing because I'm missing how this test actually works under the run-webkit-tests harness.
Yes, if I open it in a new Safari window, for example, it would be exercising the code path that used to have this problem.
But run-webkit-tests reuses the same WebView for a series of tests. Therefore - unless it goes first - this test won't actually excercise the buggy code path.
There is a fairly simple invariant for layouttests. They need to fail before the WebKit code change is applied, and pass after the WebKit code change is applied. Unless I'm missing something, I don't see how this test would fail before r59933 was applied. Note that by failure, I mean in the context of run-webkit-tests. There is a way to get DRT to run the test in a fresh WebView *even* in the context of run-webkit-tests, and that is to run the test in a new window. Created attachment 57091 [details] layout test v2 (In reply to comment #8) > (From update of attachment 57020 [details]) > (...) But run-webkit-tests reuses the same WebView for a series of tests. Therefore - unless it goes first - this test won't actually excercise the buggy code path. Wow, I didn't know, it is evil :-). (In reply to comment #7) > If a layouttest was catching this case, then that layout test should've been failing. > If a layouttest was NOT catching this case, then adding one would not be redundant. I thought that bug was exposed after some other changes. For me document-destroyed-navigate-back-with-fragment-scroll.html is failing without the patch, but only when it is run as a first test, I haven't noticed ordering issue. It is why I wrote that a new test would be a bit redundant. (In reply to comment #9) > (...) There is a way to get DRT to run the test in a fresh WebView *even* in the context of run-webkit-tests, and that is to run the test in a new window. New patch use this trick. Shouldn't we fix DRT? The bug wasn't caught only because of this "feature", for example document-destroyed-navigate-back-with-fragment-scroll.html covers similar use case. Running all the tests in the same WebView isn't a "trick", it's just how the tests are run, and it’s closer to the normal model of web browsing. Making a new WebView every time would both be slower and would hide other kinds of bugs. Most tests aren’t sensitively dependent on whether the WebView is new. Comment on attachment 57091 [details]
layout test v2
As Darin mentioned, this isn't a bug in DRT, but a conscious decision. The type of test that relies on having a fresh WebView is actually quite the rare exception.
Thanks for adding this test.
Thank you for explaination and review. Comment on attachment 57091 [details] layout test v2 Clearing flags on attachment: 57091 Committed r60295: <http://trac.webkit.org/changeset/60295> All reviewed patches have been landed. Closing bug. |