Bug 43882 – [Qt] Move the accelerated compositing build flag to the right place

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Noam Rosenthal]

WTF flags should be in Platform.h, not in WebCore.pro. This causes build issues on some platforms.

[Noam Rosenthal]

Created attachment 64169 [details]
Patch

[Ariya Hidayat]

Comment on attachment 64169 [details]
Patch

LGTM.

[WebKit Commit Bot]

Comment on attachment 64169 [details]
Patch

Rejecting patch 64169 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 20832 test cases.
fast/loader/recursive-before-unload-crash.html -> failed

Exiting early after 1 failures. 14235 tests run.
242.80s total testing time

14234 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
6 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/3739096

[Noam Rosenthal]

I can't really get how my change would have broken that layout test :)

[Csaba Osztrogonác]

(In reply to comment #4)
> I can't really get how my change would have broken that layout test :)

Your change is unrelated to Snow Leopard, it must be a flakey test.

[WebKit Commit Bot]

Comment on attachment 64169 [details]
Patch

Clearing flags on attachment: 64169

Committed r65502: <http://trac.webkit.org/changeset/65502>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Balazs Kelemen]

We are defining WTF_USE_ACCELERATED_COMPOSITING in WebKit.pri too (line 48):
greaterThan(QT_MINOR_VERSION, 5):DEFINES += WTF_USE_ACCELERATED_COMPOSITING

If it should be done in Platform.h then I think it should be done this way:
#if PLATFORM(QT)
#ifndef !defined(WTF_USE_ACCELERATED_COMPOSITING) && QT_VERSION_GREATER_THEN(4, 5, 0)
#define WTF_USE_ACCELERATED_COMPOSITING 1
#endif
#endif

With this the feature could be disabled at compile time and the version check would still exist. Do you agree?
(I am not sure that the QT_VERSION_GREATER_THEN macro exist but I know there is something like that.)

[Antonio Gomes]

(In reply to comment #8)
> We are defining WTF_USE_ACCELERATED_COMPOSITING in WebKit.pri too (line 48):
> greaterThan(QT_MINOR_VERSION, 5):DEFINES += WTF_USE_ACCELERATED_COMPOSITING

It sounds bogus now. Could you please file a bug?

> If it should be done in Platform.h then I think it should be done this way:
> #if PLATFORM(QT)
> #ifndef !defined(WTF_USE_ACCELERATED_COMPOSITING) && QT_VERSION_GREATER_THEN(4, 5, 0)
> #define WTF_USE_ACCELERATED_COMPOSITING 1
> #endif
> #endif
> 
> With this the feature could be disabled at compile time and the version check would still exist. Do you agree?
> (I am not sure that the QT_VERSION_GREATER_THEN macro exist but I know there is something like that.)

"#if QT_VERSION >= QT_VERSION_CHECK(4, 6, 0)"

Personally, I support moving all "#defined" dealed here to Platform.h

[Noam Rosenthal]

Reopening to accomodate the above comments

[Noam Rosenthal]

Created attachment 64676 [details]
Patch

[Antonio Gomes]

Comment on attachment 64676 [details]
Patch

Noam, the patch looks good. One major comment below though.

>  2010-08-17  Gavin Barraclough  <[e-mail]>
>  
>          Reviewed by Sam Weinig.
> diff --git a/JavaScriptCore/wtf/Platform.h b/JavaScriptCore/wtf/Platform.h
> index f653adb2c0a59e6add35b821d1ec139a2c38583a..78968a900d12df7728f1b258a9c546ab1d9ea2e5 100644
> --- a/JavaScriptCore/wtf/Platform.h
> +++ b/JavaScriptCore/wtf/Platform.h
> @@ -1052,7 +1052,7 @@ on MinGW. See https://bugs.webkit.org/show_bug.cgi?id=29268 */
>  #define WTF_USE_ACCELERATED_COMPOSITING 1
>  #endif
>  
> -#if PLATFORM(QT)
> +#if PLATFORM(QT) && !(defined(QT_VERSION) && QT_VERSION < 0x040600) && !defined(WTF_USE_ACCELERATED_COMPOSITING)
>  #define WTF_USE_ACCELERATED_COMPOSITING 1
>  #endif

in bug 43695, we moved away from using the  encoded hex for qt version check. could you keep the consistency?

The rest is fine.

[Andreas Kling]

@No'am: Ping, this has been in pending-commit for a month.

[Noam Rosenthal]

Created attachment 68398 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 68398 [details]
Patch

OK, I trust you on this one. So WTF_USE_ACCELERATED_COMPOSITING is set elsewhere now?

[Noam Rosenthal]

Yes, in the previous patch I moved it to Platform.h, where it belongs. This is just some follow-up cleanup.

[WebKit Commit Bot]

Comment on attachment 68398 [details]
Patch

Clearing flags on attachment: 68398

Committed r68067: <http://trac.webkit.org/changeset/68067>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.