[EFL][WK2] Regression (r128163)

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Regina Chung]

While removing compile warnings r128163 changed the logic of code for entering accelerated compositing mode, resulting in never being able to enter it.

[Regina Chung]

Created attachment 163800 [details]
Patch

[Regina Chung]

Comment on attachment 163800 [details]
Patch

Fixes regression introduced by r128163 while removing compile warnings as intended in r128163.

[Gyuyoung Kim]

Comment on attachment 163800 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:501
> +    EINA_SAFETY_ON_FALSE_RETURN_VAL(!priv->evasGl, false);

Is below code is more readable instead of above one?

EINA_SAFETY_ON_TRUE_RETURN_VAL(priv->evasGl, false);

[Regina Chung]

(In reply to comment #3)
> (From update of attachment 163800 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163800&action=review
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:501
> > +    EINA_SAFETY_ON_FALSE_RETURN_VAL(!priv->evasGl, false);
> Is below code is more readable instead of above one?
> EINA_SAFETY_ON_TRUE_RETURN_VAL(priv->evasGl, false);

Using EINA_SAFETY_ON_TRUE_RETURN_VAL(priv->evasGl, false); doesn't compile.
If we want to use EINA_SAFETY_ON_TRUE_RETURN_VAL(), we have to put !! in front of priv->evasGl.
Which one should I use?
EINA_SAFETY_ON_FALSE_RETURN_VAL(!priv->evasGl, false); or
EINA_SAFETY_ON_TRUE_RETURN_VAL(!!priv->evasGl, false);

[Gyuyoung Kim]

(In reply to comment #4)

> Using EINA_SAFETY_ON_TRUE_RETURN_VAL(priv->evasGl, false); doesn't compile.
> If we want to use EINA_SAFETY_ON_TRUE_RETURN_VAL(), we have to put !! in front of priv->evasGl.
> Which one should I use?
> EINA_SAFETY_ON_FALSE_RETURN_VAL(!priv->evasGl, false); or
> EINA_SAFETY_ON_TRUE_RETURN_VAL(!!priv->evasGl, false);

Something is strange. I succeed to compile with below code in my linux box.

EINA_SAFETY_ON_TRUE_RETURN_VAL(priv->evasGl, false);

Could you check this again ?

[Regina Chung]

(In reply to comment #5)
> (In reply to comment #4)
> > Using EINA_SAFETY_ON_TRUE_RETURN_VAL(priv->evasGl, false); doesn't compile.
> > If we want to use EINA_SAFETY_ON_TRUE_RETURN_VAL(), we have to put !! in front of priv->evasGl.
> > Which one should I use?
> > EINA_SAFETY_ON_FALSE_RETURN_VAL(!priv->evasGl, false); or
> > EINA_SAFETY_ON_TRUE_RETURN_VAL(!!priv->evasGl, false);
> Something is strange. I succeed to compile with below code in my linux box.
> EINA_SAFETY_ON_TRUE_RETURN_VAL(priv->evasGl, false);
> Could you check this again ?

You have to build with --tiled-backing-store option to include that code :)

[Gyuyoung Kim]

Comment on attachment 163800 [details]
Patch

Frankly, EINA_SAFETY_ON_NULL_RETURN_VAL is correct I think. But, it was changed due to compilation warning. So, if you need to check if priv->evasGl is null, why don't you use *if* condition explicitly ?

if (priv->evasGl) {
    EINA_LOG_DOM_WARN(_ewk_log_dom, "bla bla");
    return false;
}

[Regina Chung]

Created attachment 164479 [details]
Patch

[Regina Chung]

(In reply to comment #7)
> (From update of attachment 163800 [details])
> Frankly, EINA_SAFETY_ON_NULL_RETURN_VAL is correct I think. But, it was changed due to compilation warning. So, if you need to check if priv->evasGl is null, why don't you use *if* condition explicitly ?
> if (priv->evasGl) {
>     EINA_LOG_DOM_WARN(_ewk_log_dom, "bla bla");
>     return false;
> }

I agree that EINA_SAFETY_ON_NULL_RETURN_VAL is correct. Changed the code as you suggested.

[WebKit Review Bot]

Comment on attachment 164479 [details]
Patch

Clearing flags on attachment: 164479

Committed r128844: <http://trac.webkit.org/changeset/128844>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Comment on attachment 164479 [details]
Patch

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

> Source/WebKit2/ChangeLog:3
> +        [EFL][WK2] Regression (r128163)

Please add a title after Regression (r128163):

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:-501
> -    EINA_SAFETY_ON_NULL_RETURN_VAL(priv->evasGl, false);

EINA_SAFETY_ON_TRUE_RETURN_VAL(priv->evasGl, false); ?

[Gyuyoung Kim]

(In reply to comment #12)
> (From update of attachment 164479 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164479&action=review
> 
> > Source/WebKit2/ChangeLog:3
> > +        [EFL][WK2] Regression (r128163)
> 
> Please add a title after Regression (r128163):
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:-501
> > -    EINA_SAFETY_ON_NULL_RETURN_VAL(priv->evasGl, false);
> 
> EINA_SAFETY_ON_TRUE_RETURN_VAL(priv->evasGl, false); ?

As previous comments mentioned, EINA_SAFETY_ON_TRUE_RETURN_VAL(priv->evasGl, false) makes a below build break with --tiled-backing-store option.

/home/gyuyoung/webkit/WebKit/Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:501:5: error: invalid conversion from ‘Evas_GL* {aka _Evas_GL*}’ to ‘long int’ [-fpermissive]

[Chris Dumez]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 164479 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=164479&action=review
> > 
> > > Source/WebKit2/ChangeLog:3
> > > +        [EFL][WK2] Regression (r128163)
> > 
> > Please add a title after Regression (r128163):
> > 
> > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:-501
> > > -    EINA_SAFETY_ON_NULL_RETURN_VAL(priv->evasGl, false);
> > 
> > EINA_SAFETY_ON_TRUE_RETURN_VAL(priv->evasGl, false); ?
> 
> As previous comments mentioned, EINA_SAFETY_ON_TRUE_RETURN_VAL(priv->evasGl, false) makes a below build break with --tiled-backing-store option.
> 
> /home/gyuyoung/webkit/WebKit/Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:501:5: error: invalid conversion from ‘Evas_GL* {aka _Evas_GL*}’ to ‘long int’ [-fpermissive]

Maybe EINA_SAFETY_ON_TRUE_RETURN_VAL(!!priv->evasGl, false); would have worked then?

[Gyuyoung Kim]

(In reply to comment #14)
 /home/gyuyoung/webkit/WebKit/Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:501:5: error: invalid conversion from ‘Evas_GL* {aka _Evas_GL*}’ to ‘long int’ [-fpermissive]
> 
> Maybe EINA_SAFETY_ON_TRUE_RETURN_VAL(!!priv->evasGl, false); would have worked then?

As I said comment #7, I think EINA_SAFETY_ON_NULL_RETURN_VAL is more proper because this is for checking if priv->evasGL is not null. Generally, it looks EINA_SAFETY_ON_TRUE | FALSE_RETURN_VAL() are used for checking specific condition rather than checking if null. So, I thought that explicit *if* condition is better than EINA_SAFETY_ON_TRUE | FALSE_RETURN_VAL for null checking.