Bug 116670

Summary:

Fix loader/go-back-cached-main-resource.html test

Product:

WebKit

Reporter:

Manuel Rego Casasnovas <rego>

Component:

Tools / Tests

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, cgarcia, commit-queue, darin

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none

Description Manuel Rego Casasnovas 2013-05-23 07:19:16 PDT

This bug is to fix the issue explained in comment the following comment about loader/go-back-cached-main-resource.html layout test:
https://bugs.webkit.org/show_bug.cgi?id=112418#c29

Comment by "Alexey Proskuryakov":
> (From update of attachment 197215 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=197215&action=review
> 
> > LayoutTests/loader/go-back-cached-main-resource.html:36
> > +                target.postMessage('navigate-back', '*');
> > +                // Wait a bit for resource load callbacks
> > +                setTimeout(function () {
> 
> It is expected that the page does not get an onload when being restored from page cache, so a 'first-page' message is not sent.
> 
> A better way to check for page being restored from page cache is to handle pageshow event on it.

Comment 1 Manuel Rego Casasnovas 2013-05-23 07:25:08 PDT

Created attachment 202708 [details]
Patch

Comment 2 Alexey Proskuryakov 2013-05-23 10:29:29 PDT

Comment on attachment 202708 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=202708&action=review

Getting rid of the timeout is a very welcome improvement, so r=me. Please consider the suggestion below.

> LayoutTests/loader/resources/first-page.html:21
> +<body onpageshow="registerListeners();">

This is quite misleading. We only need to register listeners on first load, they are already present when restoring from cache. But the function does more than its name says, which is why it's necessary to call it every time!

One way to clean this up would be to rename the function to "didShowPage", and use event.persisted property to check if we are handling the initial load. Or just use an anonymous function, and register it as window.onpageshow = function (event) { ... }

Comment 3 WebKit Commit Bot 2013-05-23 10:52:34 PDT

Comment on attachment 202708 [details]
Patch

Clearing flags on attachment: 202708

Committed r150594: <http://trac.webkit.org/changeset/150594>

Comment 4 WebKit Commit Bot 2013-05-23 10:52:36 PDT

All reviewed patches have been landed.  Closing bug.

Comment 5 Manuel Rego Casasnovas 2013-05-24 03:42:36 PDT

(In reply to comment #2)
> (From update of attachment 202708 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202708&action=review
> 
> Getting rid of the timeout is a very welcome improvement, so r=me. Please consider the suggestion below.

Thanks for the review.

> > LayoutTests/loader/resources/first-page.html:21
> > +<body onpageshow="registerListeners();">
> 
> This is quite misleading. We only need to register listeners on first load, they are already present when restoring from cache. But the function does more than its name says, which is why it's necessary to call it every time!
> 
> One way to clean this up would be to rename the function to "didShowPage", and use event.persisted property to check if we are handling the initial load. Or just use an anonymous function, and register it as window.onpageshow = function (event) { ... }

Thanks for the information, I've changed the test in a new bug #116709 in order to register listeners only on initial load.