Bug 13422 - REGRESSION: Page reload loses page position
Summary: REGRESSION: Page reload loses page position
Status: VERIFIED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 523.x (Safari 3)
Hardware: Macintosh PowerPC OS X 10.4
: P1 Normal
Assignee: Nobody
URL: any. e.g. http://www.apple.com/airpor...
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2007-04-20 16:08 PDT by Justin
Modified: 2007-08-22 18:12 PDT (History)
4 users (show)

See Also:


Attachments
Proposed patch (1.81 KB, patch)
2007-08-06 01:10 PDT, Cameron Zwarich (cpst)
beidson: review-
Details | Formatted Diff | Diff
Proposed patch with test case (3.07 KB, patch)
2007-08-07 20:28 PDT, Cameron Zwarich (cpst)
beidson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin 2007-04-20 16:08:44 PDT
In the last several nightly builds (a few weeks, I believe), page reload behavior has changed slightly (in my opinion, it has broken).  Reloading the page usually has normally returned the user to the position on the page the user was viewing prior to reloading, but now, the page is reloaded and the very top is displayed.

I did find an open bug (http://bugs.webkit.org/show_bug.cgi?id=10223) related to seemingly sporadic reload behavior scroll position on pages with frames, but that bug is much older than this one, and this one occurs on pages without frames.
Comment 1 Brady Eidson 2007-04-20 16:19:15 PDT
Oh no, this is not good!  Can you regress this using the nightlies to find out when this broke?
Comment 2 Justin 2007-04-20 16:48:53 PDT
Why certainly.  It worked up through revision 17354, and stopped working in 17388.
Comment 3 Brady Eidson 2007-04-20 17:02:04 PDT
Thanks!  That'll probably be a big help.

Unfortunately, I know off the top of my hand thats when major loader refactoring was going on - many large patches completely upsetting the balance.  

I'll take a look at that range
Comment 4 Brady Eidson 2007-04-20 17:08:34 PDT
Yup, my fears confirmed
http://trac.webkit.org/projects/webkit/changeset/17365 seems to be an extremely likely culrpit, though a few others in that range might be as well...

These "moving and renaming" patches often do more than just that (unintentionally of course)
Comment 5 Darin Adler 2007-04-23 08:39:50 PDT
<rdar://problem/5153026>
Comment 6 Cameron Zwarich (cpst) 2007-08-05 02:13:58 PDT
I can confirm that this regression is indeed caused by r17365. I will try to hunt down the exact change that causes it and hopefully come up with a patch.
Comment 7 Cameron Zwarich (cpst) 2007-08-06 01:10:53 PDT
Created attachment 15846 [details]
Proposed patch

Here is a patch that fixes the bug. In revision 17365, many Objective C method bodies in WebFrame were moved to the C++ wrapper class WebFrameLoaderClient for performance reasons. However, in copying the updateHistoryForReload() method, a small error was introduced. In the original method, currItem is made the current history item of the frame:

WebHistoryItem *currItem = _private->currentItem;

In the copied version, this is changed to

WebHistoryItem *currItem = m_webFrame->_private->previousItem;

This patch corrects that change, which was carried over into the new loader code.
Comment 8 Mark Rowe (bdash) 2007-08-06 02:34:42 PDT
Comment on attachment 15846 [details]
Proposed patch

This feels like it should be testable in a layout test.  It'd be handy to have a test around so we can be sure we don't reintroduce this regression in the future.
Comment 9 Brady Eidson 2007-08-07 13:06:12 PDT
Comment on attachment 15846 [details]
Proposed patch

Looks like a good fix, and good catch on the mistake in the patch that caused the original regression.

r- because there is no layout test and this should be layout-testable

We'd be happy to help you write one (#webkit on irc.freenode.net) but you can also refer to the other layout tests for guidance
Comment 10 Cameron Zwarich (cpst) 2007-08-07 20:28:11 PDT
Created attachment 15863 [details]
Proposed patch with test case

Here is the patch with a test case. It turns out that the success200-reload test was designed to scroll the document and record the change, but nobody ever checked if it scrolled in the test output when it was made. I always run the layout tests after adding a fix, so I must have accidentally been running them for the older version of the tree I was using to find the bug.
Comment 11 Brady Eidson 2007-08-08 09:38:02 PDT
Comment on attachment 15863 [details]
Proposed patch with test case

HAH.  Very cool.
Comment 12 Brady Eidson 2007-08-08 09:44:04 PDT
Landed in http://trac.webkit.org/projects/webkit/changeset/24934
Comment 13 Justin 2007-08-09 11:24:08 PDT
Thanks all, I've confirmed that this is working now.  It's not clear to me whether I'm supposed to change the status, but I consider the bug fixed.
Comment 14 Brady Eidson 2007-08-09 11:27:46 PDT
Mark it as verified, since you've VERIFIED it is fixed   =D
Comment 15 Justin 2007-08-09 12:42:02 PDT
Verified :)
Comment 16 Cameron Zwarich (cpst) 2007-08-12 22:36:05 PDT
I should note that this bug was actually noticed the day it was introduced by the person who introduced it:

http://trac.webkit.org/projects/webkit/changeset/17368

We should probably be more careful to check changed test results in the future.
Comment 17 Darin Adler 2007-08-22 18:12:46 PDT
(In reply to comment #16)
> I should note that this bug was actually noticed the day it was introduced by
> the person who introduced it:
> 
> http://trac.webkit.org/projects/webkit/changeset/17368
> 
> We should probably be more careful to check changed test results in the future.

My bad!