Bug 130834 - Move UseJSC.cmake back to CMakeLists.txt
Summary: Move UseJSC.cmake back to CMakeLists.txt
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 114131 (view as bug list)
Depends on:
Blocks:
 
Reported: 2014-03-27 06:13 PDT by Éva Balázsfalvi
Modified: 2015-02-26 06:57 PST (History)
9 users (show)

See Also:


Attachments
Patch (29.63 KB, patch)
2014-03-27 06:28 PDT, Éva Balázsfalvi
no flags Details | Formatted Diff | Diff
Patch (22.96 KB, patch)
2014-04-10 01:25 PDT, Éva Balázsfalvi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Éva Balázsfalvi 2014-03-27 06:13:06 PDT
Long time ago JSC related stuff were moved to a separated
UseJSC.cmake from CMakeLists.txt to be able build with
V8 instead of JSC - http://trac.webkit.org/changeset/81515

After Blink fork, JSC is the only one JavaScript engine in WebKit,
so UseJSC.cmake can be moved back to CMakeLists.txt
Comment 1 Éva Balázsfalvi 2014-03-27 06:28:12 PDT
Created attachment 227943 [details]
Patch
Comment 2 Gyuyoung Kim 2014-03-27 18:08:37 PDT
Personally I prefer to keep UseJSC.cmake in order to reduce CMakeLists.txt workload. Besides someone might to wanna use V8 in future again. However, many people wanna back to it to CMakeLists.txt, I won't object to back to there.
Comment 3 Csaba Osztrogonác 2014-04-03 04:22:36 PDT
Comment on attachment 227943 [details]
Patch

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

I agree with this change, but r- now due to some minor things.

> Source/WebCore/CMakeLists.txt:2838
> +    list(APPEND WebCore_SOURCES
> +        bindings/js/JSCustomSQLStatementErrorCallback.cpp
> +        bindings/js/JSSQLResultSetRowListCustom.cpp
> +        bindings/js/JSSQLTransactionCustom.cpp
> +        bindings/js/JSSQLTransactionSyncCustom.cpp
> +    )

Please move them after Modules/webdatabase/WorkerGlobalScopeWebDatabase.cpp - few lines above.

> Source/WebCore/CMakeLists.txt:2875
> +        bindings/js/JSWebGLRenderingContextCustom.cpp

We don't need one more APPEND, just add it before html/canvas/ANGLEInstancedArrays.cpp.

> Source/WebCore/CMakeLists.txt:2976
> +        bindings/js/JSAudioTrackCustom.cpp
> +        bindings/js/JSAudioTrackListCustom.cpp
> +        bindings/js/JSTextTrackCueCustom.cpp
> +        bindings/js/JSTextTrackCustom.cpp
> +        bindings/js/JSTextTrackListCustom.cpp
> +        bindings/js/JSTrackCustom.cpp
> +        bindings/js/JSTrackEventCustom.cpp
> +        bindings/js/JSVideoTrackCustom.cpp
> +        bindings/js/JSVideoTrackListCustom.cpp
> +    )
> +
> +    list(APPEND WebCore_SOURCES

ditto, just add them before html/track/AudioTrack.cpp

> Source/WebCore/CMakeLists.txt:-3193
> -
> -include(${WEBCORE_DIR}/UseJSC.cmake)
> -

I prefer moving the line 285 - end part of UseJSC.cmake
to this place instead of splitting into several pieces.
Comment 4 Csaba Osztrogonác 2014-04-03 04:32:01 PDT
(In reply to comment #2)
> Personally I prefer to keep UseJSC.cmake in order to reduce CMakeLists.txt workload. Besides someone might to wanna use V8 in future again. However, many people wanna back to it to CMakeLists.txt, I won't object to back to there.

This patch doesn't change workload at all, now CMakeLists.txt 
simple includes UseJSC.cmake unconditionally. 

After the Blink fork the probability of adding back V8 to WebKit as 
JS engine is near 0%, I'm sure Apple would have strong objection 
against it if somebody dreamt about V8 again. :)

I think having only one cmake file for WebCore reduces the maintenance cost.
Comment 5 Gyuyoung Kim 2014-04-03 05:49:43 PDT
(In reply to comment #4)
> (In reply to comment #2)
> > Personally I prefer to keep UseJSC.cmake in order to reduce CMakeLists.txt workload. Besides someone might to wanna use V8 in future again. However, many people wanna back to it to CMakeLists.txt, I won't object to back to there.
> 
> This patch doesn't change workload at all, now CMakeLists.txt 
> simple includes UseJSC.cmake unconditionally. 
> 
> After the Blink fork the probability of adding back V8 to WebKit as 
> JS engine is near 0%, I'm sure Apple would have strong objection 
> against it if somebody dreamt about V8 again. :)
> 
> I think having only one cmake file for WebCore reduces the maintenance cost.

If so, I don't wanna object to back to CMakeLists.txt. :)
Comment 6 Éva Balázsfalvi 2014-04-09 08:27:11 PDT
I've realized, that UseJSC.cmake contained a lot of unnecessary conditions and duplicated files, so I've removed them before continuing to work on this merge issue.

Bug report of mentioned issue: https://bugs.webkit.org/show_bug.cgi?id=131438
Comment 7 Éva Balázsfalvi 2014-04-10 01:25:17 PDT
Created attachment 229030 [details]
Patch
Comment 8 Csaba Osztrogonác 2014-04-10 05:40:39 PDT
Comment on attachment 229030 [details]
Patch

LGTM
Comment 9 WebKit Commit Bot 2014-04-10 06:10:31 PDT
Comment on attachment 229030 [details]
Patch

Clearing flags on attachment: 229030

Committed r167071: <http://trac.webkit.org/changeset/167071>
Comment 10 WebKit Commit Bot 2014-04-10 06:10:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Csaba Osztrogonác 2015-02-26 06:57:55 PST
*** Bug 114131 has been marked as a duplicate of this bug. ***