Restore scroll position on page reloads scheduled by <meta http-equiv="refresh" content="XX"/>
Created attachment 96796 [details] Patch
This should make http://build.webkit.org/console substantially less painful to use in any WebKit browser.
Comment on attachment 96796 [details] Patch This needs a layout test.
Created attachment 96856 [details] Patch
Does the test pass in both Firefox and IE?
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.
The test passes in both Firefox and Opera, but not IE8. I don't have any later version of IE here.
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?
(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 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?
(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.
(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.
Created attachment 99151 [details] Patch
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 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
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
(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".
(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.
> 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 on attachment 99151 [details] Patch R- because this breaks a test.
Created attachment 99554 [details] Patch
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 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.
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.
> 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 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 on attachment 99554 [details] Patch Yep. Looks like it.
(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.
Can't we strip off the fragments before making the comparison?
Created attachment 99574 [details] Patch
(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 on attachment 99574 [details] Patch Clearing flags on attachment: 99574 Committed r90342: <http://trac.webkit.org/changeset/90342>
All reviewed patches have been landed. Closing bug.
Apparently these tests don't pass on Mac, GTK, and Win7.
(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 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).
Zalan you should have a look at this
Created attachment 99578 [details] Patch
(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.
(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?
(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 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.
(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?
> 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 on attachment 99578 [details] Patch Clearing flags on attachment: 99578 Committed r90346: <http://trac.webkit.org/changeset/90346>
> 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.
(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.