Bug 84041 – Reloading substitute-data/alternate html string for unreachableURL will add an item to the back-forward-history for each reload

• Status: RESOLVED
• Resolution: FIXED
• Component: History

[Tor Arne Vestbø]

Calling loadAlternateHTMLString on the page proxy repeatedly, or an initial loadAlternateHTMLString and then reload(), will add multiple entries for the substitute-data in the back-forward history, which does not match the behavior of a successful load and subsequent reload.

Reproducible in Safari/WebKit nightly by going to http://www.foobarbaz.com/ and clicking reload multiple times.

[Tor Arne Vestbø]

After checking Safari 5 on 10.5.8, and it also creates a new bf-item for each reload, so perhaps this is intended behavior?

[Vicki Pfau]

Created attachment 154154 [details]
Patch

[Vicki Pfau]

Created attachment 154171 [details]
Patch

The old patch was applied to an out of date version and therefore didn't apply cleanly to ToT. This is fixed now.

[WebKit Review Bot]

Attachment 154171 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/loader/FrameLoader.h:106:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brady Eidson]

Comment on attachment 154171 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154171&action=review

Please fix the ChangeLog and Stylebot's complaint before landing.

> Source/WebCore/ChangeLog:13
> +
> +        Added a TestWebKitAPI test.
> +

I think we normally mention which API test was added by filename.

[Vicki Pfau]

(In reply to comment #5)
> (From update of attachment 154171 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154171&action=review
> 
> Please fix the ChangeLog and Stylebot's complaint before landing.
> 
> > Source/WebCore/ChangeLog:13
> > +
> > +        Added a TestWebKitAPI test.
> > +
> 
> I think we normally mention which API test was added by filename.

I tried to figure out what was making the style bot complain about that line and couldn't figure it out. I also looked for other examples in the changelog for API tests, but couldn't find any.

I can give it another look, though.

[Brady Eidson]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 154171 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=154171&action=review
> > 
> > Please fix the ChangeLog and Stylebot's complaint before landing.
> > 
> > > Source/WebCore/ChangeLog:13
> > > +
> > > +        Added a TestWebKitAPI test.
> > > +
> > 
> > I think we normally mention which API test was added by filename.
> 
> I tried to figure out what was making the style bot complain about that line and couldn't figure it out. 

That's why stylebot includes style guide citations!

Go to http://www.webkit.org/coding/coding-style.html and look under "Comments"

> I also looked for other examples in the changelog for API tests, but couldn't find any.

Just say something like "Added API test 'mac/BackForwardList.mm'"

[WebKit Review Bot]

Comment on attachment 154171 [details]
Patch

Attachment 154171 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13345035

New failing tests:
http/tests/navigation/success200-loadsame.html
http/tests/navigation/success200-frames-loadsame.html

[WebKit Review Bot]

Created attachment 154190 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Brady Eidson]

Comment on attachment 154171 [details]
Patch

Removing r+ based on layout test failures.

[Tor Arne Vestbø]

Comment on attachment 154171 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154171&action=review

> Source/WebCore/loader/FrameLoader.cpp:-1239
> -    // FIXME: is this the right place to reset loadType? Perhaps this should be done after loading is finished or aborted.
> -    m_loadType = FrameLoadTypeStandard;

Is removing this FIXME correct? We're still reseting the loadtype up front, just inside loadWithDocumentLoader() instead?

> Source/WebKit/mac/WebView/WebFrame.mm:1403
> +    _private->coreFrame->loader()->load(request, substituteData, false, _private->coreFrame->loader()->loadType());

I'm also wondering if we really need to pass in the actual load type as an argument. Isn't it more of a boolean shouldReset yes/no, where in the latter case we re-use the previous load type? It just seems a bit weird to pass in  "_private->coreFrame->loader()->loadType()", when calling load() on the same instance. If it is in fact a bool relationship, we might even be able to contain the magic of _when_ that is the case inside load(), though I'm not sure how. I assume that's what the original FIXME refers to.

[Vicki Pfau]

Created attachment 154464 [details]
Patch

[Vicki Pfau]

Created attachment 154478 [details]
Patch

[WebKit Review Bot]

Comment on attachment 154478 [details]
Patch

Attachment 154478 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13350429

New failing tests:
http/tests/navigation/slowmetaredirect-basic.html

[WebKit Review Bot]

Created attachment 154505 [details]
Archive of layout-test-results from gce-cr-linux-08

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-08  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Vicki Pfau]

The failing test explicitly says in the comments about the test that the line that's missing in the test output after this patch shouldn't be in the output in the first place. It appears the test expectation might be wrong, and the output after this patch is correct.

[WebKit Review Bot]

Comment on attachment 154478 [details]
Patch

Attachment 154478 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13361252

New failing tests:
http/tests/navigation/slowmetaredirect-basic.html

[WebKit Review Bot]

Created attachment 154519 [details]
Archive of layout-test-results from gce-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Vicki Pfau]

Created attachment 154754 [details]
Patch

Speculative fix for cr-linux EWS problems

[Brady Eidson]

Comment on attachment 154754 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154754&action=review

> Tools/ChangeLog:13
> +        * TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
> +        * TestWebKitAPI/Tests/mac/BackForwardList.mm: Added.
> +        (-[BackForwardListTest webView:didFinishLoadForFrame:]):
> +        (-[BackForwardListTest webView:didFailProvisionalLoadWithError:forFrame:]):
> +        (TestWebKitAPI):
> +        (TestWebKitAPI::TEST):

BackForwardList.mm doesn't appear to be included in this diff...?

[Vicki Pfau]

Created attachment 154756 [details]
Patch

Add missing file

[Vicki Pfau]

Committed r123823: <http://trac.webkit.org/changeset/123823>