Bug 199070 – [WinCairo][PlayStation] Add automation support for RemoteInspector SocketServer implementation.

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

[Basuke Suzuki]

Add basic implementation of automation for RemoteInspector socket server backend.

[Basuke Suzuki]

Created attachment 380774 [details]
PATCH

[Basuke Suzuki]

Created attachment 380775 [details]
PATCH

[Basuke Suzuki]

Basically following GTK implementation.

[Ross Kirsling]

Comment on attachment 380775 [details]
PATCH

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

> Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorSocket.cpp:200
> +    if (!m_client)
> +        return;
> +
> +    if (!m_clientCapabilities || !m_clientCapabilities->remoteAutomationAllowed)
> +        return;
> +
> +    if (sessionID.isNull())
> +        return;

Not sure that we need three separate conditionals here.

> Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorSocket.cpp:359
> +    String sessionID = *event.message;
> +    requestAutomationSession(WTFMove(sessionID), { });

This style is a bit confusing. At the least, I think `event.message.value()` would be more readable, but I also wonder if it's worth making the new requestAutomationSession take const String& instead, so that we wouldn't have to explicitly copy.

[Basuke Suzuki]

Comment on attachment 380775 [details]
PATCH

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

>> Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorSocket.cpp:200
>> +        return;
> 
> Not sure that we need three separate conditionals here.

Latter two are runtime check which caused by configuration so I'll add different LOG_ERROR to them. First one is the correct check which caused by timing and required check.

>> Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorSocket.cpp:359
>> +    requestAutomationSession(WTFMove(sessionID), { });
> 
> This style is a bit confusing. At the least, I think `event.message.value()` would be more readable, but I also wonder if it's worth making the new requestAutomationSession take const String& instead, so that we wouldn't have to explicitly copy.

Right. I'll change this to const String& because last step of this function calls may ended up with const String& and using `move` here is meaningless.

[Basuke Suzuki]

Created attachment 381013 [details]
PATCH

[Ross Kirsling]

Comment on attachment 381013 [details]
PATCH

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

> Source/JavaScriptCore/inspector/remote/RemoteInspector.h:152
> +    void requestAutomationSession(const String& sessionID, const Client::SessionCapabilities&);

Final nit: I guess the string name can be omitted here.

[Basuke Suzuki]

Comment on attachment 381013 [details]
PATCH

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

Thanks!

>> Source/JavaScriptCore/inspector/remote/RemoteInspector.h:152
>> +    void requestAutomationSession(const String& sessionID, const Client::SessionCapabilities&);
> 
> Final nit: I guess the string name can be omitted here.

No I don't think so. General types such as bool, int or String should have its name to make it meaningful.

[WebKit Commit Bot]

Comment on attachment 381013 [details]
PATCH

Clearing flags on attachment: 381013

Committed r251373: <https://trac.webkit.org/changeset/251373>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/56472064>