Swipe snapshot is removed too early when swiping away from a page that is still loading
Created attachment 403138 [details] Patch
<rdar://problem/64576811>
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.
Created attachment 403142 [details] Patch
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`
(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!
Created attachment 403153 [details] Patch
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
Created attachment 403162 [details] Patch
Test failures look legit.
Created attachment 403331 [details] Patch
Comment on attachment 403331 [details] Patch Looks great. Iām going to say review+ even before EWS tells us what we should really think.
Just gotta skip the test on WK1!
Created attachment 403334 [details] Patch
Committed r263825: <https://trac.webkit.org/changeset/263825> All reviewed patches have been landed. Closing bug and clearing flags on attachment 403334 [details].