Bug 151224 – Remove swipe snapshot before main document load if scroll position is already restored

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Horton]

Remove swipe snapshot before main document load if scroll position is already restored

[Tim Horton]

Created attachment 265434 [details]
Patch

[Tim Horton]

Created attachment 265436 [details]
Patch

[Tim Horton]

Created attachment 265484 [details]
Patch

[Darin Adler]

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]

(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]

(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]

http://trac.webkit.org/changeset/192898