pid_t is used directly in places, but isn't present for Windows. Make an alias in WTF for process identifiers for non-port-specific files.
Created attachment 320935 [details] Test patch for ews on adding process identifier alias.
Comment on attachment 320935 [details] Test patch for ews on adding process identifier alias. View in context: https://bugs.webkit.org/attachment.cgi?id=320935&action=review > Source/WTF/wtf/ProcessID.h:40 > +using ProcessIdentifier = int; Let's just call this ProcessID like the file.
Comment on attachment 320935 [details] Test patch for ews on adding process identifier alias. View in context: https://bugs.webkit.org/attachment.cgi?id=320935&action=review >> Source/WTF/wtf/ProcessID.h:40 >> +using ProcessIdentifier = int; > > Let's just call this ProcessID like the file. Also, the common practice is to add using WTF::ProcessID at the bottom of this file so that we can just say ProcessID everywhere else instead of WTF::ProcessID.
Comment on attachment 320935 [details] Test patch for ews on adding process identifier alias. View in context: https://bugs.webkit.org/attachment.cgi?id=320935&action=review > Source/JavaScriptCore/inspector/remote/RemoteInspector.h:112 > - pid_t parentProcessIdentifier() const { return m_parentProcessIdentifier; } > + WTF::ProcessIdentifierparentProcessIdentifier() const { return m_parentProcessIdentifier; } typo here (missing " "). Please fix.
Created attachment 320950 [details] Test patch for ews on adding process identifier alias.
Created attachment 320953 [details] Test patch for ews on adding process identifier alias. Changed name, added using, added space
Comment on attachment 320953 [details] Test patch for ews on adding process identifier alias. View in context: https://bugs.webkit.org/attachment.cgi?id=320953&action=review > Source/JavaScriptCore/API/JSRemoteInspector.cpp:54 > +void JSRemoteInspectorSetParentProcessInformation(WTF::ProcessID pid, const uint8_t* auditData, size_t auditLength) You don't need the WTF:: qualifier here anymore.
Created attachment 320956 [details] Test patch for ews on adding process identifier alias. Thanks, I'm not sure how my find and replace missed that. Now removed WTF:: on JSRemoteInspectorSetParentProcessInformation
Comment on attachment 320956 [details] Test patch for ews on adding process identifier alias. View in context: https://bugs.webkit.org/attachment.cgi?id=320956&action=review > Source/WebKit/UIProcess/API/C/WKContextPrivate.h:31 > +#include <wtf/ProcessID.h> We can't use wtf/ from exported (public or private) API headers (the ones like APIProcessPoolConfiguration.h are OK -- those are internal implementations of the API; you can check if the header is public/private in Xcode if you need to confirm in each case) as far as I know, so I don't think you can do this. Can we leave it as pid_t just in the API/SPI?
I don't think leaving it as pid_t works unless that file wouldn't be used on Windows building with WebKit (as we're now trying to do as opposed to WebKitLegacy) because pid_t wouldn't be valid there. We could do something similar to what is in ProcessID, but then we'd have to worry about them staying in sync.
(In reply to Stephan Szabo from comment #10) > I don't think leaving it as pid_t works unless that file wouldn't be used on > Windows building with WebKit (as we're now trying to do as opposed to > WebKitLegacy) because pid_t wouldn't be valid there. > > We could do something similar to what is in ProcessID, but then we'd have to > worry about them staying in sync. You're planning on using the C API?
We have been trying to add port files and make things there work for Windows including that API. It shouldn't be hard to make separate definitions or to roll back the two private bits there and discuss how to approach the API headers.
Created attachment 320965 [details] Test patch for ews on adding process identifier alias, api private headers using pid_t with override To see how this fares, update the two API/C/*Private.h files back to pid_t with an OS(WINDOWS) setup of pid_t typedef.
Should have expected OS() to be unhappy. Redoing
Created attachment 320966 [details] Test patch for ews on adding process identifier alias, api private headers using pid_t with override Now with non-OS check on private headers using pid_t version
Comment on attachment 320966 [details] Test patch for ews on adding process identifier alias, api private headers using pid_t with override I'm not sure if the changes on the two private headers should be there or maybe in WKBase or if we should use a name other than pid_t.
Comment on attachment 320966 [details] Test patch for ews on adding process identifier alias, api private headers using pid_t with override View in context: https://bugs.webkit.org/attachment.cgi?id=320966&action=review > Source/JavaScriptCore/API/JSRemoteInspector.h:31 > +#include <wtf/ProcessID.h> As Tim explained, API headers cannot use wtf.
Created attachment 321104 [details] Test patch for ews on adding process identifier alias Missed that the JavaScriptCore one was an API header as well. Again not sure if putting definitions in each header is correct or if there's a more general place to put platform specific type information for the API headers, nor whether it'd be better to still use a type that isn't directly pid_t in them to prevent non-portable types from leaking.
Attachment 321104 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/API/JSRemoteInspector.h:33: pid_t is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 1 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 321236 [details] Adding process identifier aliases to avoid direct use of pid_t
Comment on attachment 321236 [details] Adding process identifier aliases to avoid direct use of pid_t Clearing flags on attachment: 321236 Committed r222309: <http://trac.webkit.org/changeset/222309>
All reviewed patches have been landed. Closing bug.
<rdar://problem/34693257>