Bug 62482 - Restore scroll position on page reloads scheduled by <meta http-equiv="refresh" content="XX"/>
Summary: Restore scroll position on page reloads scheduled by <meta http-equiv="refres...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Robert Hogan
URL:
Keywords:
Depends on: 63875
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-10 14:57 PDT by Robert Hogan
Modified: 2011-07-03 15:34 PDT (History)
12 users (show)

See Also:


Attachments
Patch (4.31 KB, patch)
2011-06-10 15:02 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (6.99 KB, patch)
2011-06-11 08:48 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (16.57 KB, patch)
2011-06-29 14:30 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (1.46 MB, application/zip)
2011-06-29 15:03 PDT, WebKit Review Bot
no flags Details
Patch (17.08 KB, patch)
2011-07-02 10:01 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (13.23 KB, patch)
2011-07-03 06:09 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (13.21 KB, patch)
2011-07-03 12:31 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Hogan 2011-06-10 14:57:56 PDT
Restore scroll position on page reloads scheduled by <meta http-equiv="refresh" content="XX"/>
Comment 1 Robert Hogan 2011-06-10 15:02:09 PDT
Created attachment 96796 [details]
Patch
Comment 2 Robert Hogan 2011-06-10 15:03:58 PDT
This should make http://build.webkit.org/console substantially less painful to use in any WebKit browser.
Comment 3 Mihai Parparita 2011-06-10 15:17:21 PDT
Comment on attachment 96796 [details]
Patch

This needs a layout test.
Comment 4 Robert Hogan 2011-06-11 08:48:24 PDT
Created attachment 96856 [details]
Patch
Comment 5 Alexey Proskuryakov 2011-06-11 09:23:52 PDT
Does the test pass in both Firefox and IE?
Comment 6 Robert Hogan 2011-06-11 09:53:25 PDT
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.
Comment 7 Robert Hogan 2011-06-12 05:03:52 PDT
The test passes in both Firefox and Opera, but not IE8. I don't have any later version of IE here.
Comment 8 Yong Li 2011-06-24 09:00:54 PDT
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?
Comment 9 Robert Hogan 2011-06-25 03:06:22 PDT
(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().
Comment 10 Adam Barth 2011-06-25 11:10:57 PDT
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?
Comment 11 Robert Hogan 2011-06-27 11:13:59 PDT
(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.
Comment 12 Adam Barth 2011-06-27 11:18:52 PDT
(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.
Comment 13 Robert Hogan 2011-06-29 14:30:29 PDT
Created attachment 99151 [details]
Patch
Comment 14 Adam Barth 2011-06-29 14:37:20 PDT
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?
Comment 15 WebKit Review Bot 2011-06-29 15:03:31 PDT
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
Comment 16 WebKit Review Bot 2011-06-29 15:03:36 PDT
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
Comment 17 Robert Hogan 2011-06-29 16:00:10 PDT
(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".
Comment 18 Robert Hogan 2011-06-29 16:09:04 PDT
(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.
Comment 19 Adam Barth 2011-06-29 16:10:58 PDT
> 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.
Comment 20 Adam Barth 2011-06-29 17:16:43 PDT
Comment on attachment 99151 [details]
Patch

R- because this breaks a test.
Comment 21 Robert Hogan 2011-07-02 10:01:31 PDT
Created attachment 99554 [details]
Patch
Comment 22 Adam Barth 2011-07-02 11:38:24 PDT
Comment on attachment 99554 [details]
Patch

This approach looks very nice.  Thanks for iterating on the patch.  I'll check in detail later today.
Comment 23 Adam Barth 2011-07-02 12:05:29 PDT
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.
Comment 24 Darin Adler 2011-07-02 12:09:20 PDT
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.
Comment 25 Adam Barth 2011-07-02 12:13:06 PDT
> 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.
Comment 26 Alexey Proskuryakov 2011-07-02 12:22:07 PDT
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?
Comment 27 Adam Barth 2011-07-02 13:20:11 PDT
Comment on attachment 99554 [details]
Patch

Yep.  Looks like it.
Comment 28 Robert Hogan 2011-07-03 03:22:45 PDT
(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.
Comment 29 Adam Barth 2011-07-03 03:25:24 PDT
Can't we strip off the fragments before making the comparison?
Comment 30 Robert Hogan 2011-07-03 06:09:28 PDT
Created attachment 99574 [details]
Patch
Comment 31 Robert Hogan 2011-07-03 06:10:23 PDT
(In reply to comment #29)
> Can't we strip off the fragments before making the comparison?

Ah, there's a convenience function for that!
Comment 32 WebKit Review Bot 2011-07-03 10:28:05 PDT
Comment on attachment 99574 [details]
Patch

Clearing flags on attachment: 99574

Committed r90342: <http://trac.webkit.org/changeset/90342>
Comment 33 WebKit Review Bot 2011-07-03 10:28:13 PDT
All reviewed patches have been landed.  Closing bug.
Comment 34 Adam Barth 2011-07-03 11:38:46 PDT
Apparently these tests don't pass on Mac, GTK, and Win7.
Comment 35 Kenneth Rohde Christiansen 2011-07-03 12:22:41 PDT
(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.
Comment 36 Kenneth Rohde Christiansen 2011-07-03 12:24:54 PDT
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).
Comment 37 Kenneth Rohde Christiansen 2011-07-03 12:25:23 PDT
Zalan you should have a look at this
Comment 38 Robert Hogan 2011-07-03 12:31:51 PDT
Created attachment 99578 [details]
Patch
Comment 39 Robert Hogan 2011-07-03 12:33:56 PDT
(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.
Comment 40 Kenneth Rohde Christiansen 2011-07-03 12:40:00 PDT
(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?
Comment 41 Robert Hogan 2011-07-03 12:42:32 PDT
(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.
Comment 42 Adam Barth 2011-07-03 12:51:43 PDT
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.
Comment 43 Robert Hogan 2011-07-03 12:52:47 PDT
(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?
Comment 44 Antonio Gomes 2011-07-03 13:01:10 PDT

> 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'?
Comment 45 WebKit Review Bot 2011-07-03 13:34:27 PDT
Comment on attachment 99578 [details]
Patch

Clearing flags on attachment: 99578

Committed r90346: <http://trac.webkit.org/changeset/90346>
Comment 46 WebKit Review Bot 2011-07-03 13:34:37 PDT
All reviewed patches have been landed.  Closing bug.
Comment 47 Kenneth Rohde Christiansen 2011-07-03 15:32:13 PDT
> 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.
Comment 48 Kenneth Rohde Christiansen 2011-07-03 15:34:22 PDT
(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.