Bug 38060 - Split up Threading.h
: Split up Threading.h
Status: RESOLVED FIXED
: WebKit
Web Template Framework
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-04-23 14:34 PST by
Modified: 2010-04-23 18:07 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-04-23 14:34:41 PST
Threading.h is getting a little large. Time for a split.
------- Comment #1 From 2010-04-23 14:39:01 PST -------
Created an attachment (id=54194) [details]
patch
------- Comment #2 From 2010-04-23 15:56:39 PST -------
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 From 2010-04-23 16:06:24 PST -------
(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 From 2010-04-23 16:10:19 PST -------
Created an attachment (id=54198) [details]
Updated patch

Updated.  The WebKit/mac forwarding headers are not needed.
------- Comment #5 From 2010-04-23 16:17:52 PST -------
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 From 2010-04-23 16:25:34 PST -------
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 From 2010-04-23 16:55:56 PST -------
(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 From 2010-04-23 18:02:33 PST -------
(From update of attachment 54198 [details])
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 From 2010-04-23 18:07:55 PST -------
Landed in r58206.