Bug 62482 – Restore scroll position on page reloads scheduled by <meta http-equiv="refresh" content="XX"/>

• Component: New Bugs
• Status: RESOLVED FIXED

[Robert Hogan]

Restore scroll position on page reloads scheduled by <meta http-equiv="refresh" content="XX"/>

[Robert Hogan]

Created attachment 96796 [details]
Patch

[Robert Hogan]

This should make http://build.webkit.org/console substantially less painful to use in any WebKit browser.

[Mihai Parparita]

Comment on attachment 96796 [details]
Patch

This needs a layout test.

[Robert Hogan]

Created attachment 96856 [details]
Patch

[Alexey Proskuryakov]

Does the test pass in both Firefox and IE?

[Robert Hogan]

It passes in firefox, I don't have IE available. If you have IE, build.webkit.org/console is a good manual test -though you have to wait a minute for the page to refresh.

[Robert Hogan]

The test passes in both Firefox and Opera, but not IE8. I don't have any later version of IE here.

[Yong Li]

Comment on attachment 96856 [details]
Patch

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

> Source/WebCore/loader/NavigationScheduler.cpp:321
> +{
> +    if (!shouldScheduleNavigation(url))
> +        return;
> +    if (delay < 0 || delay > INT_MAX / 1000)
> +        return;
> +    if (url.isEmpty())
> +        return;
> +
> +    // We want a new back/forward list item if the refresh timeout is > 1 second.
> +    if (!m_redirect || delay <= m_redirect->delay())
> +        schedule(adoptPtr(new ScheduledReload(delay, m_frame->document()->securityOrigin(), url, true, delay <= 1)));

shouldScheduleNavigation(url) better to be called as last step just before scheduling?

[Robert Hogan]

(In reply to comment #8)
> 
> shouldScheduleNavigation(url) better to be called as last step just before scheduling?

Possibly, but this function is just copying NavigationScheduler::scheduleRedirect().

[Adam Barth]

Comment on attachment 96856 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:2543
> +            if (url == m_url.string())

Does this account properly for fragment identifiers?

> Source/WebCore/loader/NavigationScheduler.h:82
> +    void scheduleReload(double delay, const String& url);

How is scheduleReload different from scheduleRefresh?  Why does scheduleReload take a url parameter?  Surely a reload is just going to use the same URL that the document already has, right?

[Robert Hogan]

(In reply to comment #10)
> (From update of attachment 96856 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96856&action=review
> 
> > Source/WebCore/dom/Document.cpp:2543
> > +            if (url == m_url.string())
> 
> Does this account properly for fragment identifiers?

I'll test for it and see.

> 
> > Source/WebCore/loader/NavigationScheduler.h:82
> > +    void scheduleReload(double delay, const String& url);
> 
> How is scheduleReload different from scheduleRefresh?  

The former calls loader()->reload(), which sets FrameLoadTypeReload for the load, which causes the user's scroll position to get honoured when loading is finished.

> Why does scheduleReload take a url parameter?  Surely a reload is just going to use the same URL that the document already has, right?

Right, it doesn't need to.

[Adam Barth]

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 96856 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=96856&action=review
> > 
> > > Source/WebCore/loader/NavigationScheduler.h:82
> > > +    void scheduleReload(double delay, const String& url);
> > 
> > How is scheduleReload different from scheduleRefresh?  
> 
> The former calls loader()->reload(), which sets FrameLoadTypeReload for the load, which causes the user's scroll position to get honoured when loading is finished.

That's super unclear from the names of these functions.  "Reload" and "Refresh" are synonymous concepts on the web.  These functions should either be combined or renamed so that it's crystal clear what's different about them.

[Robert Hogan]

Created attachment 99151 [details]
Patch

[Adam Barth]

Comment on attachment 99151 [details]
Patch

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

> Source/WebCore/loader/NavigationScheduler.h:80
> +    void scheduleLocationReload();

What is a LocationReload?  How is that different than a Reload?

[WebKit Review Bot]

Comment on attachment 99151 [details]
Patch

Attachment 99151 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8959364

New failing tests:
http/tests/misc/refresh-headers.php

[WebKit Review Bot]

Created attachment 99157 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Robert Hogan]

(In reply to comment #14)
> (From update of attachment 99151 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=99151&action=review
> 
> > Source/WebCore/loader/NavigationScheduler.h:80
> > +    void scheduleLocationReload();
> 
> What is a LocationReload?  How is that different than a Reload?

For use by location.reload(), it doesn't need to worry about delay timers like a reload() requested by http-equiv="refresh".

[Robert Hogan]

(In reply to comment #15)
> (From update of attachment 99151 [details])
> Attachment 99151 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/8959364
> 
> New failing tests:
> http/tests/misc/refresh-headers.php

This is due to the change for location.reload(). FrameLoader::reload() has:

    // FIXME: We don't have a mechanism to revalidate the main resource without reloading at the moment.
    request.setCachePolicy(ReloadIgnoringCacheData);

It looks like I'll have to revert that part and keep separate ScheduledURLNavigation sub-classes for the loads.

[Adam Barth]

> For use by location.reload(), it doesn't need to worry about delay timers like a reload() requested by http-equiv="refresh".

Why is location.reload different than http-equiv="refresh" with a timeout of zero?  If difference is in cachine behavior or scrolling behavior, these should be parameter to the reload method.

[Adam Barth]

Comment on attachment 99151 [details]
Patch

R- because this breaks a test.

[Robert Hogan]

Created attachment 99554 [details]
Patch

[Adam Barth]

Comment on attachment 99554 [details]
Patch

This approach looks very nice.  Thanks for iterating on the patch.  I'll check in detail later today.

[Adam Barth]

Comment on attachment 99554 [details]
Patch

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

> Source/WebCore/loader/NavigationScheduler.cpp:151
> +        , m_refresh(refresh)

Another option is to move this state onto ScheduledURLNavigation and have the other clients pass in false.  I'm not sure whether it matters that much.

[Darin Adler]

As a general rule, names for booleans should be nouns or adjectives, not verbs. Since we don’t want a function named refresh() that returns a flag indicating whether you should refresh, we also don’t want an argument named refresh or a data member named m_refresh.

[Adam Barth]

> As a general rule, names for booleans should be nouns or adjectives, not verbs. Since we don’t want a function named refresh() that returns a flag indicating whether you should refresh, we also don’t want an argument named refresh or a data member named m_refresh.

Definitely agree.  In this patch, specifically, Robert is begin consistent with the name of the parameter he's calling:

http://trac.webkit.org/browser/trunk/Source/WebCore/loader/FrameLoader.h#L110

    void changeLocation(PassRefPtr<SecurityOrigin>, const KURL&, const String& referrer, bool lockHistory = true, bool lockBackForwardList = true, bool refresh = false);

IMHO, we should fix that parameter name, but probably in the patch that renames/unifies the "start navigating now pls" functions on FrameLoader.

[Alexey Proskuryakov]

Comment on attachment 99554 [details]
Patch

Is there actually a need to pass any variable? Can't the same condition be checked at the time navigation occurs?

[Adam Barth]

Comment on attachment 99554 [details]
Patch

Yep.  Looks like it.

[Robert Hogan]

(In reply to comment #27)
> (From update of attachment 99554 [details])
> Yep.  Looks like it.

It doesn't work when dealing with fragment identifiers. This is because the reload for a http-equiv is scheduled on the url of the original page (before the user has scrolled to a fragment identifier). By the time ScheduledRedirect fires the frame's url contains the fragment identifier (so does DocumentLoader's originalUrl()), the ScheduledRedirect's url doesn't match the one in the frame anymore, and so it doesn't see it as a refresh.

[Adam Barth]

Can't we strip off the fragments before making the comparison?

[Robert Hogan]

Created attachment 99574 [details]
Patch

[Robert Hogan]

(In reply to comment #29)
> Can't we strip off the fragments before making the comparison?

Ah, there's a convenience function for that!

[WebKit Review Bot]

Comment on attachment 99574 [details]
Patch

Clearing flags on attachment: 99574

Committed r90342: <http://trac.webkit.org/changeset/90342>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

Apparently these tests don't pass on Mac, GTK, and Win7.

[Kenneth Rohde Christiansen]

(In reply to comment #32)
> (From update of attachment 99574 [details])
> Clearing flags on attachment: 99574
> 
> Committed r90342: <http://trac.webkit.org/changeset/90342>

What about restoring pageScale? I believe that Safari in Mac OS X Lion makes use of that.

[Kenneth Rohde Christiansen]

Comment on attachment 99574 [details]
Patch

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

> Source/WebCore/loader/NavigationScheduler.cpp:161
> +        UserGestureIndicator gestureIndicator(wasUserGesture() ? DefinitelyProcessingUserGesture : DefinitelyNotProcessingUserGesture);
> +        bool refresh = equalIgnoringFragmentIdentifier(frame->document()->url(), KURL(ParsedURLString, url()));
> +        frame->loader()->changeLocation(securityOrigin(), KURL(ParsedURLString, url()), referrer(), lockHistory(), lockBackForwardList(), refresh);

I wonder how this is going to work for WebKit2, or for clients doing panning and scaling on the UI side (usually together with async tiling).

[Kenneth Rohde Christiansen]

Zalan you should have a look at this

[Robert Hogan]

Created attachment 99578 [details]
Patch

[Robert Hogan]

(In reply to comment #36)
> 
> I wonder how this is going to work for WebKit2, or for clients doing panning and scaling on the UI side (usually together with async tiling).

It should work exactly as it currently does when they refresh the page manually.

[Kenneth Rohde Christiansen]

(In reply to comment #39)
> (In reply to comment #36)
> > 
> > I wonder how this is going to work for WebKit2, or for clients doing panning and scaling on the UI side (usually together with async tiling).
> 
> It should work exactly as it currently does when they refresh the page manually.

When there are user interactions the page should be suspended, so I guess we do not get the refresh event until it is restored; though I'm not really sure this works currently.

I guess the behavior should be similar for clients not suspending during user interaction, thus you should not do the refresh until after the gesture ends.

Would it be possible to make some tests to verify that the pageScale is restored as well?

[Robert Hogan]

(In reply to comment #38)
> Created an attachment (id=99578) [details]
> Patch

To fix the tests I just scrolled 20 pixels instead of 100. The dimensions used by DRT on Gtk at least meant that there weren't 100 pixels available to scroll. I confirmed the fix works on Gtk.

[Adam Barth]

Comment on attachment 99578 [details]
Patch

Ok.  Let's try this again.  If there are WebKit2 issues, we can work those out in a followup patch.  This patch just re-uses mechanism that already exist.  If those mechanisms are broken, that's a separate issue.

[Robert Hogan]

(In reply to comment #40)
> Would it be possible to make some tests to verify that the pageScale is restored as well?
It should restore it, the scroll position is retrieved from the HistoryItem which contains the page scale factor too. Any pointers on coverage for page scale elsewhere?

[Antonio Gomes]

> When there are user interactions the page should be suspended, so I guess we do not get the refresh event until it is restored; though I'm not really sure this works currently.
> 
> I guess the behavior should be similar for clients not suspending during user interaction, thus you should not do the refresh until after the gesture ends.
> 
> Would it be possible to make some tests to verify that the pageScale is restored as well?

Not sure if I agree here.

Do you mean in a frozen-backing-store state, due to an on-going user interaction should, WebCore should not allow network and page demanded requests like 'refrash'?

[WebKit Review Bot]

Comment on attachment 99578 [details]
Patch

Clearing flags on attachment: 99578

Committed r90346: <http://trac.webkit.org/changeset/90346>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kenneth Rohde Christiansen]

> Not sure if I agree here.
> 
> Do you mean in a frozen-backing-store state, due to an on-going user interaction should, WebCore should not allow network and page demanded requests like 'refrash'?

Nope, they should be deferred till after the user interaction ended. Think about it, it is strange for the user to be pinch zooming etc and have the contents change, maybe radically, underneath.

At least the N9 browser and the iOS browser suspends when there are user interactions, ie. suspends painting, css animations, js executions, defers loads etc.

[Kenneth Rohde Christiansen]

(In reply to comment #47)
> > Not sure if I agree here.
> > 
> > Do you mean in a frozen-backing-store state, due to an on-going user interaction should, WebCore should not allow network and page demanded requests like 'refrash'?
> 
> Nope, they should be deferred till after the user interaction ended. Think about it, it is strange for the user to be pinch zooming etc and have the contents change, maybe radically, underneath.
> 
> At least the N9 browser and the iOS browser suspends when there are user interactions, ie. suspends painting, css animations, js executions, defers loads etc.

This is for making things not change underneath and confuse the user, but also to ensure that we can have user interactions while never dropping below 60fps.