[Qt][WK2] Refactor handling of content suspension to properly cover corner cases
Created attachment 192037 [details] Patch
*** Bug 111081 has been marked as a duplicate of this bug. ***
Comment on attachment 192037 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192037&action=review > Source/WebKit2/UIProcess/PageViewportController.cpp:315 > +void PageViewportController::suspendContent() > +{ > m_webPageProxy->suspendActiveDOMObjectsAndAnimations(); > } With those modifications could suspendContent, resumeContent and hasSuspendedContent be removed and directly accessed through WebPageProxy? > Source/WebKit2/UIProcess/qt/PageViewportControllerClientQt.cpp:174 > + ASSERT(!scrollAnimationActive()); What does this protect, would anything break if this was the case? There is also already a similar guard in animateContentRectVisible. > Source/WebKit2/UIProcess/qt/PageViewportControllerClientQt.cpp:192 > + ++m_activeSuspensionCount; I see this as a violation of the abstraction, it dupplicates logic and this could lead to problems if the code evolves in some wicked way. How about having some kind of "guardOnly" flag/property in a m_touchSuspensionGuard SuspensionController that would do the same except triggering the call to suspendContent()? > Source/WebKit2/UIProcess/qt/PageViewportControllerClientQt.h:109 > + SuspensionController(PageViewportControllerClientQt* parent) "parent" is usually used in a hierarchy, maybe just "client"?
Created attachment 192939 [details] Patch
Created attachment 193559 [details] Patch
(In reply to comment #5) > Created an attachment (id=193559) [details] > Patch Rebased the original patch and made some renames to better indicate the responsibilities.
Created attachment 193572 [details] Patch
(In reply to comment #7) > Created an attachment (id=193572) [details] > Patch Stripped out some unrelated changes.
Comment on attachment 193572 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193572&action=review > Source/WebKit2/UIProcess/qt/PageViewportControllerClientQt.cpp:48 > + , m_touchInteraction(this, false /* shouldSuspend */) Nit: I think we usually put the comment before the value. Otherwise it LGTM. You really need owners to look at the WebPageProxy change, in particular.
(In reply to comment #9) > Otherwise it LGTM. You really need owners to look at the WebPageProxy change, in particular. @Benjamin, do you have a minute to check it out?
Comment on attachment 193572 [details] Patch I don't really have time to review for correctness. The changes on WebPageProxy are fine. Signed off by me for WebKit2.
Comment on attachment 193572 [details] Patch Thanks, r=me then.
Comment on attachment 193572 [details] Patch Clearing flags on attachment: 193572 Committed r146355: <http://trac.webkit.org/changeset/146355>
All reviewed patches have been landed. Closing bug.