Bug 68278 - http/tests/history/back-with-fragment-change.php fails
Summary: http/tests/history/back-with-fragment-change.php fails
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mihai Parparita
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-09-16 13:31 PDT by Mihai Parparita
Modified: 2013-05-22 15:44 PDT (History)
14 users (show)

See Also:


Attachments
Patch (10.22 KB, patch)
2011-09-16 15:56 PDT, Mihai Parparita
no flags Details | Formatted Diff | Diff
Patch (10.22 KB, patch)
2011-09-16 16:56 PDT, Mihai Parparita
darin: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mihai Parparita 2011-09-16 13:31:44 PDT
Test landed in http://trac.webkit.org/changeset/95259. See bug 64556 for history of this issue.
Comment 1 Mihai Parparita 2011-09-16 13:44:08 PDT
Test was skipped with http://trac.webkit.org/changeset/95322.
Comment 2 Mihai Parparita 2011-09-16 15:56:08 PDT
Created attachment 107739 [details]
Patch
Comment 3 Adam Barth 2011-09-16 16:05:53 PDT
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?
Comment 4 Mihai Parparita 2011-09-16 16:15:45 PDT
(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.
Comment 5 Mihai Parparita 2011-09-16 16:56:25 PDT
Created attachment 107747 [details]
Patch
Comment 6 Adam Barth 2011-09-16 17:02:14 PDT
This history stuff is somewhat mysterious to me.  Probably fishd should review as our history expert.
Comment 7 WebKit Review Bot 2011-09-17 17:56:17 PDT
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
Comment 8 David Kilzer (:ddkilzer) 2011-11-28 08:00:59 PST
<rdar://problem/10490675>
Comment 9 Eric Seidel (no email) 2012-05-21 15:10:37 PDT
I think you borked cr-linux?  This is clearly an Adam/Nate review.
Comment 10 Darin Adler 2012-08-14 16:17:24 PDT
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.
Comment 11 Simon Fraser (smfr) 2012-09-19 13:55:26 PDT
The test still fails.
Comment 12 Ádám Kallai 2013-02-14 06:38:49 PST
This test fails on Qt too. Is Anybody working on this bug?
Comment 13 Stephen Chenney 2013-04-09 05:43:54 PDT
There is nobody from Chromium working on this.
Comment 14 Ryosuke Niwa 2013-05-22 15:44:07 PDT
What happened to this patch?