Bug 28659 - We could use a stale frame size when going back to a cached page
Summary: We could use a stale frame size when going back to a cached page
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows Vista
: P2 Normal
Assignee: Ada Chan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2009-08-22 13:32 PDT by Ada Chan
Modified: 2009-08-24 14:38 PDT (History)
2 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Ada Chan 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.
Comment 1 Ada Chan 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.
Comment 2 Ada Chan 2009-08-22 14:09:17 PDT
Created attachment 38438 [details]
Patch
Comment 3 Ada Chan 2009-08-22 14:42:55 PDT
Created attachment 38439 [details]
Updated patch - added null check for m_frame->view()
Comment 4 Sam Weinig 2009-08-22 15:38:56 PDT
Test case?  I believe we can now test b/f cache bugs.
Comment 5 Eric Seidel (no email) 2009-08-23 00:05:18 PDT
http://trac.webkit.org/browser/trunk/LayoutTests/fast/harness has some examples.
Comment 6 Ada Chan 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".
Comment 7 Ada Chan 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
Comment 8 Brady Eidson 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
Comment 9 Ada Chan 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>
Comment 10 Ada Chan 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
Comment 11 Eric Seidel (no email) 2009-08-24 14:31:18 PDT
Comment on attachment 38438 [details]
Patch

Looks like this was landed.  Clearing r=?
Comment 12 Ada Chan 2009-08-24 14:38:10 PDT
Yes, the fix was landed.  Thanks for clearing the review flag!