Bug 111751 – [Qt][WK2] Refactor handling of content suspension to properly cover corner cases

• Component: New Bugs
• Status: RESOLVED FIXED

[Andras Becsi]

[Qt][WK2] Refactor handling of content suspension to properly cover corner cases

[Andras Becsi]

Created attachment 192037 [details]
Patch

[Andras Becsi]

*** Bug 111081 has been marked as a duplicate of this bug. ***

[Jocelyn Turcotte]

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]

Created attachment 192939 [details]
Patch

[Andras Becsi]

Created attachment 193559 [details]
Patch

[Andras Becsi]

(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]

Created attachment 193572 [details]
Patch

[Andras Becsi]

(In reply to comment #7)
> Created an attachment (id=193572) [details]
> Patch

Stripped out some unrelated changes.

[Jocelyn Turcotte]

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]

(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 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 on attachment 193572 [details]
Patch

Thanks, r=me then.

[Andras Becsi]

Comment on attachment 193572 [details]
Patch

Clearing flags on attachment: 193572

Committed r146355: <http://trac.webkit.org/changeset/146355>

[Andras Becsi]

All reviewed patches have been landed.  Closing bug.