Add missing implementation of functions for AC in ChromeClientEfl and ewkView.
Created attachment 134008 [details] Patch
Comment on attachment 134008 [details] Patch If another EFL developer wants to sign off on this, LGTM
Comment on attachment 134008 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134008&action=review Looks fine overall with one minor comment. > Source/WebKit/efl/ewk/ewk_view.cpp:154 > + Evas_Object* compositingObject; You could wrap an OwnPtr around this Evas_Object to avoid having to delete it manually below.
(In reply to comment #3) > (From update of attachment 134008 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=134008&action=review > > Looks fine overall with one minor comment. > > > Source/WebKit/efl/ewk/ewk_view.cpp:154 > > + Evas_Object* compositingObject; > > You could wrap an OwnPtr around this Evas_Object to avoid having to delete it manually below. (If you don't do this you should probably check if the pointer is valid before calling evas_object_del()).
Created attachment 134478 [details] Modified patch add null-check for evas_object(compositingObject) in _ewk_view_priv_del().
> > Looks fine overall with one minor comment. > > > > > Source/WebKit/efl/ewk/ewk_view.cpp:154 > > > + Evas_Object* compositingObject; > > > > You could wrap an OwnPtr around this Evas_Object to avoid having to delete it manually below. > > (If you don't do this you should probably check if the pointer is valid before calling evas_object_del()). Thanks Raphael. :) I've added some code to check the evas_object pointer.
Created attachment 145720 [details] updated patch
Peer review first please :)
Comment on attachment 145720 [details] updated patch Attachment 145720 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12896614 New failing tests: http/tests/media/media-source/video-media-source-event-attributes.html
Created attachment 145834 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 146477 [details] rebased patch
Hyowon Kim could you add the necessary files in EFL build system? So other people can test.
(In reply to comment #12) > Hyowon Kim could you add the necessary files in EFL build system? So other people can test. Sure. I need feedback. Bug 88630 is needed to build, and bug 88634 makes AC enable.
Comment on attachment 146477 [details] rebased patch Cleared review? from attachment 146477 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach new patch to this bug or new bug again.
Created attachment 170795 [details] rebased patch I've attached the rebased patch again. I kindly ask for review and comments.
Comment on attachment 170795 [details] rebased patch View in context: https://bugs.webkit.org/attachment.cgi?id=170795&action=review > Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:611 > + return AllTriggers; As a curiosity, Doesn't we need options to choose features ? > Source/WebKit/efl/ewk/ewk_view.cpp:4514 > +static void _ewk_view_accelerated_compositing_cb(void* data, Evas_Object* object) (void* data, Evas_Object*) looks enough. > Source/WebKit/efl/ewk/ewk_view.cpp:4519 > + EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv); If this use only priv, why don't you just send priv as data. > Source/WebKit/efl/ewk/ewk_view.cpp:4527 > +void _ewk_view_accelerated_compositing_context_create_if_needed(Evas_Object* ewkView) static? > Source/WebKit/efl/ewk/ewk_view.cpp:4566 > +WebCore::GraphicsContext3D* ewk_view_accelerated_compositing_context_get(Evas_Object* ewkView) This function returns GraphicsContext3D. > Source/WebKit/efl/ewk/ewk_view.cpp:4592 > + _ewk_view_accelerated_compositing_context_create_if_needed(ewkView); > + evas_object_show(priv->compositingObject); > + } else > + evas_object_hide(priv->compositingObject); > + > + priv->acceleratedCompositingContext->attachRootGraphicsLayer(rootLayer); acceleratedCompositingContext are used, irrespective of compositingActive
Comment on attachment 170795 [details] rebased patch View in context: https://bugs.webkit.org/attachment.cgi?id=170795&action=review > Source/WebKit/efl/ewk/ewk_view.cpp:255 > + bool compositingActive; Do you really need this "compositingActive" . IMHO, it is enough to check that acceleratedCompositingContext exists.
Comment on attachment 170795 [details] rebased patch View in context: https://bugs.webkit.org/attachment.cgi?id=170795&action=review > Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:596 > - notImplemented(); > + ewk_view_root_graphics_layer_set(m_view, rootLayer); Werent we moving to use C++ methods? > Source/WebKit/efl/ewk/ewk_view.cpp:4538 > + > +bool ewk_view_accelerated_compositing_object_create(Evas_Object* ewkView, Evas_Native_Surface* nativeSurface, const WebCore::IntRect& rect) > +{ > + EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, false); Same...
Created attachment 170846 [details] patch from comment #16
Comment on attachment 170846 [details] patch from comment #16 View in context: https://bugs.webkit.org/attachment.cgi?id=170846&action=review > Source/WebKit/efl/ewk/ewk_view.cpp:255 > + bool compositingActive; isCompositingActive > Source/WebKit/efl/ewk/ewk_view.cpp:960 > + if (priv->compositingObject) > + evas_object_del(priv->compositingObject); use RefPtr they support evas objects > Source/WebKit/efl/ewk/ewk_view.cpp:4572 > +void ewk_view_root_graphics_layer_set(Evas_Object* ewkView, WebCore::GraphicsLayer* rootLayer) Why cant these be in the EwkViewImpl ? > Source/WebKit/efl/ewk/ewk_view.cpp:4577 > + bool active = rootLayer ? true : false; = !!rootLayer
(In reply to comment #16) > (From update of attachment 170795 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=170795&action=review > > Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:611 > > + return AllTriggers; > As a curiosity, Doesn't we need options to choose features ? To check options from settings would be needed. I think it would be better to make a new bug to deal with it. > > Source/WebKit/efl/ewk/ewk_view.cpp:4514 > > +static void _ewk_view_accelerated_compositing_cb(void* data, Evas_Object* object) > (void* data, Evas_Object*) looks enough. Done. > > Source/WebKit/efl/ewk/ewk_view.cpp:4519 > > + EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv); > If this use only priv, why don't you just send priv as data. Done. > > Source/WebKit/efl/ewk/ewk_view.cpp:4527 > > +void _ewk_view_accelerated_compositing_context_create_if_needed(Evas_Object* ewkView) > static? My C++ habit. I've removed it. > > Source/WebKit/efl/ewk/ewk_view.cpp:4566 > > +WebCore::GraphicsContext3D* ewk_view_accelerated_compositing_context_get(Evas_Object* ewkView) > This function returns GraphicsContext3D. Any problem? > > Source/WebKit/efl/ewk/ewk_view.cpp:4592 > > + _ewk_view_accelerated_compositing_context_create_if_needed(ewkView); > > + evas_object_show(priv->compositingObject); > > + } else > > + evas_object_hide(priv->compositingObject); > > + > > + priv->acceleratedCompositingContext->attachRootGraphicsLayer(rootLayer); > acceleratedCompositingContext are used, irrespective of compositingActive The acceleratedCompositingContext should always know the current root layer. If compositingActive is false, because the root layer is 0, we should also pass 0 to acceleratedCompositingContext.
(In reply to comment #17) > (From update of attachment 170795 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=170795&action=review > > Source/WebKit/efl/ewk/ewk_view.cpp:255 > > + bool compositingActive; > Do you really need this "compositingActive" . IMHO, it is enough to check that acceleratedCompositingContext exists. The instance of ACContextEfl is created, the first time the root layer is attached. If the root layer is 0, compositingActive is false but ACContextEfl still exists. The compositingActive can be changed regardless of ACContextEfl.
> > Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:596 > > - notImplemented(); > > + ewk_view_root_graphics_layer_set(m_view, rootLayer); > Werent we moving to use C++ methods? > > Source/WebKit/efl/ewk/ewk_view.cpp:4538 > > + > > +bool ewk_view_accelerated_compositing_object_create(Evas_Object* ewkView, Evas_Native_Surface* nativeSurface, const WebCore::IntRect& rect) > > +{ > > + EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, false); > Same... Should we also apply that to WK1? In ChromeClientEfl.cpp, many ewk_view_*() functions are called now.
No, let's leave webkit1 for now, unless for new code I suppose
Created attachment 170859 [details] patch from comment #20
> > Source/WebKit/efl/ewk/ewk_view.cpp:255 > > + bool compositingActive; > isCompositingActive Done. > > Source/WebKit/efl/ewk/ewk_view.cpp:960 > > + if (priv->compositingObject) > > + evas_object_del(priv->compositingObject); > use RefPtr they support evas objects Done. > > Source/WebKit/efl/ewk/ewk_view.cpp:4572 > > +void ewk_view_root_graphics_layer_set(Evas_Object* ewkView, WebCore::GraphicsLayer* rootLayer) > Why cant these be in the EwkViewImpl ? This patch is for WK1. > > Source/WebKit/efl/ewk/ewk_view.cpp:4577 > > + bool active = rootLayer ? true : false; > = !!rootLayer Done. Thanks you for your review.
Comment on attachment 170859 [details] patch from comment #20 Clearing flags on attachment: 170859 Committed r132615: <http://trac.webkit.org/changeset/132615>
All reviewed patches have been landed. Closing bug.