PageClientEfl is needed for accelerated compositing in EFL port.
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).
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.
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.
Comment on attachment 131780 [details] patch Attachment 131780 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11954226
Comment on attachment 131780 [details] patch Looks like a real build breakage.
Created attachment 132486 [details] modified patch
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.
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~
If one of the EFL guys wants to do an unofficial review, I'd be happy to rs this; the AC parts look ok.
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.
Created attachment 132982 [details] patch with updates from comment #10
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. :)
(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 ).
Created attachment 132990 [details] reupload Rebased the patch.
(In reply to comment #14) > Created an attachment (id=132990) [details] > reupload > > Rebased the patch. Looks good to me. Thanks.
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.
Created attachment 133219 [details] patch with updates from comment #16
Comment on attachment 133219 [details] patch with updates from comment #16 Clearing flags on attachment: 133219 Committed r111673: <http://trac.webkit.org/changeset/111673>
All reviewed patches have been landed. Closing bug.