Bug 109290 – [Qt][WK2] Replace suspend/resume internal API usage with C API in the PageViewportController

• Status: RESOLVED
• Resolution: INVALID
• Component: New Bugs

[Andras Becsi]

[Qt][WK2] Replace suspend/resume internal API usage with C API in the PageViewportController

[Andras Becsi]

Created attachment 187298 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 187298 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=187298&action=review

> Source/WebKit2/UIProcess/API/C/WKPagePrivate.h:77
> +WK_EXPORT bool WKPageAreActiveDOMObjectsAndAnimationsSuspended(WKPageRef page);
> +WK_EXPORT void WKPageSuspendActiveDOMObjectsAndAnimations(WKPageRef page);
> +WK_EXPORT void WKPageResumeActiveDOMObjectsAndAnimations(WKPageRef page);

These are very view specific... we added similar methods to the EFL WKView.

[Jocelyn Turcotte]

Comment on attachment 187298 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=187298&action=review

> Source/WebKit2/UIProcess/PageViewportController.cpp:43
> +PageViewportController::PageViewportController(WKPageRef pageRef, PageViewportControllerClient* client)

It's called pageRef within the WK* C API implementation, but from the client side, we can name the variable "page".

> Source/WebKit2/UIProcess/PageViewportController.cpp:45
> +    : m_webPageProxy(toImpl(pageRef))
> +    , m_wkPage(pageRef)

I didn't understand why we did that in QQuickWebView as well, but why do we need to store two types of the same value?
Can't we just call toImpl/toAPI when needed?

> Source/WebKit2/UIProcess/PageViewportController.cpp:-322
> -    if (!m_hasSuspendedContent)
> -        return;

This reverts the work done in bug #80294. Is this what we want?

> Source/WebKit2/UIProcess/PageViewportController.h:96
> +    WKRetainPtr<WKPageRef> m_wkPage;

Is this a normal practice to increase the refcount for this kind of case?
Technically only the QQuickWebView should need to retain it, maybe we want this for safety. In any case, I'm just asking.

[Simon Hausmann]

Comment on attachment 187298 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=187298&action=review

>> Source/WebKit2/UIProcess/API/C/WKPagePrivate.h:77
>> +WK_EXPORT void WKPageResumeActiveDOMObjectsAndAnimations(WKPageRef page);
> 
> These are very view specific... we added similar methods to the EFL WKView.

How are they view specific if all they do is operate on a WebPageProxy and send a message to the WebPage in the WebProcess? :)

[Kenneth Rohde Christiansen]

(In reply to comment #4)
> (From update of attachment 187298 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187298&action=review
> 
> >> Source/WebKit2/UIProcess/API/C/WKPagePrivate.h:77
> >> +WK_EXPORT void WKPageResumeActiveDOMObjectsAndAnimations(WKPageRef page);
> > 
> > These are very view specific... we added similar methods to the EFL WKView.
> 
> How are they view specific if all they do is operate on a WebPageProxy and send a message to the WebPage in the WebProcess? :)

Well there is no view class for IPC so it has to go through the web page proxy. But I more mean that this is probably something you would do per view.

[Andras Becsi]

(In reply to comment #3)
> (From update of attachment 187298 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187298&action=review
> 
> > Source/WebKit2/UIProcess/PageViewportController.cpp:43
> > +PageViewportController::PageViewportController(WKPageRef pageRef, PageViewportControllerClient* client)
> 
> It's called pageRef within the WK* C API implementation, but from the client side, we can name the variable "page".
> 
> > Source/WebKit2/UIProcess/PageViewportController.cpp:45
> > +    : m_webPageProxy(toImpl(pageRef))
> > +    , m_wkPage(pageRef)
> 
> I didn't understand why we did that in QQuickWebView as well, but why do we need to store two types of the same value?
> Can't we just call toImpl/toAPI when needed?

I think the webPageProxy member should be removed as soon as everything is moved to the C API, we could use toAPI, and change everything to WKPage in one go at the end if you think that would be better.

> 
> > Source/WebKit2/UIProcess/PageViewportController.cpp:-322
> > -    if (!m_hasSuspendedContent)
> > -        return;
> 
> This reverts the work done in bug #80294. Is this what we want?

There is a check for that in WebPageProxy, so the call does not end up in an IPC message if the page is already suspended.
In fact I think the issue of #80294 has already been reintroduced when the update guard was removed. I have to look into that.

> 
> > Source/WebKit2/UIProcess/PageViewportController.h:96
> > +    WKRetainPtr<WKPageRef> m_wkPage;
> 
> Is this a normal practice to increase the refcount for this kind of case?
> Technically only the QQuickWebView should need to retain it, maybe we want this for safety. In any case, I'm just asking.

I think we want this for safety.

[Jocelyn Turcotte]

(In reply to comment #6)
> (In reply to comment #3)
> > (From update of attachment 187298 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=187298&action=review
> > 
> > > Source/WebKit2/UIProcess/PageViewportController.cpp:45
> > > +    : m_webPageProxy(toImpl(pageRef))
> > > +    , m_wkPage(pageRef)
> > 
> > I didn't understand why we did that in QQuickWebView as well, but why do we need to store two types of the same value?
> > Can't we just call toImpl/toAPI when needed?
> 
> I think the webPageProxy member should be removed as soon as everything is moved to the C API, we could use toAPI, and change everything to WKPage in one go at the end if you think that would be better.

You could also replace the WebPageProxy* with a WKPageRef now and wrap references that you can't change with toImpl, as you wish.
If you keep it, could you also name the member m_page instead of m_wkPage? No need to repeat a prefix whose purpose is to avoid clashes of the API in the global namespace.

[Andras Becsi]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #3)
> > > (From update of attachment 187298 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=187298&action=review
> > > 
> > > > Source/WebKit2/UIProcess/PageViewportController.cpp:45
> > > > +    : m_webPageProxy(toImpl(pageRef))
> > > > +    , m_wkPage(pageRef)
> > > 
> > > I didn't understand why we did that in QQuickWebView as well, but why do we need to store two types of the same value?
> > > Can't we just call toImpl/toAPI when needed?
> > 
> > I think the webPageProxy member should be removed as soon as everything is moved to the C API, we could use toAPI, and change everything to WKPage in one go at the end if you think that would be better.
> 
> You could also replace the WebPageProxy* with a WKPageRef now and wrap references that you can't change with toImpl, as you wish.
> If you keep it, could you also name the member m_page instead of m_wkPage? No need to repeat a prefix whose purpose is to avoid clashes of the API in the global namespace.

I think changing everything reference to WKPageRef would make the patch less compact, and make the places which still need to be moved to the C API harder to spot, so I would rather keep the WebPageProxy member yet.

[Andras Becsi]

Created attachment 187318 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 187318 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=187318&action=review

> Source/WebKit2/UIProcess/PageViewportController.h:95
>      WebPageProxy* const m_webPageProxy;
> +

This might have slipped in.

Other that that the patch looks fine to me, as long as you can get the added C API approved.

[Jocelyn Turcotte]

(In reply to comment #2)
> (From update of attachment 187298 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187298&action=review
> 
> > Source/WebKit2/UIProcess/API/C/WKPagePrivate.h:77
> > +WK_EXPORT bool WKPageAreActiveDOMObjectsAndAnimationsSuspended(WKPageRef page);
> > +WK_EXPORT void WKPageSuspendActiveDOMObjectsAndAnimations(WKPageRef page);
> > +WK_EXPORT void WKPageResumeActiveDOMObjectsAndAnimations(WKPageRef page);
> 
> These are very view specific... we added similar methods to the EFL WKView.

It's true that this could be better abstracted behind the view with some kind of isMoving/beginMove/endMove method calls and visible rect updated in-between.

This is where it is going to be difficult to have a dumb fixedLayout view with the viewport interaction being handled outside of the C API.
By thinking a bit more about it, I'm not sure either if bluntly exposing those WebPageProxy functions is the right way to do it.

[Kenneth Rohde Christiansen]

EFL added these methods to our WKView (similar to your QRawWebView), but ofcourse that way we cannot share

[Jocelyn Turcotte]

(In reply to comment #12)
> EFL added these methods to our WKView (similar to your QRawWebView), but ofcourse that way we cannot share

How about sharing a platform agnostic view that takes care of handling events and rendering through OpenGL? :)

[Kenneth Rohde Christiansen]

(In reply to comment #13)
> (In reply to comment #12)
> > EFL added these methods to our WKView (similar to your QRawWebView), but ofcourse that way we cannot share
> 
> How about sharing a platform agnostic view that takes care of handling events and rendering through OpenGL? :)

I am up for that. We now have a very limited (but growing) WKView C API. I have some local patches making it possible to actually draw using this API.

[Caio Marcelo de Oliveira Filho]

(In reply to comment #13)
> (In reply to comment #12)
> > EFL added these methods to our WKView (similar to your QRawWebView), but ofcourse that way we cannot share
> 
> How about sharing a platform agnostic view that takes care of handling events and rendering through OpenGL? :)

Wow, this pretty much describes what is the WebView in WebKitNix! I'm working a patch transforming our NIXView into a WKView for Coordinated Graphics. As soon as it looks nice I'll post it for you and Kenneth to review.

[Caio Marcelo de Oliveira Filho]

(In reply to comment #15)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > EFL added these methods to our WKView (similar to your QRawWebView), but ofcourse that way we cannot share
> > 
> > How about sharing a platform agnostic view that takes care of handling events and rendering through OpenGL? :)
> 
> Wow, this pretty much describes what is the WebView in WebKitNix! I'm working a patch transforming our NIXView into a WKView for Coordinated Graphics. As soon as it looks nice I'll post it for you and Kenneth to review.

This is already happening as part of https://bugs.webkit.org/show_bug.cgi?id=107662. MPozdnyakov is working on patches toward that direction.

[Andras Becsi]

Comment on attachment 187318 [details]
Patch

Removing the patch from the review queue. Revisiting this as soon as we have a shared WebView and its responsibilities are sorted out.

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.