Bug 97646 – [EFL][WK2] WebProcess keeps on waiting for UIProcess

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Regina Chung]

WebKit2 Efl with Coordinated Graphics is not working because WebProcess never gets RenderNextFrame message from UIProcess.

[Regina Chung]

Created attachment 165759 [details]
Patch

[Yael]

Comment on attachment 165759 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:533
> +    priv->viewportHandler->setRendererActive(true);

Is this really needed every time we enter AC or only the first time?
As an aside, PageClientImpl::enterAcceleratedCompositingMode() is not implemented in any other port, so we should probably move the initialization code out of it instead of adding to it.

[Yael]

(In reply to comment #2)
> (From update of attachment 165759 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165759&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:533
> > +    priv->viewportHandler->setRendererActive(true);
> 
> Is this really needed every time we enter AC or only the first time?
> As an aside, PageClientImpl::enterAcceleratedCompositingMode() is not implemented in any other port, so we should probably move the initialization code out of it instead of adding to it.

After http://trac.webkit.org/changeset/130389 ewk_view_accelerated_compositing_mode_enter is called only once.
This patch looks good to me.

[Gyuyoung Kim]

Don't you need to set setRendererActive(false) in ewk_view_accelerated_compositing_mode_exit() ?

[Yael]

(In reply to comment #4)
> Don't you need to set setRendererActive(false) in ewk_view_accelerated_compositing_mode_exit() ?

What is the benefit of doing that?
We need to call setActive(true) to trigger the first renderNextFrame, so that we can start rendering.

[Kenneth Rohde Christiansen]

Comment on attachment 165759 [details]
Patch

OK, r=me then. Anyway we should rename the class to follow current trunk style and inherit from the ViewportPageController, like Qt. But that is not directly related to this, but maybe something for you Yael?

[WebKit Review Bot]

Comment on attachment 165759 [details]
Patch

Clearing flags on attachment: 165759

Committed r131091: <http://trac.webkit.org/changeset/131091>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yael]

(In reply to comment #6)
> (From update of attachment 165759 [details])
> OK, r=me then. Anyway we should rename the class to follow current trunk style and inherit from the ViewportPageController, like Qt. But that is not directly related to this, but maybe something for you Yael?

Filed https://bugs.webkit.org/show_bug.cgi?id=99101 for that.

[Gyuyoung Kim]

(In reply to comment #5)
> (In reply to comment #4)
> > Don't you need to set setRendererActive(false) in ewk_view_accelerated_compositing_mode_exit() ?
> 
> What is the benefit of doing that?
> We need to call setActive(true) to trigger the first renderNextFrame, so that we can start rendering.

First, I'm not expert for graphics area. To my experience, however, we have been set true/false in a pair of functions generally if there is needs to set specific setting. In this case, ewk_view_accelerated_compositing_mode_exit() might don't need to set setActive(false) because ewk_view_accelerated_compositing_mode_exit() will be called only once when existing program, right ?  If so, we might not call setActive(false) from a functionality perspective. But, IMO, it is not bad to call setActive(false) in _exit() from a pair of functions perspective. If the _exit() will be called many times, isn't it better or safer to call setActive(false) in _exit() ?

Anyway, patch was already landed. I'm just wondering about this.