RESOLVED FIXED 212312
[PlayStation] Enable RemoteWebInspector
https://bugs.webkit.org/show_bug.cgi?id=212312
Summary [PlayStation] Enable RemoteWebInspector
Yoshiaki Jitsukawa
Reported 2020-05-24 01:10:30 PDT
Enable RemoteWebInspector on PlayStation.
Attachments
Patch (8.30 KB, patch)
2020-05-24 01:21 PDT, Yoshiaki Jitsukawa
no flags
Patch (5.63 KB, patch)
2020-05-26 13:50 PDT, Yoshiaki Jitsukawa
no flags
Patch (3.57 KB, patch)
2020-05-26 14:06 PDT, Yoshiaki Jitsukawa
no flags
Yoshiaki Jitsukawa
Comment 1 2020-05-24 01:21:16 PDT
Don Olmstead
Comment 2 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.
Devin Rousso
Comment 3 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`
Yoshiaki Jitsukawa
Comment 4 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.
Yoshiaki Jitsukawa
Comment 5 2020-05-26 13:50:11 PDT
Yoshiaki Jitsukawa
Comment 6 2020-05-26 14:06:19 PDT
Don Olmstead
Comment 7 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
Yoshiaki Jitsukawa
Comment 8 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.
EWS
Comment 9 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].
Radar WebKit Bug Importer
Comment 10 2020-05-26 15:44:14 PDT
Note You need to log in before you can comment on or make changes to this bug.