WTF flags should be in Platform.h, not in WebCore.pro. This causes build issues on some platforms.
Created attachment 64169 [details] Patch
Comment on attachment 64169 [details] Patch LGTM.
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
I can't really get how my change would have broken that layout test :)
(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 on attachment 64169 [details] Patch Clearing flags on attachment: 64169 Committed r65502: <http://trac.webkit.org/changeset/65502>
All reviewed patches have been landed. Closing bug.
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.)
(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
Reopening to accomodate the above comments
Created attachment 64676 [details] Patch
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.
@No'am: Ping, this has been in pending-commit for a month.
Created attachment 68398 [details] Patch
Comment on attachment 68398 [details] Patch OK, I trust you on this one. So WTF_USE_ACCELERATED_COMPOSITING is set elsewhere now?
Yes, in the previous patch I moved it to Platform.h, where it belongs. This is just some follow-up cleanup.
Comment on attachment 68398 [details] Patch Clearing flags on attachment: 68398 Committed r68067: <http://trac.webkit.org/changeset/68067>