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

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

Summary: [EFL][AC] Fix bugs preventing us from running layout tests with AC turned on

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	WebKit EFL (show other bugs)
Version:	528+ (Nightly build)
Hardware:	Unspecified Unspecified

Importance:	P2 Normal
Assignee:	Nobody

URL:
Keywords:

Depends on:
Blocks:

Reported:	2012-10-27 16:45 PDT by Yael
Modified:	2012-10-29 07:30 PDT (History)
CC List:	6 users (show)

See Also:

Attachments
Patch (3.51 KB, patch) 2012-10-28 20:33 PDT, Yael	kenneth: review+	Details \| Formatted Diff \| Diff
Patch for landing. (3.64 KB, patch) 2012-10-29 05:42 PDT, Yael	no flags	Details \| Formatted Diff \| Diff
Patch for landing. (3.68 KB, patch) 2012-10-29 06:16 PDT, Yael	no flags	Details \| Formatted Diff \| Diff
One more time . (3.68 KB, patch) 2012-10-29 06:29 PDT, Yael	no flags	Details \| Formatted Diff \| Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Yael 2012-10-27 16:45:47 PDT

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

Comment 1 Yael 2012-10-28 20:33:44 PDT

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.

Comment 2 Kenneth Rohde Christiansen 2012-10-28 23:10:39 PDT

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?

Comment 3 Yael 2012-10-29 05:42:38 PDT

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.

Comment 4 Chris Dumez 2012-10-29 05:49:14 PDT

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.

Comment 5 Gyuyoung Kim 2012-10-29 05:58:43 PDT

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.

Comment 6 Yael 2012-10-29 06:13:56 PDT

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

Comment 7 Yael 2012-10-29 06:16:07 PDT

Created attachment 171219 [details]
Patch for landing.

Address comment #4 and comment #5 .

Comment 8 Gyuyoung Kim 2012-10-29 06:20:43 PDT

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 ?

Comment 9 Yael 2012-10-29 06:27:25 PDT

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

Comment 10 Yael 2012-10-29 06:29:22 PDT

Created attachment 171227 [details]
One more time .

Comment 11 Dominik Röttsches (drott) 2012-10-29 06:47:58 PDT

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

Comment 12 Gyuyoung Kim 2012-10-29 07:03:21 PDT

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

Comment 13 Yael 2012-10-29 07:07:12 PDT

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

Comment 14 Dominik Röttsches (drott) 2012-10-29 07:22:05 PDT

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

Comment 15 Gyuyoung Kim 2012-10-29 07:25:36 PDT

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.

Comment 16 WebKit Review Bot 2012-10-29 07:30:09 PDT

Comment on attachment 171227 [details]
One more time .

Clearing flags on attachment: 171227

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

Comment 17 WebKit Review Bot 2012-10-29 07:30:14 PDT

All reviewed patches have been landed.  Closing bug.