Bug 43882 - [Qt] Move the accelerated compositing build flag to the right place
Summary: [Qt] Move the accelerated compositing build flag to the right place
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2010-08-11 15:20 PDT by Noam Rosenthal
Modified: 2010-09-22 12:18 PDT (History)
4 users (show)

See Also:


Attachments
Patch (4.37 KB, patch)
2010-08-11 15:25 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (2.08 KB, patch)
2010-08-18 01:59 PDT, Noam Rosenthal
tonikitoo: review+
tonikitoo: commit-queue-
Details | Formatted Diff | Diff
Patch (824 bytes, patch)
2010-09-22 11:12 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Noam Rosenthal 2010-08-11 15:20:31 PDT
WTF flags should be in Platform.h, not in WebCore.pro. This causes build issues on some platforms.
Comment 1 Noam Rosenthal 2010-08-11 15:25:34 PDT
Created attachment 64169 [details]
Patch
Comment 2 Ariya Hidayat 2010-08-12 09:36:26 PDT
Comment on attachment 64169 [details]
Patch

LGTM.
Comment 3 WebKit Commit Bot 2010-08-12 13:41:41 PDT
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
Comment 4 Noam Rosenthal 2010-08-12 14:00:27 PDT
I can't really get how my change would have broken that layout test :)
Comment 5 Csaba Osztrogonác 2010-08-17 05:00:03 PDT
(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.
Comment 6 WebKit Commit Bot 2010-08-17 07:54:56 PDT
Comment on attachment 64169 [details]
Patch

Clearing flags on attachment: 64169

Committed r65502: <http://trac.webkit.org/changeset/65502>
Comment 7 WebKit Commit Bot 2010-08-17 07:55:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Balazs Kelemen 2010-08-17 16:16:28 PDT
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.)
Comment 9 Antonio Gomes 2010-08-17 20:21:29 PDT
(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
Comment 10 Noam Rosenthal 2010-08-18 01:57:03 PDT
Reopening to accomodate the above comments
Comment 11 Noam Rosenthal 2010-08-18 01:59:50 PDT
Created attachment 64676 [details]
Patch
Comment 12 Antonio Gomes 2010-08-18 05:16:30 PDT
Comment on attachment 64676 [details]
Patch

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

>  2010-08-17  Gavin Barraclough  <barraclough@apple.com>
>  
>          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.
Comment 13 Andreas Kling 2010-09-17 19:39:52 PDT
@No'am: Ping, this has been in pending-commit for a month.
Comment 14 Noam Rosenthal 2010-09-22 11:12:06 PDT
Created attachment 68398 [details]
Patch
Comment 15 Kenneth Rohde Christiansen 2010-09-22 11:42:08 PDT
Comment on attachment 68398 [details]
Patch

OK, I trust you on this one. So WTF_USE_ACCELERATED_COMPOSITING is set elsewhere now?
Comment 16 Noam Rosenthal 2010-09-22 11:50:07 PDT
Yes, in the previous patch I moved it to Platform.h, where it belongs. This is just some follow-up cleanup.
Comment 17 WebKit Commit Bot 2010-09-22 12:18:40 PDT
Comment on attachment 68398 [details]
Patch

Clearing flags on attachment: 68398

Committed r68067: <http://trac.webkit.org/changeset/68067>
Comment 18 WebKit Commit Bot 2010-09-22 12:18:46 PDT
All reviewed patches have been landed.  Closing bug.