Bug 151224

Summary: Remove swipe snapshot before main document load if scroll position is already restored
Product: WebKit Reporter: Tim Horton <thorton>
Component: New BugsAssignee: Tim Horton <thorton>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, cdumez, cmarcelo, commit-queue, jamesr, japhet, luiz, sam, simon.fraser, tonikitoo
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch darin: review+

Tim Horton
Reported 2015-11-12 14:24:08 PST
Remove swipe snapshot before main document load if scroll position is already restored
Attachments
Patch (16.76 KB, patch)
2015-11-12 14:24 PST, Tim Horton
no flags
Patch (19.15 KB, patch)
2015-11-12 14:38 PST, Tim Horton
no flags
Patch (19.78 KB, patch)
2015-11-13 10:19 PST, Tim Horton
darin: review+
Tim Horton
Comment 1 2015-11-12 14:24:50 PST
Tim Horton
Comment 2 2015-11-12 14:38:46 PST
Tim Horton
Comment 3 2015-11-13 10:19:16 PST
Darin Adler
Comment 4 2015-11-14 18:50:58 PST
Comment on attachment 265484 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265484&action=review > Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:207 > + WTFLogAlways("requestScrollPositionUpdate %d", scrollPosition.y()); Did you mean to leave this in? > Source/WebKit2/UIProcess/ViewGestureController.cpp:126 > +#if PLATFORM(MAC) > + // On Mac, we check if the scroll position restoration succeeded by comparing the > + // requested and actual scroll position. It's possible that we will never succeed in restoring > + // the exact scroll position we wanted, in the case of a dynamic page, but we know that by > + // main frame load time that we've gotten as close as we're going to get, so stop waiting. > + // We don't want to do this on iOS because scroll position restoration is baked into the transaction. > + m_snapshotRemovalTracker.cancelOutstandingEvent(SnapshotRemovalTracker::ScrollPositionRestoration); > +#endif This mentions Mac and iOS; not sure if other platforms would be more like Mac or more like iOS. > Source/WebKit2/UIProcess/ios/PageClientImplIOS.mm:758 > +{ > + > +} I’d omit the blank line.
Tim Horton
Comment 5 2015-11-14 23:20:52 PST
(In reply to comment #4) > Comment on attachment 265484 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=265484&action=review > > > Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:207 > > + WTFLogAlways("requestScrollPositionUpdate %d", scrollPosition.y()); > > Did you mean to leave this in? Definitely not. > > Source/WebKit2/UIProcess/ViewGestureController.cpp:126 > > +#if PLATFORM(MAC) > > + // On Mac, we check if the scroll position restoration succeeded by comparing the > > + // requested and actual scroll position. It's possible that we will never succeed in restoring > > + // the exact scroll position we wanted, in the case of a dynamic page, but we know that by > > + // main frame load time that we've gotten as close as we're going to get, so stop waiting. > > + // We don't want to do this on iOS because scroll position restoration is baked into the transaction. > > + m_snapshotRemovalTracker.cancelOutstandingEvent(SnapshotRemovalTracker::ScrollPositionRestoration); > > +#endif > > This mentions Mac and iOS; not sure if other platforms would be more like > Mac or more like iOS. You raise a good point. Really, this should be "uses UI-side compositing/transactional scrolling/something", and we'd want the "iOS" behavior if you were using UI-side compositing on Mac. We don't have a fantastic way to tell right now, but we should. I'll come up with something. > > Source/WebKit2/UIProcess/ios/PageClientImplIOS.mm:758 > > +{ > > + > > +} > > I’d omit the blank line.
Tim Horton
Comment 6 2015-12-01 11:36:53 PST
(In reply to comment #5) > (In reply to comment #4) > > Comment on attachment 265484 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=265484&action=review > > > > > Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:207 > > > + WTFLogAlways("requestScrollPositionUpdate %d", scrollPosition.y()); > > > > Did you mean to leave this in? > > Definitely not. > > > > Source/WebKit2/UIProcess/ViewGestureController.cpp:126 > > > +#if PLATFORM(MAC) > > > + // On Mac, we check if the scroll position restoration succeeded by comparing the > > > + // requested and actual scroll position. It's possible that we will never succeed in restoring > > > + // the exact scroll position we wanted, in the case of a dynamic page, but we know that by > > > + // main frame load time that we've gotten as close as we're going to get, so stop waiting. > > > + // We don't want to do this on iOS because scroll position restoration is baked into the transaction. > > > + m_snapshotRemovalTracker.cancelOutstandingEvent(SnapshotRemovalTracker::ScrollPositionRestoration); > > > +#endif > > > > This mentions Mac and iOS; not sure if other platforms would be more like > > Mac or more like iOS. > > You raise a good point. Really, this should be "uses UI-side > compositing/transactional scrolling/something", and we'd want the "iOS" > behavior if you were using UI-side compositing on Mac. We don't have a > fantastic way to tell right now, but we should. I'll come up with something. ... but in another patch, because it is going to be a sizable project to get ViewGestureController to work correctly based on DrawingArea implementation instead of platform ifdefs. > > > Source/WebKit2/UIProcess/ios/PageClientImplIOS.mm:758 > > > +{ > > > + > > > +} > > > > I’d omit the blank line.
Tim Horton
Comment 7 2015-12-01 11:57:35 PST
Note You need to log in before you can comment on or make changes to this bug.