[Webkit-unassigned] [Bug 91581] [WK2][EFL] Add an ACCELERATED_COMPOSITING implementation for Efl WebKit2

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=91581





--- Comment #5 from YoungTaeck Song <youngtaeck.song at samsung.com>  2012-07-18 19:08:18 PST ---
(In reply to comment #3)

Thank you for kind review.

> (From update of attachment 152925 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152925&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:76
> >      ~_Ewk_View_Private_Data()
> 
> You probably need Evas_GL clean up code in the destructor. I'm assuming the view can be destroyed without ewk_view_exit_accelerated_compositing_mode() being called beforehand.

When the view is destroyed, ewk_view_exit_accelerated_compositing_mode is called like below.
~_Ewk_View_Private_Data() => ~PageClientImpl() => ~WebPageProxy() => ~DrawingAreaProxyImpl()
=> WebPageProxy::exitAcceleratedCompositingMode => PageClientImpl::exitAcceleratedCompositingMode
=> ewk_view_exit_accelerated_compositing_mode

> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:383
> > +IntSize ewk_view_size_get(Evas_Object* ewkView)
> 
> argument should be const.
> 

fixed.

> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:403
> > +
> 
> How about an ASSERT(!priv->evasGlSurface); to make sure we don't leak memory?
> 

fixed.

> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:415
> > +void ewk_view_enter_accelerated_compositing_mode(Evas_Object* ewkView)
> 
> How about returning a bool so that the client know if it was successful?
> 
fixed.

> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:421
> > +    priv->evasGl = evas_gl_new(evas);
> 
> I think we need an EINA_SAFETY check to make sure priv->evasGl is NULL when this function is called. What if the client calls this function several times without calling the ewk_view_exit_accelerated_compositing_mode()? We should print a warning instead of leaking memory.
> 

fixed.

> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:441
> > +void ewk_view_exit_accelerated_compositing_mode(Evas_Object* ewkView)
> 
> How about returning a bool so that the client know if it was successful?
> 

fixed.

> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:446
> > +    if (!priv->evasGl)
> 
> Should it be an if check or an EINA_SAFETY? I guess this mean this means the client called ewk_view_exit_accelerated_compositing_mode() without calling ewk_view_enter_accelerated_compositing_mode() first, which would be wrong API usage. Also, if ewk_view_enter_accelerated_compositing_mode() starts returning a boolean, the client has not excuse.
> 

fixed.

> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:483
> > +        evas_gl_surface_destroy(priv->evasGl, priv->evasGlSurface);
> 
> I would feel better with a "priv->evasGlSurface = 0;" right after this line. I know it is not strictly needed but we might experience issues later if the ewk_view_create_gl_surface() starts checking the value of priv->evasGlSurface.
> 

fixed.

> > Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:72
> > +WebCore::IntSize ewk_view_size_get(Evas_Object* ewkView);
> 
> Argument should be const.
> 

fixed.

> > Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:75
> > +void ewk_view_enter_accelerated_compositing_mode(Evas_Object* ewkView);
> 
> the verb is usually at the end: "ewk_view_accelerated_compositing_mode_enter()".
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:76
> > +void ewk_view_exit_accelerated_compositing_mode(Evas_Object* ewkView);
> 
> Ditto.
> 

fixed.

> > Tools/ChangeLog:8
> > +        This patch is a subset of Efl's UI_SIDE_COMPOSITING implementation.
> 
> This changelog should reflect the actual change to the MiniBrowser instead of being so generic.

fixed.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.