Bug 31956 – Move some build decisions from Qt build system into source files

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Laszlo Gombos]

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.

[Laszlo Gombos]

Created attachment 43985 [details]
proposed patch

[Kevin Ollivier]

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

[Laszlo Gombos]

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 ?

[Eric Seidel (no email)]

Hmm... This looks OK, but it's going to make for some crazy platform/ files in WebCore. :)

[Kevin Ollivier]

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

[Adam Barth]

style-queue ran check-webkit-style on attachment 43987 [details] without any errors.

[Laszlo Gombos]

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

[Kenneth Rohde Christiansen]

Comment on attachment 43987 [details]
Respond to Kevin's comments

Great change! There seems to be general consensus on this, so r+

[WebKit Commit Bot]

Comment on attachment 43987 [details]
Respond to Kevin's comments

Clearing flags on attachment: 43987

Committed r52355: <http://trac.webkit.org/changeset/52355>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.