Bug 216908 – [PlayStation][WinCairo] Enable WebDriver target on PlayStation and client for WinCairo

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

[Basuke Suzuki]

Time to make it.

[Basuke Suzuki]

Created attachment 409627 [details]
PATCH

[Basuke Suzuki]

Created attachment 409628 [details]
PATCH

[Basuke Suzuki]

With this patch, followings are activated:
- WinCairo WebDriver client binary which can connect to PlayStation.
- PlayStation WebDriver client binary which can connect to itself.
- PlayStation automation Host backed by RemoteInspector protocol.

[Don Olmstead]

Comment on attachment 409628 [details]
PATCH

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

Just commenting on the build part currently

> Source/WebDriver/HTTPServer.h:39
> +#define USE_NATIVE_HTTPD 1

Don't define this. Just do USE(INSPECTOR_SOCKET_SERVER).

> Source/WebDriver/PlatformPlayStation.cmake:13
> +    ${JavaScriptCore_DERIVED_SOURCES_DIR}

Why is this including the JSC Derived Sources dir?

> Source/WebDriver/PlatformWin.cmake:15
> +    ${JavaScriptCore_DERIVED_SOURCES_DIR}

Ditto

> Source/WebDriver/socket/HTTPParser.cpp:2
> + * Copyright (C) 2019 Sony Interactive Entertainment Inc.

Nit copyright year.

> Source/WebDriver/socket/HTTPParser.h:2
> + * Copyright (C) 2019 Sony Interactive Entertainment Inc.

Nit copyright year

[Adrian Perez]

Comment on attachment 409628 [details]
PATCH

The parts that touch the common code look reasonable to me, but I am
not that well acquainted with the WebDriver and RemoteInspector bits
to dare to rubber-stamp it.

If Carlos García could take a look, that would the best ☝️

[Basuke Suzuki]

Comment on attachment 409628 [details]
PATCH

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

Thanks!

>> Source/WebDriver/HTTPServer.h:39
>> +#define USE_NATIVE_HTTPD 1
> 
> Don't define this. Just do USE(INSPECTOR_SOCKET_SERVER).

Okay, got it.

>> Source/WebDriver/PlatformPlayStation.cmake:13
>> +    ${JavaScriptCore_DERIVED_SOURCES_DIR}
> 
> Why is this including the JSC Derived Sources dir?

Because it uses common code of JSC's socket RemoteInspector implementation.

>> Source/WebDriver/socket/HTTPParser.cpp:2
>> + * Copyright (C) 2019 Sony Interactive Entertainment Inc.
> 
> Nit copyright year.

Yey. We need style checker rule for this :)

[Basuke Suzuki]

(In reply to Adrian Perez from comment #5)
> Comment on attachment 409628 [details]
> PATCH
> 
> The parts that touch the common code look reasonable to me, but I am
> not that well acquainted with the WebDriver and RemoteInspector bits
> to dare to rubber-stamp it.
> 
> If Carlos García could take a look, that would the best ☝️

Thanks, Adrian!

[Carlos Garcia Campos]

Comment on attachment 409628 [details]
PATCH

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

LGTM

> Source/WebDriver/WebDriverService.cpp:51
> +    printf("  -t <ip:port> --target=<ip:port> [WinCairo] Target IP and port\n");

Make this conditional to WinCairo (or INSPECTOR_SOCKET_SERVER)

[Stephan Szabo]

Comment on attachment 409628 [details]
PATCH

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

> Source/WebDriver/socket/SessionHostSocket.cpp:146
> +    m_capabilities.browserVersion = String::fromUTF8("TODO/browserVersion");

Is there a reason to leave the TODO names here (like do they also get set in some other path?)

[Basuke Suzuki]

Comment on attachment 409628 [details]
PATCH

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

>>> Source/WebDriver/PlatformPlayStation.cmake:13
>>> +    ${JavaScriptCore_DERIVED_SOURCES_DIR}
>> 
>> Why is this including the JSC Derived Sources dir?
> 
> Because it uses common code of JSC's socket RemoteInspector implementation.

It is built without this line, so I'll remove it.

>> Source/WebDriver/PlatformWin.cmake:15
>> +    ${JavaScriptCore_DERIVED_SOURCES_DIR}
> 
> Ditto

Dotto

>> Source/WebDriver/WebDriverService.cpp:51
>> +    printf("  -t <ip:port> --target=<ip:port> [WinCairo] Target IP and port\n");
> 
> Make this conditional to WinCairo (or INSPECTOR_SOCKET_SERVER)

Right. Thanks.

>> Source/WebDriver/socket/SessionHostSocket.cpp:146
>> +    m_capabilities.browserVersion = String::fromUTF8("TODO/browserVersion");
> 
> Is there a reason to leave the TODO names here (like do they also get set in some other path?)

Yes, that must be done quickly, but we don't have proper versioning in our port and we also don't have a way to access them from the code. I'll create a ticket for that.

[Basuke Suzuki]

Created attachment 410091 [details]
PATCH

[EWS]

Committed r267807: <https://trac.webkit.org/changeset/267807>

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

[Radar WebKit Bug Importer]

<rdar://problem/69808308>