Bug 31956 - Move some build decisions from Qt build system into source files
Summary: Move some build decisions from Qt build system into source files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-28 15:52 PST by Laszlo Gombos
Modified: 2009-12-18 16:30 PST (History)
6 users (show)

See Also:


Attachments
proposed patch (7.18 KB, patch)
2009-11-28 16:04 PST, Laszlo Gombos
no flags Details | Formatted Diff | Diff
Respond to Kevin's comments (7.04 KB, patch)
2009-11-28 17:45 PST, Laszlo Gombos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Laszlo Gombos 2009-11-28 15:52:02 PST
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.
Comment 1 Laszlo Gombos 2009-11-28 16:04:05 PST
Created attachment 43985 [details]
proposed patch
Comment 2 Kevin Ollivier 2009-11-28 16:58:37 PST
(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.
Comment 3 Laszlo Gombos 2009-11-28 17:45:40 PST
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 ?
Comment 4 Eric Seidel (no email) 2009-11-29 07:16:33 PST
Hmm... This looks OK, but it's going to make for some crazy platform/ files in WebCore. :)
Comment 5 Kevin Ollivier 2009-11-30 09:13:55 PST
(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!
Comment 6 Adam Barth 2009-11-30 12:47:40 PST
style-queue ran check-webkit-style on attachment 43987 [details] without any errors.
Comment 7 Laszlo Gombos 2009-12-01 11:58:36 PST
(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 8 Kenneth Rohde Christiansen 2009-12-17 08:39:15 PST
Comment on attachment 43987 [details]
Respond to Kevin's comments

Great change! There seems to be general consensus on this, so r+
Comment 9 WebKit Commit Bot 2009-12-18 16:30:03 PST
Comment on attachment 43987 [details]
Respond to Kevin's comments

Clearing flags on attachment: 43987

Committed r52355: <http://trac.webkit.org/changeset/52355>
Comment 10 WebKit Commit Bot 2009-12-18 16:30:11 PST
All reviewed patches have been landed.  Closing bug.