Bug 177017 – [Win] WTF: Add alias for process id to use in place of direct uses of pid_t

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Stephan Szabo]

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.

[Stephan Szabo]

Created attachment 320935 [details]
Test patch for ews on adding process identifier alias.

[Mark Lam]

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.

[Mark Lam]

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.

[Mark Lam]

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.

[Stephan Szabo]

Created attachment 320950 [details]
Test patch for ews on adding process identifier alias.

[Stephan Szabo]

Created attachment 320953 [details]
Test patch for ews on adding process identifier alias.

Changed name, added using, added space

[Mark Lam]

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.

[Stephan Szabo]

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

[Tim Horton]

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?

[Stephan Szabo]

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.

[Tim Horton]

(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?

[Stephan Szabo]

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.

[Stephan Szabo]

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.

[Stephan Szabo]

Should have expected OS() to be unhappy. Redoing

[Stephan Szabo]

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

[Stephan Szabo]

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.

[Alexey Proskuryakov]

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.

[Stephan Szabo]

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.

[Build Bot]

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.

[Stephan Szabo]

Created attachment 321236 [details]
Adding process identifier aliases to avoid direct use of pid_t

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34693257>