[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 #3 from Christophe Dumez <christophe.dumez at intel.com>  2012-07-17 22:57:27 PST ---
(From update of attachment 152925)
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.

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

argument should be const.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:403
> +

How about an ASSERT(!priv->evasGlSurface); to make sure we don't leak memory?

> 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?

> 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.

> 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?

> 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.

> 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.

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

Argument should be const.

> 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.

> 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.

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