Bug 80748 – [EFL] Add PageClientEfl to WebCoreSupport

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Hyowon Kim]

PageClientEfl is needed for accelerated compositing in EFL port.

[Hyowon Kim]

Created attachment 131780 [details]
patch

This patch adds PageClientEfl to WebKit/efl/WebCoreSupport.
PageClientEfl now has two functions are called in GraphicsContext3DPrivate, 
createEvasObjectForAcceleratedCompositing() and acceleratedCompositingContext().
For more details, see the implementation of GraphicsContext3DPrivate (Bug 80211).

[Gyuyoung Kim]

Comment on attachment 131780 [details]
patch

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

> Source/WebKit/efl/WebCoreSupport/PageClientEfl.cpp:38
> +bool PageClientEfl::createEvasObjectForAcceleratedCompositing(Evas_Native_Surface *nativeSurface, int x, int y, int width, int height)

Minor nit : move '*' to data type.

> Source/WebKit/efl/ewk/ewk_view.cpp:717
> +    priv->pageClient = adoptPtr(new WebCore::PageClientEfl(smartData->self));

As you may know, there is PageClients struct of Page class in order to manage clients for WebCoreSupport. It looks confusion by this class name may occur.
In my humble opinion,  it is better to use a little more clear name for PageClientEfl.

> Source/WebKit/efl/ewk/ewk_view.cpp:3928
> +bool ewk_view_accelerated_compositing_object_create(Evas_Object* ewkView, Evas_Native_Surface *nativeSurface, int x, int y, int width, int height)

ditto.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 131780 [details]
patch

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

> Source/WebKit/ChangeLog:8
> +        No new tests. No behavior change.

Not really necessary here.

> Source/WebKit/efl/WebCoreSupport/PageClientEfl.h:40
> +    Evas_Object* view() { return m_view; }

Where is this being used?

> Source/WebKit/efl/ewk/ewk_private.h:247
> +bool ewk_view_accelerated_compositing_object_create(Evas_Object* ewkView, Evas_Native_Surface* nativeSurface, int x, int y, int width, int height);
> +WebCore::GraphicsContext3D* ewk_view_accelerated_compositing_context_get(Evas_Object* ewkView);

These functions have no real implementation. Why don't you just call notImplemented() and return false/0 in PageClientEfl.cpp and get rid of these two functions here?

> Source/WebKit/efl/ewk/ewk_private.h:250
> +WebCore::PlatformPageClient ewk_view_page_client_get(Evas_Object* ewkView);

Please don't return a PlatformPageClient like this; follow the core{Frame,Page,HistoryItem} style instead.

> Source/WebKit/efl/ewk/ewk_view.cpp:3931
> +    EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, false);
> +    EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv, false);

You're always returning false, so these checks are not needed.

> Source/WebKit/efl/ewk/ewk_view.cpp:3940
> +    EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, 0);
> +    EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv, 0);

Ditto.

[Gyuyoung Kim]

Comment on attachment 131780 [details]
patch

Attachment 131780 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11954226

[Hajime Morrita]

Comment on attachment 131780 [details]
patch

Looks like a real build breakage.

[Hyowon Kim]

Created attachment 132486 [details]
modified patch

[Hyowon Kim]

Thanks for your comments. :D

> > Source/WebKit/efl/WebCoreSupport/PageClientEfl.cpp:38
> > +bool PageClientEfl::createEvasObjectForAcceleratedCompositing(Evas_Native_Surface *nativeSurface, int x, int y, int width, int height)
> 
> Minor nit : move '*' to data type.

Done.

> > Source/WebKit/efl/ewk/ewk_view.cpp:717
> > +    priv->pageClient = adoptPtr(new WebCore::PageClientEfl(smartData->self));
> 
> As you may know, there is PageClients struct of Page class in order to manage clients for WebCoreSupport. It looks confusion by this class name may occur.
> In my humble opinion,  it is better to use a little more clear name for PageClientEfl.

I agree with your opinion.
But, I have no idea of better name for this class now.
how about refactoring to rename it in another patch?

> > Source/WebKit/efl/ewk/ewk_view.cpp:3928
> > +bool ewk_view_accelerated_compositing_object_create(Evas_Object* ewkView, Evas_Native_Surface *nativeSurface, int x, int y, int width, int height)
> 
> ditto.

Done.

[Hyowon Kim]

Thanks for your comments. :D

> > Source/WebKit/ChangeLog:8
> > +        No new tests. No behavior change.
> 
> Not really necessary here.

I've removed it. Done.

> > Source/WebKit/efl/WebCoreSupport/PageClientEfl.h:40
> > +    Evas_Object* view() { return m_view; }
> 
> Where is this being used?

To get Evas* in GraphicsContext3DPrivate::initialize()

bool GraphicsContext3DPrivate::initialize(GraphicsContext3D::Attributes attributes, HostWindow* hostWindow, bool renderDirectlyToHostWindow)
{
    PageClientEfl* pageClient = static_cast<PageClientEfl*>(hostWindow->platformPageClient());

    Evas* evas = evas_object_evas_get(pageClient->view());
	
    ......
}

> > Source/WebKit/efl/ewk/ewk_private.h:247
> > +bool ewk_view_accelerated_compositing_object_create(Evas_Object* ewkView, Evas_Native_Surface* nativeSurface, int x, int y, int width, int height);
> > +WebCore::GraphicsContext3D* ewk_view_accelerated_compositing_context_get(Evas_Object* ewkView);
> 
> These functions have no real implementation. Why don't you just call notImplemented() and return false/0 in PageClientEfl.cpp and get rid of these two functions here?

I just want to show what to do in PageClientEfl even though not implemented functions are called.
And I'll upload next patch for implementation of these two functions soon.

> > Source/WebKit/efl/ewk/ewk_private.h:250
> > +WebCore::PlatformPageClient ewk_view_page_client_get(Evas_Object* ewkView);
> 
> Please don't return a PlatformPageClient like this; follow the core{Frame,Page,HistoryItem} style instead.

Done.

> > Source/WebKit/efl/ewk/ewk_view.cpp:3931
> > +    EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, false);
> > +    EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv, false);
> 
> You're always returning false, so these checks are not needed.
>
> > Source/WebKit/efl/ewk/ewk_view.cpp:3940
> > +    EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, 0);
> > +    EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv, 0);
> 
> Ditto.

Done both~

[Noam Rosenthal]

If one of the EFL guys wants to do an unofficial review, I'd be happy to rs this; the AC parts look ok.

[Ryuan Choi]

Comment on attachment 132486 [details]
modified patch

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

> Source/WebCore/platform/Widget.h:97
> +#elif PLATFORM(EFL)
> +namespace WebCore {
> +class PageClientEfl;
> +typedef PageClientEfl* PlatformPageClient;
> +}

Check PluginViewEfl.cpp. this file also use platformPageClient()

> Source/WebKit/efl/ewk/ewk_private.h:246
> +bool ewk_view_accelerated_compositing_object_create(Evas_Object* ewkView, Evas_Native_Surface* nativeSurface, int x, int y, int width, int height);

I prefer IntRect for internal functions.

[Hyowon Kim]

Created attachment 132982 [details]
patch with updates from comment #10

[Hyowon Kim]

Thanks for your comments. :D

> > Source/WebCore/platform/Widget.h:97
> > +#elif PLATFORM(EFL)
> > +namespace WebCore {
> > +class PageClientEfl;
> > +typedef PageClientEfl* PlatformPageClient;
> > +}
> 
> Check PluginViewEfl.cpp. this file also use platformPageClient()

I've checked and modified it.

> > Source/WebKit/efl/ewk/ewk_private.h:246
> > +bool ewk_view_accelerated_compositing_object_create(Evas_Object* ewkView, Evas_Native_Surface* nativeSurface, int x, int y, int width, int height);
> 
> I prefer IntRect for internal functions.

Done. IntRect looks better. Thanks. :)

[Ryuan Choi]

(In reply to comment #11)
> Created an attachment (id=132982) [details]
> patch with updates from comment #10

Source/JavaScriptCore/wtf/Platform.h was moved after r111504 ( Bug 80911 ).

[Hyowon Kim]

Created attachment 132990 [details]
reupload

Rebased the patch.

[Ryuan Choi]

(In reply to comment #14)
> Created an attachment (id=132990) [details]
> reupload
> 
> Rebased the patch.

Looks good to me.
Thanks.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 132990 [details]
reupload

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

Looks OK to me.

> Source/WebKit/efl/WebCoreSupport/PageClientEfl.h:26
> +typedef struct _Evas_Object Evas_Object;
> +typedef struct _Evas_Native_Surface Evas_Native_Surface;

Nitpick: these two should be alphabetically sorted.

[Hyowon Kim]

Created attachment 133219 [details]
patch with updates from comment #16

[WebKit Review Bot]

Comment on attachment 133219 [details]
patch with updates from comment #16

Clearing flags on attachment: 133219

Committed r111673: <http://trac.webkit.org/changeset/111673>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.