This patch a step towards a new way to handle porting #ifdefs - as it has been proposed by Maciej Stachowiak on webkit-dev. > I know there is a potential compile time tradeoff, but in some ways > it would be nicer if all build systems built the same set of files, > and we ifdef'd the contents. That would put the policy decisions all > in one place (the port header). see https://lists.webkit.org/pipermail/webkit-dev/2009-May/007858.html. There was not probably a consensus on all aspects of Maciej's proposal but I was hoping that there was enough of consensus to approve the direction of the following patch.
Created attachment 43985 [details] proposed patch
(In reply to comment #0) > This patch a step towards a new way to handle porting #ifdefs - as it has been > proposed by Maciej Stachowiak on webkit-dev. > > > I know there is a potential compile time tradeoff, but in some ways > > it would be nicer if all build systems built the same set of files, > > and we ifdef'd the contents. That would put the policy decisions all > > in one place (the port header). > > see https://lists.webkit.org/pipermail/webkit-dev/2009-May/007858.html. > > There was not probably a consensus on all aspects of Maciej's proposal but I > was hoping that there was enough of consensus to approve the direction of the > following patch. I like the general idea, as it would simplify the wx port's wscripts even more, but the wx part of the patch is wrong. We are compiling ThreadingPthreads.cpp on all platforms, not ThreadingNone.cpp. It should be safe to define JSC_MULTIPLE_THREADS for wx too, I'm actually not sure how it was overlooked.
Created attachment 43987 [details] Respond to Kevin's comments Kevin, thanks for the comments. I removed the WX part from Platform.h - this makes the patch even simpler. I found ThreadingNone.cpp in JavaScriptCoreSources.bkl, this is why I though the Wx port is using ThreadingNone.cpp. Is the JavaScriptCoreSources.bkl file now obsolete ? As for JSC_MULTIPLE_THREADS for wx, can you please file a separate patch for that ?
Hmm... This looks OK, but it's going to make for some crazy platform/ files in WebCore. :)
(In reply to comment #3) > Created an attachment (id=43987) [details] > Respond to Kevin's comments > > Kevin, thanks for the comments. I removed the WX part from Platform.h - this > makes the patch even simpler. > > I found ThreadingNone.cpp in JavaScriptCoreSources.bkl, this is why I though > the Wx port is using ThreadingNone.cpp. Is the JavaScriptCoreSources.bkl file > now obsolete ? Yes, actually all of the Bakefile stuff needs to be removed, as it is no longer being used. I just haven't had time to devote to doing the cleanup recently. > As for JSC_MULTIPLE_THREADS for wx, can you please file a separate patch for > that ? Yeah, I'll put this on my TODO list and handle it as a separate ticket. Thanks!
style-queue ran check-webkit-style on attachment 43987 [details] without any errors.
(In reply to comment #4) > Hmm... This looks OK, but it's going to make for some crazy platform/ files in > WebCore. :) I do not see landing this patch as a commitment to follow this approach trough for the entire WebKit source tree, but I did wanted to highlight that this was discussed on webkit-dev and proposed before. I think for now we should continue the discussion/reviews on a per patch bases. Kevin's feedback for the from the wx build system perspective seems to be positive.
Comment on attachment 43987 [details] Respond to Kevin's comments Great change! There seems to be general consensus on this, so r+
Comment on attachment 43987 [details] Respond to Kevin's comments Clearing flags on attachment: 43987 Committed r52355: <http://trac.webkit.org/changeset/52355>
All reviewed patches have been landed. Closing bug.