Bug 91581 – [WK2][EFL] Add an ACCELERATED_COMPOSITING implementation for Efl WebKit2

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[YoungTaeck Song]

Created attachment 152921 [details]
patch

This patch is a subset of Efl's UI_SIDE_COMPOSITING implementation.
When enter accelerated compositing mode, create evas_gl, evas_gl_context, and evas_gl_surface.

[WebKit Review Bot]

Attachment 152921 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Tools/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[YoungTaeck Song]

Created attachment 152925 [details]
patch

miss bug number

[Chris Dumez]

Comment on attachment 152925 [details]
patch

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.

[YoungTaeck Song]

Created attachment 153157 [details]
Patch

[YoungTaeck Song]

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

[Gyuyoung Kim]

Comment on attachment 153157 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:393
> +    EWK_VIEW_SD_GET(ewkView, smartData);

Use EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, 0);

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:394
> +    EWK_VIEW_PRIV_GET(smartData, priv);

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:404
> +    ASSERT(!priv->evasGlSurface);

Though below condition check this again, should we use this ?

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:416
> +bool ewk_view_accelerated_compositing_mode_enter(Evas_Object* ewkView)

If possible, it is good to use *const* keyword.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:418
> +    EWK_VIEW_SD_GET(ewkView, smartData);

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:419
> +    EWK_VIEW_PRIV_GET(smartData, priv);

ditto.

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

Add an empty line.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:446
> +bool ewk_view_accelerated_compositing_mode_exit(Evas_Object* ewkView)

If possible, it is good to use *const* keyword.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:448
> +    EWK_VIEW_SD_GET(ewkView, smartData);

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:449
> +    EWK_VIEW_PRIV_GET(smartData, priv);

ditto.

> Tools/MiniBrowser/efl/main.c:51
> +    "GPL",

AFAIK, WebKit is based on LGPL and Apple BSD. So, LGPL is more proper.

> Tools/MiniBrowser/efl/main.c:224
> +    ecore_app_args_set(argc, (const char**) argv);

s/(const char**)/(const char **)/g

[YoungTaeck Song]

Created attachment 153959 [details]
Patch

[YoungTaeck Song]

(In reply to comment #6)

Thanks for kind review.

> (From update of attachment 153157 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153157&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:393
> > +    EWK_VIEW_SD_GET(ewkView, smartData);
> 
> Use EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, 0);

fixed.

> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:394
> > +    EWK_VIEW_PRIV_GET(smartData, priv);
> 
> ditto.

fixed.

> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:404
> > +    ASSERT(!priv->evasGlSurface);
> 
> Though below condition check this again, should we use this ?
It's Christophe Dumez's suggestion. 
It's for memory leak when the client calls this function several times.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:416
> > +bool ewk_view_accelerated_compositing_mode_enter(Evas_Object* ewkView)
> 
> If possible, it is good to use *const* keyword.
fixed.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:418
> > +    EWK_VIEW_SD_GET(ewkView, smartData);
> 
> ditto.
fixed.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:419
> > +    EWK_VIEW_PRIV_GET(smartData, priv);
> 
> ditto.
fixed.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:442
> > +    }
> 
> Add an empty line.
fixed.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:446
> > +bool ewk_view_accelerated_compositing_mode_exit(Evas_Object* ewkView)
> 
> If possible, it is good to use *const* keyword.
fixed.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:448
> > +    EWK_VIEW_SD_GET(ewkView, smartData);
> 
> ditto.
fixed.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:449
> > +    EWK_VIEW_PRIV_GET(smartData, priv);
> 
> ditto.
fixed.
> 
> > Tools/MiniBrowser/efl/main.c:51
> > +    "GPL",
> 
> AFAIK, WebKit is based on LGPL and Apple BSD. So, LGPL is more proper.
fixed.
> 
> > Tools/MiniBrowser/efl/main.c:224
> > +    ecore_app_args_set(argc, (const char**) argv);
> 
> s/(const char**)/(const char **)/g
fixed.

[YoungTaeck Song]

Created attachment 153970 [details]
Patch

[YoungTaeck Song]

Created attachment 154626 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 154626 [details]
Patch

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

> Source/WebKit2/Shared/efl/LayerTreeContextEfl.cpp:2
> +    Copyright (C) 2012 Samsung Electronics

I was told BSD license is more useful. So, could you change LGPL with BSD ?

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:434
> +    EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, 0);

Change 0 with false.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:435
> +    EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv, 0);

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:459
> +    EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, 0);

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:460
> +    EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv, 0);

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:490
> +    EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, 0);

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:491
> +    EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv, 0);

ditto.

[YoungTaeck Song]

Created attachment 154860 [details]
Patch

[YoungTaeck Song]

(In reply to comment #11)
Thanks for kind review.

> (From update of attachment 154626 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154626&action=review
> 
> > Source/WebKit2/Shared/efl/LayerTreeContextEfl.cpp:2
> > +    Copyright (C) 2012 Samsung Electronics
> 
> I was told BSD license is more useful. So, could you change LGPL with BSD ?
fixed.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:434
> > +    EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, 0);
> 
> Change 0 with false.
fixed.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:435
> > +    EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv, 0);
> 
> ditto.
fixed.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:459
> > +    EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, 0);
> 
> ditto.
fixed.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:460
> > +    EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv, 0);
> 
> ditto.
fixed.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:490
> > +    EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, 0);
> 
> ditto.
fixed.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:491
> > +    EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv, 0);
> 
> ditto.
fixed.

[Gyuyoung Kim]

Comment on attachment 154860 [details]
Patch

As a step to support UI_SIDE_COMPOSITING for EFL port, looks good to me now.

[WebKit Review Bot]

Comment on attachment 154860 [details]
Patch

Clearing flags on attachment: 154860

Committed r123872: <http://trac.webkit.org/changeset/123872>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.