Bug 109290 - [Qt][WK2] Replace suspend/resume internal API usage with C API in the PageViewportController
Summary: [Qt][WK2] Replace suspend/resume internal API usage with C API in the PageVie...
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andras Becsi
URL:
Keywords:
Depends on:
Blocks: 108471
  Show dependency treegraph
 
Reported: 2013-02-08 05:27 PST by Andras Becsi
Modified: 2014-02-03 03:24 PST (History)
11 users (show)

See Also:


Attachments
Patch (16.47 KB, patch)
2013-02-08 05:29 PST, Andras Becsi
no flags Details | Formatted Diff | Diff
Patch (16.39 KB, patch)
2013-02-08 07:28 PST, Andras Becsi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andras Becsi 2013-02-08 05:27:24 PST
[Qt][WK2] Replace suspend/resume internal API usage with C API in the PageViewportController
Comment 1 Andras Becsi 2013-02-08 05:29:09 PST
Created attachment 187298 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2013-02-08 05:40:29 PST
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.
Comment 3 Jocelyn Turcotte 2013-02-08 06:03:43 PST
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.
Comment 4 Simon Hausmann 2013-02-08 06:22:05 PST
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? :)
Comment 5 Kenneth Rohde Christiansen 2013-02-08 06:25:46 PST
(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.
Comment 6 Andras Becsi 2013-02-08 06:45:30 PST
(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.
Comment 7 Jocelyn Turcotte 2013-02-08 07:09:40 PST
(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.
Comment 8 Andras Becsi 2013-02-08 07:18:32 PST
(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.
Comment 9 Andras Becsi 2013-02-08 07:28:40 PST
Created attachment 187318 [details]
Patch
Comment 10 Jocelyn Turcotte 2013-02-08 07:45:15 PST
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.
Comment 11 Jocelyn Turcotte 2013-02-08 07:54:58 PST
(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.
Comment 12 Kenneth Rohde Christiansen 2013-02-08 08:11:12 PST
EFL added these methods to our WKView (similar to your QRawWebView), but ofcourse that way we cannot share
Comment 13 Jocelyn Turcotte 2013-02-08 08:46:08 PST
(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? :)
Comment 14 Kenneth Rohde Christiansen 2013-02-08 08:55:54 PST
(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.
Comment 15 Caio Marcelo de Oliveira Filho 2013-02-18 14:55:00 PST
(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.
Comment 16 Caio Marcelo de Oliveira Filho 2013-02-20 09:21:33 PST
(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.
Comment 17 Andras Becsi 2013-02-25 03:46:44 PST
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.
Comment 18 Jocelyn Turcotte 2014-02-03 03:24:59 PST
=== 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.