Summary: | Split up Threading.h | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sam Weinig <sam> | ||||||
Component: | Web Template Framework | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | levin, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Sam Weinig
2010-04-23 14:34:41 PDT
Created attachment 54194 [details]
patch
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? (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. Created attachment 54198 [details]
Updated patch
Updated. The WebKit/mac forwarding headers are not needed.
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.
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 (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 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).
|