Bug 213763 – Swipe snapshot is removed too early when swiping away from a page that is still loading

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Horton]

Swipe snapshot is removed too early when swiping away from a page that is still loading

[Tim Horton]

Created attachment 403138 [details]
Patch

[Tim Horton]

<rdar://problem/64576811>

[Tim Horton]

swipe-test.js has some extra unused stuff in it, but it's a copy of the one from the non-HTTP tests directory, so I think it's preferable this way.

[Tim Horton]

Created attachment 403142 [details]
Patch

[Devin Rousso]

Comment on attachment 403142 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403142&action=review

it looks like GTK and WPE some love :(

> Source/WebKit/UIProcess/PageClient.h:467
> +    virtual void didFinishNavigation(API::Navigation*) = 0;
> +    virtual void didFailNavigation(API::Navigation*) = 0;
> +    virtual void didSameDocumentNavigationForMainFrame(API::Navigation*, SameDocumentNavigationType) = 0;

Can we use `API::Navigation&` for all of this plumbing instead so we know that it's providing something that exists?  It seems less-than-ideal to me to have the possibility of a function called `_didFinishNavigation` that passes a `nullptr` instead of an `API::Navigation` 😅

> Source/WebKit/UIProcess/WebPageProxy.cpp:4875
> +        pageClient().didFinishNavigation(navigation.get());

Should we still include some mention to "main frame" in these renamed function names since they're only called if `isMainFrame`?  i.e. `didFinishMainFrameNavigation`

> Source/WebKit/UIProcess/mac/ViewGestureControllerMac.mm:647
> +    m_pendingNavigation = nullptr;

I think we should also do this inside `ViewGestureController::removeSwipeSnapshot` for GTK (`ViewGestureControllerGtk.cpp`)

> LayoutTests/http/tests/swipe/resources/swipe-test.js:42
> +    var duration = Date.now() - window.localStorage[key + "swipeStartTime"];

NIT: `let`

[Tim Horton]

(In reply to Devin Rousso from comment #5)
> Comment on attachment 403142 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403142&action=review
> 
> it looks like GTK and WPE some love :(

Yep, uploading a new patch.

> > Source/WebKit/UIProcess/PageClient.h:467
> > +    virtual void didFinishNavigation(API::Navigation*) = 0;
> > +    virtual void didFailNavigation(API::Navigation*) = 0;
> > +    virtual void didSameDocumentNavigationForMainFrame(API::Navigation*, SameDocumentNavigationType) = 0;
> 
> Can we use `API::Navigation&` for all of this plumbing instead so we know
> that it's providing something that exists?  It seems less-than-ideal to me
> to have the possibility of a function called `_didFinishNavigation` that
> passes a `nullptr` instead of an `API::Navigation` 😅

Sadly, it is currently possible to get a null here.

> > Source/WebKit/UIProcess/WebPageProxy.cpp:4875
> > +        pageClient().didFinishNavigation(navigation.get());
> 
> Should we still include some mention to "main frame" in these renamed
> function names since they're only called if `isMainFrame`?  i.e.
> `didFinishMainFrameNavigation`

Nope, Navigation is a main-frame thing in the WebKit2 API (see e.g. WKNavigationDelegate); these names are a holdover from the olden days.

(Possible we should *remove* it from the SameDocument one, really, but I am much too lazy).

> > Source/WebKit/UIProcess/mac/ViewGestureControllerMac.mm:647
> > +    m_pendingNavigation = nullptr;
> 
> I think we should also do this inside
> `ViewGestureController::removeSwipeSnapshot` for GTK
> (`ViewGestureControllerGtk.cpp`)

Quite!

> > LayoutTests/http/tests/swipe/resources/swipe-test.js:42
> > +    var duration = Date.now() - window.localStorage[key + "swipeStartTime"];
> 
> NIT: `let`

You JavaScript people :) 

Thank you for your eyes!

[Tim Horton]

Created attachment 403153 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Tim Horton]

Created attachment 403162 [details]
Patch

[Tim Horton]

Test failures look legit.

[Tim Horton]

Created attachment 403331 [details]
Patch

[Darin Adler]

Comment on attachment 403331 [details]
Patch

Looks great. I’m going to say review+ even before EWS tells us what we should really think.

[Tim Horton]

Just gotta skip the test on WK1!

[Tim Horton]

Created attachment 403334 [details]
Patch

[EWS]

Committed r263825: <https://trac.webkit.org/changeset/263825>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403334 [details].