Bug 111751

Summary: [Qt][WK2] Refactor handling of content suspension to properly cover corner cases
Product: WebKit Reporter: Andras Becsi <abecsi>
Component: New BugsAssignee: 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 Flags
Patch
none
Patch
none
Patch
none
Patch none

Andras Becsi
Reported 2013-03-07 10:03:45 PST
[Qt][WK2] Refactor handling of content suspension to properly cover corner cases
Attachments
Patch (16.07 KB, patch)
2013-03-07 10:12 PST, Andras Becsi 		no flags
DetailsFormatted DiffDiff
Patch (19.36 KB, patch)
2013-03-13 09:46 PDT, Andras Becsi 		no flags
DetailsFormatted DiffDiff
Patch (20.01 KB, patch)
2013-03-18 07:31 PDT, Andras Becsi 		no flags
DetailsFormatted DiffDiff
Patch (18.59 KB, patch)
2013-03-18 08:48 PDT, Andras Becsi 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Andras Becsi
Comment 1 2013-03-07 10:12:38 PST
Created attachment 192037 [details] Patch
Andras Becsi
Comment 2 2013-03-07 10:14:55 PST
*** Bug 111081 has been marked as a duplicate of this bug. ***
Jocelyn Turcotte
Comment 3 2013-03-11 08:41:11 PDT
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"?
Andras Becsi
Comment 4 2013-03-13 09:46:47 PDT
Created attachment 192939 [details] Patch
Andras Becsi
Comment 5 2013-03-18 07:31:06 PDT
Created attachment 193559 [details] Patch
Andras Becsi
Comment 6 2013-03-18 07:35:15 PDT
(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.
Andras Becsi
Comment 7 2013-03-18 08:48:21 PDT
Created attachment 193572 [details] Patch
Andras Becsi
Comment 8 2013-03-18 08:49:18 PDT
(In reply to comment #7) > Created an attachment (id=193572) [details] > Patch Stripped out some unrelated changes.
Jocelyn Turcotte
Comment 9 2013-03-18 08:52:51 PDT
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.
Jocelyn Turcotte
Comment 10 2013-03-18 08:54:29 PDT
(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?
Benjamin Poulain
Comment 11 2013-03-19 15:18:26 PDT
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.
Jocelyn Turcotte
Comment 12 2013-03-20 09:36:42 PDT
Comment on attachment 193572 [details] Patch Thanks, r=me then.
Andras Becsi
Comment 13 2013-03-20 09:47:38 PDT
Comment on attachment 193572 [details] Patch Clearing flags on attachment: 193572 Committed r146355: <http://trac.webkit.org/changeset/146355>
Andras Becsi
Comment 14 2013-03-20 09:47:43 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.