Bug 212312 - [PlayStation] Enable RemoteWebInspector
Summary: [PlayStation] Enable RemoteWebInspector
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yoshiaki Jitsukawa
URL:
Keywords: InRadar
Depends on:
Blocks: 191038
  Show dependency treegraph
 
Reported: 2020-05-24 01:10 PDT by Yoshiaki Jitsukawa
Modified: 2020-05-26 15:44 PDT (History)
18 users (show)

See Also:


Attachments
Patch (8.30 KB, patch)
2020-05-24 01:21 PDT, Yoshiaki Jitsukawa
no flags Details | Formatted Diff | Diff
Patch (5.63 KB, patch)
2020-05-26 13:50 PDT, Yoshiaki Jitsukawa
no flags Details | Formatted Diff | Diff
Patch (3.57 KB, patch)
2020-05-26 14:06 PDT, Yoshiaki Jitsukawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yoshiaki Jitsukawa 2020-05-24 01:10:30 PDT
Enable RemoteWebInspector on PlayStation.
Comment 1 Yoshiaki Jitsukawa 2020-05-24 01:21:16 PDT
Created attachment 400151 [details]
Patch
Comment 2 Don Olmstead 2020-05-26 08:37:55 PDT
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.
Comment 3 Devin Rousso 2020-05-26 08:47:37 PDT
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`
Comment 4 Yoshiaki Jitsukawa 2020-05-26 13:40:52 PDT
(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.
Comment 5 Yoshiaki Jitsukawa 2020-05-26 13:50:11 PDT
Created attachment 400271 [details]
Patch
Comment 6 Yoshiaki Jitsukawa 2020-05-26 14:06:19 PDT
Created attachment 400274 [details]
Patch
Comment 7 Don Olmstead 2020-05-26 15:33:27 PDT
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
Comment 8 Yoshiaki Jitsukawa 2020-05-26 15:39:56 PDT
(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.
Comment 9 EWS 2020-05-26 15:43:45 PDT
Committed r262166: <https://trac.webkit.org/changeset/262166>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400274 [details].
Comment 10 Radar WebKit Bug Importer 2020-05-26 15:44:14 PDT
<rdar://problem/63646513>