Bug 53253 - Move wince/mt19937ar.c to ThirdParty and make it a policy choice
Summary: Move wince/mt19937ar.c to ThirdParty and make it a policy choice
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Other
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords:
Depends on:
Blocks: 53506
  Show dependency treegraph
 
Reported: 2011-01-27 12:51 PST by Daniel Bates
Modified: 2011-02-01 09:54 PST (History)
3 users (show)

See Also:


Attachments
Patch (15.62 KB, patch)
2011-01-27 13:35 PST, Daniel Bates
paroga: review-
Details | Formatted Diff | Diff
Patch (17.88 KB, patch)
2011-01-27 14:43 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (17.92 KB, patch)
2011-01-28 07:09 PST, Daniel Bates
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2011-01-27 12:51:44 PST
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.
Comment 1 Daniel Bates 2011-01-27 13:35:26 PST
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.
Comment 2 Patrick R. Gansterer 2011-01-27 13:45:39 PST
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"
Comment 3 Daniel Bates 2011-01-27 14:00:06 PST
(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.
Comment 4 Early Warning System Bot 2011-01-27 14:05:35 PST
Attachment 80361 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7584370
Comment 5 Patrick R. Gansterer 2011-01-27 14:12:01 PST
(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.
Comment 6 Daniel Bates 2011-01-27 14:43:05 PST
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.
Comment 7 Early Warning System Bot 2011-01-27 15:04:25 PST
Attachment 80365 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7521351
Comment 8 Csaba Osztrogonác 2011-01-27 15:12:29 PST
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 \
Comment 9 Daniel Bates 2011-01-27 16:25:00 PST
(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!
Comment 10 Daniel Bates 2011-01-28 07:09:27 PST
Created attachment 80448 [details]
Patch
Comment 11 Eric Seidel (no email) 2011-01-28 12:14:04 PST
Comment on attachment 80448 [details]
Patch

OK.
Comment 12 Daniel Bates 2011-01-29 14:17:51 PST
Comment on attachment 80448 [details]
Patch

Clearing flags on attachment: 80448

Committed r77070: <http://trac.webkit.org/changeset/77070>
Comment 13 Daniel Bates 2011-01-29 14:17:58 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Daniel Bates 2011-01-29 14:40:46 PST
(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.