Test landed in http://trac.webkit.org/changeset/95259. See bug 64556 for history of this issue.
Test was skipped with http://trac.webkit.org/changeset/95322.
Created attachment 107739 [details] Patch
Comment on attachment 107739 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=107739&action=review > Source/WebCore/loader/FrameLoader.cpp:2301 > +// In cases where a load is not committed, undoes the speculative updating of > +// the current history item that's done by HistoryController::goToItem. > +void FrameLoader::undoSpeculativeBackForwardListUpdate() Shouldn't this be a method of HistoryController since it interacts almost exclusively with HistoryController state?
(In reply to comment #3) > Shouldn't this be a method of HistoryController since it interacts almost exclusively with HistoryController state? Makes sense. Updated patch coming up.
Created attachment 107747 [details] Patch
This history stuff is somewhat mysterious to me. Probably fishd should review as our history expert.
Comment on attachment 107747 [details] Patch Attachment 107747 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9736381 New failing tests: http/tests/navigation/forward-and-cancel.html
<rdar://problem/10490675>
I think you borked cr-linux? This is clearly an Adam/Nate review.
Comment on attachment 107747 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=107747&action=review > Source/WebCore/ChangeLog:11 > + navigated-to history item was cloberred by the same-document navigation. typo: cloberred should be clobbered > Source/WebCore/loader/HistoryController.cpp:549 > +void HistoryController::updateForProvisionalLoadStopped() Should this function also set m_provisionalItem to 0? If so, you could remove that call from the one call site that does it separately and explicitly. > Source/WebCore/loader/HistoryController.cpp:555 > + bool isTargetItem = m_provisionalItem ? m_provisionalItem->isTargetItem() : false; You just moved this line of code, but I suggest writing it with && instead of the ternary operator. bool isTargetItem = m_provisionalItem && m_provisionalItem->isTargetItem(); > Source/WebCore/loader/HistoryController.cpp:558 > + if (HistoryItem* resetItem = mainFrame->loader()->history()->currentItem()) { I find the variable name “reset item” to be a bit confusing. It sounds like a verb phrase to me. > Source/WebCore/loader/HistoryController.h:73 > + void updateForProvisionalLoadStopped(); A better name for this would be updateForStoppedProvisionalLoad. For the record, I also think the function above should be renamed updateForCompletedFrameLoad. I think I understand why someone gave that a different name before, but it’s unnecessarily non-grammatical naming.
The test still fails.
This test fails on Qt too. Is Anybody working on this bug?
There is nobody from Chromium working on this.
What happened to this patch?
This test is marked as failing, not flaky, in TestExpectations but it’s not failing for me.
There are a lot of failures (both flaky and consistent) on https://results.webkit.org/?suite=layout-tests&test=http%2Ftests%2Fhistory%2Fback-with-fragment-change.php
So we should mark this as flaky, not as known failure.
Marked test as failing for mac-wk1, flaky for mac-wk2 in http://trac.webkit.org/r266758.
Created attachment 452118 [details] expectations
Comment on attachment 452118 [details] expectations Clearing flags on attachment: 452118 Committed r289871 (?): <https://commits.webkit.org/r289871>
Re-added test expectations because they had been inadvertently removed.
http/tests/history/back-with-fragment-change.py https://results.webkit.org/?suite=layout-tests&test=http%2Ftests%2Fhistory%2Fback-with-fragment-change.py Mac WK1 is constantly failing, GTK and WPE WK2 is randomly failing. https://build.webkit.org/results/GTK-Linux-64-bit-Debug-Tests/270007@main%20(11606)/http/tests/history/back-with-fragment-change-diff.txt --- /home/buildbot/worker/GTK-Linux-64-bit-Debug-Tests/build/layout-test-results/http/tests/history/back-with-fragment-change-expected.txt +++ /home/buildbot/worker/GTK-Linux-64-bit-Debug-Tests/build/layout-test-results/http/tests/history/back-with-fragment-change-actual.txt @@ -4,6 +4,5 @@ ============== Back Forward List ============== http://127.0.0.1:8000/history/back-with-fragment-change.py **nav target** - http://127.0.0.1:8000/history/resources/back-with-fragment-change-target.html **nav target** curr-> http://127.0.0.1:8000/history/resources/back-with-fragment-change-target.html#foo **nav target** ===============================================
I think this test case is not compliant with the specification: https://html.spec.whatwg.org/multipage/browsing-the-web.html#centralized-modifications-of-session-historyn (the first example describes exactly the same case as is in this testcase). In my opinion (but please correct me if I am wrong) that the result of this test case should be: ============== Back Forward List ============== curr-> http://127.0.0.1:8000/history/back-with-fragment-change.py **nav target** - http://127.0.0.1:8000/history/resources/back-with-fragment-change-target.html **nav target** http://127.0.0.1:8000/history/resources/back-with-fragment-change-target.html#foo **nav target** =============================================== Chrome and FireFox work in such way (at the end the back-with-fragment-change.py is displayed and due to deleting the sessionStorage.didNavigate; in function "done" of the back-with-fragment-change-target.html, the test case restarts).