Bug 100598 – [EFL][AC] Fix bugs preventing us from running layout tests with AC turned on

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

[Yael]

Right now, all the tests crash when AC is turned on.
If there are many issues, this bug should become a meta bug.

[Yael]

Created attachment 171148 [details]
Patch

Make sure to use opengl_x11 engine when AC is turned on and X11 is in use.
We cannot create a gl context otherwise.

[Kenneth Rohde Christiansen]

Comment on attachment 171148 [details]
Patch

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

> Tools/EWebLauncher/main.c:813
>      app->ee = ecore_evas_new(userArgs->engine, 0, 0, userArgs->geometry.w, userArgs->geometry.h, NULL);

shouldn't that be indented now?

> Tools/WebKitTestRunner/efl/PlatformWebViewEfl.cpp:39
>  static Ecore_Evas* initEcoreEvas()
>  {
> +#ifdef WTF_USE_ACCELERATED_COMPOSITING && defined HAVE_ECORE_X
> +    const char* engine = "opengl_x11";
> +    Ecore_Evas* ecoreEvas = ecore_evas_new(engine, 0, 0, 800, 600, 0);
> +#else
>      Ecore_Evas* ecoreEvas = ecore_evas_new(0, 0, 0, 800, 600, 0);
> +#endif
>      if (!ecoreEvas)

I would prefer

const char* engine = 0;

#ifdef WTF_USE_ACCELERATED_COMPOSITING && defined HAVE_ECORE_X
engine = "opengl_x11";
#endif

Ecore_Evas* ecoreEvas = ecore_evas_new(engine, 0, 0, 800, 600, 0);

> Tools/DumpRenderTree/efl/DumpRenderTree.cpp:427
> +#ifdef WTF_USE_ACCELERATED_COMPOSITING && defined HAVE_ECORE_X
> +    const char* engine = "opelgl_x11";
> +    Ecore_Evas* ecoreEvas = ecore_evas_new(engine, 0, 0, 800, 600, 0);
> +#else
>      Ecore_Evas* ecoreEvas = ecore_evas_new(0, 0, 0, 800, 600, 0);
> +#endif

so much copied code, can't we concentrate it?

[Yael]

Created attachment 171211 [details]
Patch for landing.

Take comment 2 into account.
I don't see how to share code between DumpRenderTree and WebKitTestRunner, so did not combine the code.

[Chris Dumez]

Comment on attachment 171211 [details]
Patch for landing.

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

> Tools/EWebLauncher/main.c:810
> +#ifdef WTF_USE_ACCELERATED_COMPOSITING && defined HAVE_ECORE_X

I feel this is more readable:
#if defined(WTF_USE_ACCELERATED_COMPOSITING) && defined(HAVE_ECORE_X)

It looks odd to mix #ifdef and defined. It also decreases readability to use "defined" without parenthesis.

[Gyuyoung Kim]

Comment on attachment 171211 [details]
Patch for landing.

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

> Tools/DumpRenderTree/efl/DumpRenderTree.cpp:422
> +const char* engine = 0;

Nit : wrong indentation.

[Yael]

(In reply to comment #5)
> (From update of attachment 171211 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171211&action=review
> 
> > Tools/DumpRenderTree/efl/DumpRenderTree.cpp:422
> > +const char* engine = 0;
> 
> Nit : wrong indentation.

Oops :) 
The stylebot should have caught it.

[Yael]

Created attachment 171219 [details]
Patch for landing.

Address comment #4 and comment #5 .

[Gyuyoung Kim]

Comment on attachment 171219 [details]
Patch for landing.

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

> Tools/WebKitTestRunner/efl/PlatformWebViewEfl.cpp:33
> +const char* engine = 0;

ditto ?

[Yael]

(In reply to comment #8)
> (From update of attachment 171219 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171219&action=review
> 
> > Tools/WebKitTestRunner/efl/PlatformWebViewEfl.cpp:33
> > +const char* engine = 0;
> 
> ditto ?

Fixed but didn't save :(

[Yael]

Created attachment 171227 [details]
One more time .

[Dominik Röttsches (drott)]

(In reply to comment #4)
> (From update of attachment 171211 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171211&action=review
> 
> > Tools/EWebLauncher/main.c:810
> > +#ifdef WTF_USE_ACCELERATED_COMPOSITING && defined HAVE_ECORE_X
> 
> I feel this is more readable:
> #if defined(WTF_USE_ACCELERATED_COMPOSITING) && defined(HAVE_ECORE_X)

While we're at it, and I don't mean to halt the cq+, but shouldn't the first part be 
#if USE(ACCELERATED_COMPOSITING)?

[Gyuyoung Kim]

(In reply to comment #11)
> (In reply to comment #4)
> > (From update of attachment 171211 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=171211&action=review
> > 
> > > Tools/EWebLauncher/main.c:810
> > > +#ifdef WTF_USE_ACCELERATED_COMPOSITING && defined HAVE_ECORE_X
> > 
> > I feel this is more readable:
> > #if defined(WTF_USE_ACCELERATED_COMPOSITING) && defined(HAVE_ECORE_X)
> 
> While we're at it, and I don't mean to halt the cq+, but shouldn't the first part be 
> #if USE(ACCELERATED_COMPOSITING)?

As far as I know, we have to use USE(XXX) macro in WebKit inside because this macro is defined in Platform.h file.  So, it seems to me that Tools can't use this.  However, if Tools can use this, I think it is better to use USE() macro. To my quick search, Tools can use it. Yael, could you check this ?

[Yael]

(In reply to comment #11)
> (In reply to comment #4)
> > (From update of attachment 171211 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=171211&action=review
> > 
> > > Tools/EWebLauncher/main.c:810
> > > +#ifdef WTF_USE_ACCELERATED_COMPOSITING && defined HAVE_ECORE_X
> > 
> > I feel this is more readable:
> > #if defined(WTF_USE_ACCELERATED_COMPOSITING) && defined(HAVE_ECORE_X)
> 
> While we're at it, and I don't mean to halt the cq+, but shouldn't the first part be 
> #if USE(ACCELERATED_COMPOSITING)?

AFAIK USE() macro is defined only internally to webkit.
EWebLauncher is not part of webkit itself and so the USE() macro is not defined.

[Dominik Röttsches (drott)]

(In reply to comment #13)
> (In reply to comment #11)
> > (In reply to comment #4)
> > > (From update of attachment 171211 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=171211&action=review
> > > 
> > > > Tools/EWebLauncher/main.c:810
> > > > +#ifdef WTF_USE_ACCELERATED_COMPOSITING && defined HAVE_ECORE_X
> > > 
> > > I feel this is more readable:
> > > #if defined(WTF_USE_ACCELERATED_COMPOSITING) && defined(HAVE_ECORE_X)
> > 
> > While we're at it, and I don't mean to halt the cq+, but shouldn't the first part be 
> > #if USE(ACCELERATED_COMPOSITING)?
> 
> AFAIK USE() macro is defined only internally to webkit.
> EWebLauncher is not part of webkit itself and so the USE() macro is not defined.

Alright, thanks for checking. Sorry for the noise then.

[Gyuyoung Kim]

Comment on attachment 171227 [details]
One more time .

To my quick search, USE() is being used in Tools as well. However, I prefer to use USE() in WebKit inside only. cq+ again.

[WebKit Review Bot]

Comment on attachment 171227 [details]
One more time .

Clearing flags on attachment: 171227

Committed r132799: <http://trac.webkit.org/changeset/132799>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.