[PlayStation] Enable RemoteWebInspector

• Component: Platform
• Status: RESOLVED FIXED

[Yoshiaki Jitsukawa]

Enable RemoteWebInspector on PlayStation.

[Yoshiaki Jitsukawa]

Created attachment 400151 [details]
Patch

[Don Olmstead]

Comment on attachment 400151 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=400151&action=review

> Source/JavaScriptCore/API/JSRemoteInspectorServer.cpp:33
> +    auto& server = Inspector::RemoteInspectorServer::singleton();

Any reason this should change?

> Source/JavaScriptCore/PlatformPlayStation.cmake:2
> +    API/JSBasePrivate.h

I know why you have this here since there's that unhandled promise handler rejection handler that never became a public C API but its still a PRIVATE_FRAMEWORK_HEADER. Add it to the list of headers to install.

> Source/WTF/wtf/PlatformEnable.h:100
> +#include <wtf/PlatformEnablePlayStation.h>

I feel like this is a separate change unrelated to this bug.

> Source/cmake/OptionsPlayStation.cmake:35
> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_REMOTE_INSPECTOR PRIVATE ON)

You need to remove ENABLE_REMOTE_INSPECTOR from the Experimental features listing below.

[Devin Rousso]

Comment on attachment 400151 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=400151&action=review

> Source/JavaScriptCore/inspector/remote/socket/posix/RemoteInspectorSocketPOSIX.cpp:93
>      }

Style: please add a newline after multiline `if` that have a `return`

[Yoshiaki Jitsukawa]

(In reply to Don Olmstead from comment #2)
> Comment on attachment 400151 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=400151&action=review
> 
> > Source/JavaScriptCore/API/JSRemoteInspectorServer.cpp:33
> > +    auto& server = Inspector::RemoteInspectorServer::singleton();
> 
> Any reason this should change?

That's because start() is not a const function and it doesn't compile as-is.

> > Source/JavaScriptCore/PlatformPlayStation.cmake:2
> > +    API/JSBasePrivate.h
> 
> I know why you have this here since there's that unhandled promise handler
> rejection handler that never became a public C API but its still a
> PRIVATE_FRAMEWORK_HEADER. Add it to the list of headers to install.

Ok,JSBasePrivate.h itself is not needed with this patch. I'll remove this line.

> > Source/WTF/wtf/PlatformEnable.h:100
> > +#include <wtf/PlatformEnablePlayStation.h>
> 
> I feel like this is a separate change unrelated to this bug.

Yes, will do.
 
> > Source/cmake/OptionsPlayStation.cmake:35
> > +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_REMOTE_INSPECTOR PRIVATE ON)
> 
> You need to remove ENABLE_REMOTE_INSPECTOR from the Experimental features
> listing below.

Oh, I see.

(In reply to Devin Rousso from comment #3)
> Comment on attachment 400151 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=400151&action=review
> 
> > Source/JavaScriptCore/inspector/remote/socket/posix/RemoteInspectorSocketPOSIX.cpp:93
> >      }
> 
> Style: please add a newline after multiline `if` that have a `return`

Sure.

[Yoshiaki Jitsukawa]

Created attachment 400271 [details]
Patch

[Yoshiaki Jitsukawa]

Created attachment 400274 [details]
Patch

[Don Olmstead]

Comment on attachment 400274 [details]
Patch

r=me

Didn’t you want to enable remote inspector by default? My comment was that to do that you needed to remove the line tying it to experimental features as well

[Yoshiaki Jitsukawa]

(In reply to Don Olmstead from comment #7)
> Comment on attachment 400274 [details]
> Patch
> 
> r=me
> 
> Didn’t you want to enable remote inspector by default? My comment was that
> to do that you needed to remove the line tying it to experimental features
> as well

I did, but now I don't because it may make it a bit difficult to land WebKit portion so let me keep the default value as it is for now.

[EWS]

Committed r262166: <https://trac.webkit.org/changeset/262166>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400274 [details].

[Radar WebKit Bug Importer]

<rdar://problem/63646513>