RESOLVED FIXED 28659
We could use a stale frame size when going back to a cached page 
https://bugs.webkit.org/show_bug.cgi?id=28659
Summary We could use a stale frame size when going back to a cached page
Ada Chan
Reported 2009-08-22 13:32:13 PDT
I tried the following steps on Safari, Windows Vista, r47676: 1. Make the browser window small. Go to http://en.wikipedia.org/wiki/Main_Page. 2. Click on a link on that page. 3. Resize the window to be a lot bigger. 4. Press "Back" RESULT: the wikipedia front page is formatted in the old smaller size, even though the window is bigger now. EXPECTED RESULT: the wikipedia front page should be formatted according to the current window size.
Attachments
Patch (1.23 KB, patch)
2009-08-22 14:09 PDT, Ada Chan 		no flags
DetailsFormatted DiffDiff
Updated patch - added null check for m_frame->view() (1.13 KB, patch)
2009-08-22 14:42 PDT, Ada Chan 		sfalken: review+
DetailsFormatted DiffDiff
Add layout test (3.23 KB, patch)
2009-08-24 11:19 PDT, Ada Chan 		beidson: review+
DetailsFormatted DiffDiff
Skip test for qt and gtk (1.41 KB, patch)
2009-08-24 14:02 PDT, Ada Chan 		mrowe: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Ada Chan
Comment 1 2009-08-22 13:54:38 PDT
<rdar://problem/5735948> The problem here is when we go back to a cached page, we reuse the cached frame's FrameView which has the old frame rect. This problem does not happen on the Mac because its FrameView is backed by a platform widget (NSView) and it can calculate the layout width/height based on the widget. But the FrameView is not backed by a platform widget on Windows.
Ada Chan
Comment 2 2009-08-22 14:09:17 PDT
Created attachment 38438 [details] Patch
Ada Chan
Comment 3 2009-08-22 14:42:55 PDT
Created attachment 38439 [details] Updated patch - added null check for m_frame->view()
Sam Weinig
Comment 4 2009-08-22 15:38:56 PDT
Test case? I believe we can now test b/f cache bugs.
Eric Seidel (no email)
Comment 5 2009-08-23 00:05:18 PDT
http://trac.webkit.org/browser/trunk/LayoutTests/fast/harness has some examples.
Ada Chan
Comment 6 2009-08-24 11:19:14 PDT
Created attachment 38488 [details] Add layout test Since this test uses some significantly big timeouts, it's not placed under the "fast" folder. Once the pageshow event has been implemented for b/f cache and we use that instead rather than relying on the big timer in this test, we can move this test to under "fast".
Ada Chan
Comment 7 2009-08-24 11:23:30 PDT
Updated the comments about the navigation steps to: // Navigation steps: // 1- loads this page // 2- resizes the window to (300, 300) // 3- loads a data URL that resizes the window to (1000, 1000) and navigates back // 4- loads this page which restores the timer to check the window width to make sure it's > 300
Brady Eidson
Comment 8 2009-08-24 12:00:53 PDT
Comment on attachment 38488 [details] Add layout test > Index: LayoutTests/loader/go-back-to-different-window-size.html > =================================================================== > --- LayoutTests/loader/go-back-to-different-window-size.html (revision 0) > +++ LayoutTests/loader/go-back-to-different-window-size.html (revision 0) > @@ -0,0 +1,44 @@ > +<html> > +<script> > + > +// Navigation steps: > +// 1- loads this page > +// 2- resizes the window to (300, 300) > +// 2- loads this page + hash > +// 3- loads a data URL that resizes the window to (1000, 1000) and navigates back > +// 4- loads this page + hash and checks the window width to make sure it's > 300 I think this comment is out of date, and refers to an earlier version of the patch. > + // We don't need to depend on this long timer when pageshow event is implemented for b/f cache (<rdar://problem/6440869>). Could you file a bug to update this test once we've implemented pageshow, and block that bug on the implementing pageshow bug? > + window.setTimeout("verifyWindowSizeAfterNavigateBackToCachedPage()", 1000); As discussed in person, we have a bug with timers when restoring pages from the page cache and this timer fires instantly instead of waiting the full second. We need a bug on this, to at least explore other browsers' behavior and see what we should match. These comments accounted for, r=me
Ada Chan
Comment 9 2009-08-24 13:25:49 PDT
Fix checked in: r47722. Test checked in: r47723. Filed a bug on how restored timers from cached page fires instantly instead of waiting the specified delay first: https://bugs.webkit.org/show_bug.cgi?id=28683 Filed a bug about changing loader/go-back-to-different-window-size.html to use the pageshow event once it's implemented: <rdar://problem/7165517>
Ada Chan
Comment 10 2009-08-24 14:02:32 PDT
Created attachment 38499 [details] Skip test for qt and gtk Skip the test for qt and gtk due to missing DRT ability to override 'standard' preferences
Eric Seidel (no email)
Comment 11 2009-08-24 14:31:18 PDT
Comment on attachment 38438 [details] Patch Looks like this was landed. Clearing r=?
Ada Chan
Comment 12 2009-08-24 14:38:10 PDT
Yes, the fix was landed. Thanks for clearing the review flag!
Note You need to log in before you can comment on or make changes to this bug.