Bug 14147 – REGRESSION: No history item created when viewing "All Sizes" on Flickr

• Component: History
• Status: RESOLVED DUPLICATE

[Matt Lilek]

Going back from the 'All Sizes' page of a photo on Flickr no longer works.

1. In Safari, open a new tab and load http://www.flickr.com/photos/n1c0star/538230787/
2. Click the "All Sizes" button above the photo.
3. The back button is disabled and no history item created.

Bug is present in Safari 3 Beta and r23521. Note that this is not an issue with Safari.app as Safari.app v2.0.4 with current ToT displays this bug.

[Mark Rowe (bdash)]

<rdar://problem/5270940>

[Maxime BRITTO]

Created attachment 15137 [details]
proposed patch

The call for scheduleLocationChange() in JSHTMLDocument::setLocation()  (JSHTMLDocumentCustom.cpp) was missing an argument (userGesture) and the default behavior in scheduleLocationChange() was to consider it false.
As this JS event was not considered as userGesture initiated, his load type was internal and it wasn't added to the history.
With this modification I can no longer reproduce the bug

[Maxime BRITTO]

Layout test is on his way. I'll send it within the next hours.

[Maxime BRITTO]

Created attachment 15159 [details]
patch with layout test

Here is the patch with the layout test included.

I noticed that this patch is also fixing the Bug 13849.

[Geoffrey Garen]

Comment on attachment 15159 [details]
patch with layout test

r=me

[Andrew Wellington]

*** Bug 13849 has been marked as a duplicate of this bug. ***

[Maxime BRITTO]

Created attachment 15184 [details]
patch with correct layout test 

The layout test wasn't correctly working due to a race condition. Now it's works as it should.

[Andrew Wellington]

Comment on attachment 15159 [details]
patch with layout test

Setting r- so it doesn't show up in commit queue.

[Mark Rowe (bdash)]

Comment on attachment 15184 [details]
patch with correct layout test 

I went to land this patch but noticed that the test case it adds causes an assertion failure to be hit.

$ run-webkit-tests LayoutTests/http/tests/navigation/javascript-document-location-changed.html LayoutTests/http/tests/navigation/javascript-document-location-changed.html
[...]
http/tests/navigation ..Assertion failed: ([bfList currentItem] != prevTestBFItem), function dumpBackForwardListForWebView, file /Volumes/Data/Home/Documents/Work/OpenSource/WebKitTools/DumpRenderTree/DumpRenderTree.m, line 768.

I hit this while regenerating the test results after removing the tab characters, fixing spelling, and changing the test to dump as text.

[Maxime BRITTO]

Created attachment 15243 [details]
proposed patch

I can't reproduce the assertion failure you hitted, so I made some changes without being able to test the final result.
I hope this will be good.
And I'm sorry for the bad spelling : I'm french and I'm trying very hard to create correct sentences, I hope I will improve my writing within the next 4 months.

[Matt Lilek]

(In reply to comment #10)
> Created an attachment (id=15243) [edit]
> proposed patch
> 
> I can't reproduce the assertion failure you hitted, so I made some changes
> without being able to test the final result.
> I hope this will be good.
> And I'm sorry for the bad spelling : I'm french and I'm trying very hard to
> create correct sentences, I hope I will improve my writing within the next 4
> months.
> 

I'm hitting the same assertion failure in your latest patch.  Are you sure you're using a debug build?

[Geoffrey Garen]

Comment on attachment 15243 [details]
proposed patch

r- due to assertion failures.

[Maxime BRITTO]

Created attachment 15271 [details]
proposed patch

I changed the whole test to be sure to avoid the assertion failure.
I think it's for the best because the test is now faster and more simple.

[Mark Rowe (bdash)]

I no longer see the assertion failure.  The question I have is why was the assertion failing, and why does it not fail with your new test?

[Maxime BRITTO]

(In reply to comment #14)
> I no longer see the assertion failure.  The question I have is why was the
> assertion failing, and why does it not fail with your new test?
> 
I think it's because the test was based on a simulated clic to go to the next page instead of LayoutTestController.queueLoad(). So when I came back to the first page, the LayoutTestController.dumpBackForwardList() was called again.
In the new test I clear the backForwardList on the first page and, when I'm on the second page, instead of going back to the first page, I'm analysing the history to see if it's there.

[Maciej Stachowiak]

Comment on attachment 15271 [details]
proposed patch

r=me

[Mark Rowe (bdash)]

Comment on attachment 15271 [details]
proposed patch

It seems that JSHTMLDocument::setLocation has been changed since you created the patch, so the patch no longer applies cleanly.  It's not obvious to me how to reconcile the changes so it would be great if you could do so and submit an updated patch.

[Maxime BRITTO]

This bug is now fixed by the patch of Bug 13400 which is a duplicate of this one

[Maxime BRITTO]

*** This bug has been marked as a duplicate of 13400 ***