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+

Description Tim Horton 2015-11-12 14:24:08 PST
Remove swipe snapshot before main document load if scroll position is already restored
Comment 1 Tim Horton 2015-11-12 14:24:50 PST
Created attachment 265434 [details]
Patch
Comment 2 Tim Horton 2015-11-12 14:38:46 PST
Created attachment 265436 [details]
Patch
Comment 3 Tim Horton 2015-11-13 10:19:16 PST
Created attachment 265484 [details]
Patch
Comment 4 Darin Adler 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.
Comment 5 Tim Horton 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.
Comment 6 Tim Horton 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.
Comment 7 Tim Horton 2015-12-01 11:57:35 PST
http://trac.webkit.org/changeset/192898