Bug 38060 - Split up Threading.h
Summary: Split up Threading.h
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-23 14:34 PDT by Sam Weinig
Modified: 2010-04-23 18:07 PDT (History)
2 users (show)

See Also:


Attachments
patch (38.44 KB, patch)
2010-04-23 14:39 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Updated patch (75.61 KB, patch)
2010-04-23 16:10 PDT, Sam Weinig
levin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2010-04-23 14:34:41 PDT
Threading.h is getting a little large. Time for a split.
Comment 1 Sam Weinig 2010-04-23 14:39:01 PDT
Created attachment 54194 [details]
patch
Comment 2 David Levin 2010-04-23 15:56:39 PDT
Two comments:
1. The include<wtf/*> sorting is off for the newly added items. 
2. I'm surprised that the new headers don't have entries in the ForwardingHeaders/wtf directories in 
JavaScriptGlue, WebCore, WebKit/mac, WebKitTools/DumpRenderTree (which is where Threading.h appears). Is this not necessary?
Comment 3 Sam Weinig 2010-04-23 16:06:24 PDT
(In reply to comment #2)
> Two comments:
> 1. The include<wtf/*> sorting is off for the newly added items. 

Will fix.

> 2. I'm surprised that the new headers don't have entries in the
> ForwardingHeaders/wtf directories in 
> JavaScriptGlue, WebCore, WebKit/mac, WebKitTools/DumpRenderTree (which is where
> Threading.h appears). Is this not necessary?

:( They do. I just forgot to upload that part of the patch.
Comment 4 Sam Weinig 2010-04-23 16:10:19 PDT
Created attachment 54198 [details]
Updated patch

Updated.  The WebKit/mac forwarding headers are not needed.
Comment 5 WebKit Review Bot 2010-04-23 16:17:52 PDT
Attachment 54198 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/wtf/ThreadSafeShared.h:65:  Alphabetical sorting problem.  [build/include_order] [4]
JavaScriptCore/wtf/Atomics.h:71:  More than one command on the same line in if  [whitespace/parens] [4]
Total errors found: 2 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 David Levin 2010-04-23 16:25:34 PDT
Atomics.h is messed up in this version. "#ifndef Threading_h" etc.

Also, none of your fwd'ing headers have ifndef guards but that seems rather useless and just adds extra clutter so I'm fine with omitting that.

And the style bot noticed a sorting issue.

I would r+ except for the Atomics.h
Comment 7 Sam Weinig 2010-04-23 16:55:56 PDT
(In reply to comment #6)
> Atomics.h is messed up in this version. "#ifndef Threading_h" etc.
> 

I don's see the issue you are referring to.

> Also, none of your fwd'ing headers have ifndef guards but that seems rather
> useless and just adds extra clutter so I'm fine with omitting that.

Some do (the ones in WebCore/ForwardingHeaders).  I tried to be consistent with the files around them.

> And the style bot noticed a sorting issue.

Will fix before landing.
Comment 8 David Levin 2010-04-23 18:02:33 PDT
Comment on attachment 54198 [details]
Updated patch

You're missing the changes in WebCore, so please make sure to include that when you land it.

About this "Atomics.h is messed up in this version."... I got mixed up because it was in the patch twice (due to the svn cp I guess).
Comment 9 Sam Weinig 2010-04-23 18:07:55 PDT
Landed in r58206.