Bug 28659

Summary:

We could use a stale frame size when going back to a cached page

Product:

WebKit

Reporter:

Ada Chan <adachan>

Component:

Layout and Rendering

Assignee:

Ada Chan <adachan>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

eric, sam

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

OS:

Windows Vista

Attachments:

Description	Flags
Patch	none
Updated patch - added null check for m_frame->view()	sfalken: review+
Add layout test	beidson: review+
Skip test for qt and gtk	mrowe: review+

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	Details Formatted Diff Diff
Updated patch - added null check for m_frame->view() (1.13 KB, patch) 2009-08-22 14:42 PDT, Ada Chan	sfalken: review+	Details Formatted Diff Diff
Add layout test (3.23 KB, patch) 2009-08-24 11:19 PDT, Ada Chan	beidson: review+	Details Formatted Diff Diff
Skip test for qt and gtk (1.41 KB, patch) 2009-08-24 14:02 PDT, Ada Chan	mrowe: review+	Details Formatted Diff Diff
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.