Summary: | [Qt][WK2] Refactor handling of content suspension to properly cover corner cases | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andras Becsi <abecsi> | ||||||||||
Component: | New Bugs | Assignee: | Andras Becsi <abecsi> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | benjamin, cmarcelo, jturcotte, menard, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 110211, 112679 | ||||||||||||
Attachments: |
|
Description
Andras Becsi
2013-03-07 10:03:45 PST
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. |