Bug 53253 – Move wince/mt19937ar.c to ThirdParty and make it a policy choice

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Daniel Bates]

We should make the decision to use the Merseene Twister pseudorandom number generator a policy choice and move the implementation of the Merseene Twister algorithm, in file wince/mt19937ar.c, to  common location, say ThirdParty.

Currently, the choice to use this pseudorandom number generator is dependent on whether we are building JavaScriptCore on Windows CE. Instead, we should make this a policy choice and enable this by default in the Windows CE port.

[Daniel Bates]

Created attachment 80361 [details]
Patch

I thought to move the file wince/mt19937ar.c to Source/ThirdParty since it is a third party library. I am open to suggestions on where to place this file as well as the macro define to use.

I am not very familiar with CMake and the Windows CE port so I am unsure of the correctness of the Cmake change. I used WebCore/CMakeLists.txt and JavaScriptCore/CMakeLists.txt as references.

[Patrick R. Gansterer]

Comment on attachment 80361 [details]
Patch

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

> Source/JavaScriptCore/CMakeListsWinCE.txt:3
> +LIST(APPEND JavaScriptCore_INCLUDE_DIRECTORIES
> +    "${CMAKE_SOURCE_DIR}/Source/ThirdParty"
> +)

${CMAKE_SOURCE_DIR} is already <root>/Source, so it should read "${CMAKE_SOURCE_DIR}/ThirdParty"

I'll try this change on my local build, because I'm not sure if it's the correct "*_INCLUDE_DIRECTORIES"

[Daniel Bates]

(In reply to comment #2)
> (From update of attachment 80361 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80361&action=review
> 
> > Source/JavaScriptCore/CMakeListsWinCE.txt:3
> > +LIST(APPEND JavaScriptCore_INCLUDE_DIRECTORIES
> > +    "${CMAKE_SOURCE_DIR}/Source/ThirdParty"
> > +)
> 
> ${CMAKE_SOURCE_DIR} is already <root>/Source, so it should read "${CMAKE_SOURCE_DIR}/ThirdParty"

Will change.

Based on your response then we should also change
<http://trac.webkit.org/browser/trunk/Source/WebCore/CMakeLists.txt?rev=76792#L83>.

> 
> I'll try this change on my local build, because I'm not sure if it's the correct "*_INCLUDE_DIRECTORIES"

Actually, we should probably do this in change in file JavaScriptCore/wtf/CmakeListsWinCE.txt.

[Early Warning System Bot]

Attachment 80361 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7584370

[Patrick R. Gansterer]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 80361 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=80361&action=review
> > 
> > > Source/JavaScriptCore/CMakeListsWinCE.txt:3
> > > +LIST(APPEND JavaScriptCore_INCLUDE_DIRECTORIES
> > > +    "${CMAKE_SOURCE_DIR}/Source/ThirdParty"
> > > +)
> > 
> > ${CMAKE_SOURCE_DIR} is already <root>/Source, so it should read "${CMAKE_SOURCE_DIR}/ThirdParty"
> 
> Will change.

FYI, IMHO the "correct" change:

diff --git a/Source/JavaScriptCore/wtf/CMakeLists.txt b/Source/JavaScriptCore/wtf/CMakeLists.txt
index b1931d7..9406f5d 100644
--- a/Source/JavaScriptCore/wtf/CMakeLists.txt
+++ b/Source/JavaScriptCore/wtf/CMakeLists.txt
@@ -136,6 +136,7 @@ INCLUDE_IF_EXISTS(${JAVASCRIPTCORE_DIR}/wtf/CMakeLists${PORT}.txt)
 
 LIST(APPEND WTF_INCLUDE_DIRECTORIES
     "${CMAKE_BINARY_DIR}"
+    "${CMAKE_SOURCE_DIR}/ThirdParty"
 )
 
 WEBKIT_WRAP_SOURCELIST(${WTF_SOURCES})


OR


diff --git a/Source/JavaScriptCore/wtf/CMakeListsWinCE.txt b/Source/JavaScriptCore/wtf/CMakeListsWinCE.txt
index 9c558eb..19e9ceb 100644
--- a/Source/JavaScriptCore/wtf/CMakeListsWinCE.txt
+++ b/Source/JavaScriptCore/wtf/CMakeListsWinCE.txt
@@ -22,6 +22,10 @@ LIST(APPEND WTF_SOURCES
     ${3RDPARTY_DIR}/ce-compat/ce_unicode.cpp
 )
 
+LIST(APPEND WTF_INCLUDE_DIRECTORIES
+    "${CMAKE_SOURCE_DIR}/ThirdParty"
+)
+
 LIST(APPEND WTF_LIBRARIES
     mmtimer
 )


I like the first more, since you changed OS(WINCE) to USE(MERSENNE_TWISTER_19937).


> Based on your response then we should also change
> <http://trac.webkit.org/browser/trunk/Source/WebCore/CMakeLists.txt?rev=76792#L83>.
Unfortunately CMake build system need some cleanup :-(.
There are many "wrong" includes from the early "EFL only" days too.

[Daniel Bates]

Created attachment 80365 [details]
Patch

Updated patch based on Patrick Gansterer's suggestions.

Appended Source/ThirdParty to the end of the list of include directories in JavaScriptCore/JavaScriptCore.pri in an attempt to fix the Qt Windows EWS bot.

[Early Warning System Bot]

Attachment 80365 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7521351

[Csaba Osztrogonác]

Comment on attachment 80365 [details]
Patch

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

> Source/JavaScriptCore/JavaScriptCore.pri:26
> +    $$PWD/../ThirdParty

$$PWD/../ThirdParty \

[Daniel Bates]

(In reply to comment #8)
> (From update of attachment 80365 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80365&action=review
> 
> > Source/JavaScriptCore/JavaScriptCore.pri:26
> > +    $$PWD/../ThirdParty
> 
> $$PWD/../ThirdParty \

Thanks!

[Daniel Bates]

Created attachment 80448 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 80448 [details]
Patch

OK.

[Daniel Bates]

Comment on attachment 80448 [details]
Patch

Clearing flags on attachment: 80448

Committed r77070: <http://trac.webkit.org/changeset/77070>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.

[Daniel Bates]

(In reply to comment #5)
> > Based on your response then we should also change
> > <http://trac.webkit.org/browser/trunk/Source/WebCore/CMakeLists.txt?rev=76792#L83>.
> Unfortunately CMake build system need some cleanup :-(.
> There are many "wrong" includes from the early "EFL only" days too.

Filed bug #53382 to remove references to ${CMAKE_SOURCE_DIR}/Source in CMake files.