Bug 153934 – Add a WebKit SPI for creating an automation session and advertise the active session via RemoteInspector

• Component: WebKit2
• Status: RESOLVED FIXED

[Blaze Burg]

An automation session is the main place where WebKit-side support for browser automation should be implemented. A session encapsulates the state (default timeouts, page identifier mappings), objects (active automation WebPages) and logic (how to open a window, run JS, send user input, etc) used to implement automation commands. A process pool can only have one session at a time. The session is created on demand and only if the _WKAutomationDelegate implementation allows it.

I'm currently focusing on *remote* automation, over the RemoteInspector/webinspectord relay system. This is needed in WK2 since its clients cannot directly talk to RemoteInspector in JavaScriptCore.framework.

The session has a delegate so that WK2 clients can perform actions that are outside the scope of WK2, such as creating new tabs, enumerating windows, or sending native user inputs.

Patch forthcoming.

[Blaze Burg]

Created attachment 270805 [details]
Take 1

[Blaze Burg]

<rdar://problem/24077538>

[Joseph Pecoraro]

Comment on attachment 270805 [details]
Take 1

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

Looks good to me. This should get a WK2 Owner for the API bits

> Source/WebKit2/UIProcess/API/APIAutomationSessionClient.h:42
> +    virtual String sessionIdentifier() const { return emptyString(); }

Null string would be even simpler. String().

> Source/WebKit2/UIProcess/API/Cocoa/_WKAutomationSession.mm:74
> +- (NSString *)sessionIdentifier
> +{
> +    return _session->name();
> +}

This only exists in WebKit::WebAutomationSession when ENABLE(REMOTE_INSPECTOR) via:

    virtual String name() const override { return m_sessionIdentifier; }

WebKit::WebAutomationSession should probably have a `String sessionIdentifier() const { return m_sessionIdentifier; }` for this to work sans REMOTE_INSPECTOR.

> Source/WebKit2/UIProcess/WebAutomationSession.cpp:74
> +void WebAutomationSession::connect(Inspector::FrontendChannel* channel, bool isAutomaticConnection)
> +{
> +    UNUSED_PARAM(isAutomaticConnection);
> +
> +    m_remoteChannel = channel;
> +    setAutomationAllowed(false);
> +}

Wouldn't this make the listing for this AutomationTarget go away? It seems this might cause the remote client to disconnect from it. I'm not sure this part is right.

> Source/WebKit2/UIProcess/WebAutomationSession.h:66
> +    String m_sessionIdentifier { ASCIILiteral("Untitled Session") };

Whoa! I didn't realize you could do this.

> Source/WebKit2/UIProcess/WebAutomationSession.h:69
> +    Inspector::FrontendChannel* m_remoteChannel;

Nit: { nullptr }, otherwise this will be uninitialized.

> Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:5160
> +				990D28AF1C65203900986977 /* _WKAutomationSessionInternal.h */,
> +				990D28A81C6404B000986977 /* _WKAutomationSessionDelegate.h */,

Sort order here may be important.

[Blaze Burg]

Comment on attachment 270805 [details]
Take 1

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

>> Source/WebKit2/UIProcess/API/Cocoa/_WKAutomationSession.mm:74
>> +}
> 
> This only exists in WebKit::WebAutomationSession when ENABLE(REMOTE_INSPECTOR) via:
> 
>     virtual String name() const override { return m_sessionIdentifier; }
> 
> WebKit::WebAutomationSession should probably have a `String sessionIdentifier() const { return m_sessionIdentifier; }` for this to work sans REMOTE_INSPECTOR.

REMOTE_INSPECTOR is always enabled when building Cocoa API, as far as I know. So I didn't bother. But maybe it would be fine to have a second method that's not a RemoteInspector::Client override.

>> Source/WebKit2/UIProcess/WebAutomationSession.cpp:74
>> +}
> 
> Wouldn't this make the listing for this AutomationTarget go away? It seems this might cause the remote client to disconnect from it. I'm not sure this part is right.

You are right. I think I should change automationAllowed() to pairedWithDriver() or something similar. I want this bit to be included in the listing, so other drivers don't try to connect to the same session. On the backend, a driver can check sessionIdentifier against its own identifier to make sure that it was the one who paired with the automation target.

>> Source/WebKit2/UIProcess/WebAutomationSession.h:66
>> +    String m_sessionIdentifier { ASCIILiteral("Untitled Session") };
> 
> Whoa! I didn't realize you could do this.

Yeah, I didn't know either until I searched.

>> Source/WebKit2/UIProcess/WebAutomationSession.h:69
>> +    Inspector::FrontendChannel* m_remoteChannel;
> 
> Nit: { nullptr }, otherwise this will be uninitialized.

Yup.

[Blaze Burg]

Comment on attachment 270805 [details]
Take 1

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

>>> Source/WebKit2/UIProcess/WebAutomationSession.cpp:74
>>> +}
>> 
>> Wouldn't this make the listing for this AutomationTarget go away? It seems this might cause the remote client to disconnect from it. I'm not sure this part is right.
> 
> You are right. I think I should change automationAllowed() to pairedWithDriver() or something similar. I want this bit to be included in the listing, so other drivers don't try to connect to the same session. On the backend, a driver can check sessionIdentifier against its own identifier to make sure that it was the one who paired with the automation target.

Note: the _WKAutomationDelegate can refuse to create a session if it doesn't support automation or one already exists, so this bit is not useful as-is.

[Blaze Burg]

Comment on attachment 270805 [details]
Take 1

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

>>>> Source/WebKit2/UIProcess/WebAutomationSession.cpp:74
>>>> +}
>>> 
>>> Wouldn't this make the listing for this AutomationTarget go away? It seems this might cause the remote client to disconnect from it. I'm not sure this part is right.
>> 
>> You are right. I think I should change automationAllowed() to pairedWithDriver() or something similar. I want this bit to be included in the listing, so other drivers don't try to connect to the same session. On the backend, a driver can check sessionIdentifier against its own identifier to make sure that it was the one who paired with the automation target.
> 
> Note: the _WKAutomationDelegate can refuse to create a session if it doesn't support automation or one already exists, so this bit is not useful as-is.

Split this off: https://bugs.webkit.org/show_bug.cgi?id=154077

>> Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:5160
>> +				990D28A81C6404B000986977 /* _WKAutomationSessionDelegate.h */,
> 
> Sort order here may be important.

The WK2 project is well overdue for a sorting.

[Blaze Burg]

Created attachment 271019 [details]
Take 2 (Depends on 154077)

[Joseph Pecoraro]

Comment on attachment 271019 [details]
Take 2 (Depends on 154077)

Looks fine to me. Needs a WK2 Owner nod

[Blaze Burg]

Committed r196488: <http://trac.webkit.org/changeset/196488>

[Csaba Osztrogonác]

(In reply to comment #9)
> Committed r196488: <http://trac.webkit.org/changeset/196488>

It broke the Apple Mac cmake build:

Undefined symbols for architecture x86_64:
  "_OBJC_CLASS_$__WKAutomationSession", referenced from:
      objc-class-ref in APIObject.mm.o
  "_OBJC_IVAR_$__WKAutomationSession._session", referenced from:
      -[WKProcessPool(WKPrivate) _setAutomationSession:] in WKProcessPool.mm.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [lib/WebKit.framework/Versions/SOVERSION/WebKit] Error 1
make[1]: *** [Source/WebKit2/CMakeFiles/WebKit2.dir/all] Error 2
make: *** [all] Error 2

[Csaba Osztrogonác]

build fixed by https://trac.webkit.org/changeset/196630