Bug 130834 – Move UseJSC.cmake back to CMakeLists.txt

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Éva Balázsfalvi]

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

[Éva Balázsfalvi]

Created attachment 227943 [details]
Patch

[Gyuyoung Kim]

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.

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

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

[Gyuyoung Kim]

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

[Éva Balázsfalvi]

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

[Éva Balázsfalvi]

Created attachment 229030 [details]
Patch

[Csaba Osztrogonác]

Comment on attachment 229030 [details]
Patch

LGTM

[WebKit Commit Bot]

Comment on attachment 229030 [details]
Patch

Clearing flags on attachment: 229030

Committed r167071: <http://trac.webkit.org/changeset/167071>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

*** Bug 114131 has been marked as a duplicate of this bug. ***