[Qt][WK2] Remove ViewportUpdateDeferrer from PageViewportController
Created attachment 171220 [details] Patch
Comment on attachment 171220 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171220&action=review I liked that we had a stack for suspend resume before, now it might evolve in a group of loosely related booleans depending on future needs. It also feels like there might be case where we'd have to call resume that could slip. Having the update deferrer acting as a "suspend lock" was pretty effective I think, so I'm not sure if it's a good idea to get rid of it completely. What do you think? > Source/WebKit2/UIProcess/qt/PageViewportControllerClientQt.cpp:179 > + m_isUserInteracting = false; What if you touched, but didn't start a flick and released your finger, shouldn't you call resume here for that case? > Source/WebKit2/UIProcess/qt/PageViewportControllerClientQt.cpp:-461 > - m_scaleUpdateDeferrer.reset(); // Clear after starting potential animation, which takes over deferring. No resume here either?
Created attachment 171281 [details] Patch
In reply to comment #2) > (From update of attachment 171220 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=171220&action=review > > I liked that we had a stack for suspend resume before, now it might evolve in a group of loosely related booleans depending on future needs. It also feels like there might be case where we'd have to call resume that could slip. Having the update deferrer acting as a "suspend lock" was pretty effective I think, so I'm not sure if it's a good idea to get rid of it completely. What do you think? As soon as the functionality of guarding suspend-resume cycles (viewport guard) was mixed with the deferring functionality to delay updates until the deferrer is in scope the usage became quite messy. After the recent refactoring, besides convenience, there is no real need to have a stack suspend/resume any more. We can simply have suspend/resume calls for the former where needed and updateViewportController for the later. The only boolean we need is to track if the user has the finger on the screen or not, a state which was previously indirect through the usage of a deferrer. I do not see a use-case which would need to add a new state that could not be deduced from current states, thus I think having a stacking mechanism not only complicates debugging but also hides possible bugs by obfuscating the state machine. I see the tempting convenience of not having to deal with the proper placement of suspend/resume pairs, though :) > > > Source/WebKit2/UIProcess/qt/PageViewportControllerClientQt.cpp:179 > > + m_isUserInteracting = false; > > What if you touched, but didn't start a flick and released your finger, shouldn't you call resume here for that case? If the pan gesture did not result in a movement then the content is not suspended, if it resulted in a movement the content is suspended when movement starts and resumed when the movement ends, no matter if the movement ended with a flick animation or not (eg. if the content is just dragged or flicked). I see how the name of flickMoveStarted/Ended in the client is misleading and should be changed since they are not triggered on flick animation start/end but movement start/end. But since this is the current behavior and unrelated to this patch the rename should be addressed in a separate patch, I think. > > > Source/WebKit2/UIProcess/qt/PageViewportControllerClientQt.cpp:-461 > > - m_scaleUpdateDeferrer.reset(); // Clear after starting potential animation, which takes over deferring. > > No resume here either? There is no need for resume here since animateContentRectVisible should result in a resume if there is no animation performed. Added a comment.
Comment on attachment 171281 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171281&action=review For some reasons this makes me afraid that as the code evolves we'll get cases where the contents is left suspended. That might be because the current code was over protective too. Let's test it hard and make sure that it won't happen for 5.0, worst case we can add protection later. > Source/WebKit2/UIProcess/qt/PageViewportControllerClientQt.cpp:458 > + // Should always resume the content, even if no animation was performed. Maybe write it something like "This should" or "This will take care or resuming". I wasn't sure if you meant that we should do it here or if it's expected from animateContentRectVisible until I went back to read your comment on the bug report.
Committed r132898: <http://trac.webkit.org/changeset/132898>
Comment on attachment 171281 [details] Patch (In reply to comment #5) > (From update of attachment 171281 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=171281&action=review > > For some reasons this makes me afraid that as the code evolves we'll get cases where the contents is left suspended. That might be because the current code was over protective too. > Let's test it hard and make sure that it won't happen for 5.0, worst case we can add protection later. Thanks for the review. Yes, in some cases the code became overprotective since the switch to Flickable and after the recent refactoring. I'll make sure it does not introduce issues. > > > Source/WebKit2/UIProcess/qt/PageViewportControllerClientQt.cpp:458 > > + // Should always resume the content, even if no animation was performed. > > Maybe write it something like "This should" or "This will take care or resuming". I wasn't sure if you meant that we should do it here or if it's expected from animateContentRectVisible until I went back to read your comment on the bug report. Done.