Bug 155650 – Web Automation: Add Automation protocol commands to resolve frames as handles

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Timothy Hatcher]

We need a frame handle to track it as the active frame in the driver. We need to be able to resolve them via a number, name or element handle.

[Radar WebKit Bug Importer]

<rdar://problem/25242422>

[Timothy Hatcher]

Created attachment 274428 [details]
Patch

[Timothy Hatcher]

Created attachment 274429 [details]
Patch

[Blaze Burg]

Comment on attachment 274429 [details]
Patch

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

Very nice, just a few issues to address.

> Source/WebKit2/UIProcess/Automation/Automation.json:35
> +            "id": "PageFrameHandle",

I think simply 'FrameHandle' is ok. Or, add 'Page' to NodeHandle.

> Source/WebKit2/UIProcess/Automation/Automation.json:173
> +            "description": "Determines the <code>PageFrameHandle</code> based on the ordinal of the frame or a node handle to a frame owner element.",

Nit: node handle 'for' a frame owner element

> Source/WebKit2/UIProcess/Automation/Automation.json:175
> +                { "name": "browsingContextHandle", "$ref": "BrowsingContextHandle", "description": "The handle for the browsing context the frame is located." },

Nit: ...context in which to search for the frame.

> Source/WebKit2/UIProcess/Automation/Automation.json:176
> +                { "name": "ordinal", "type": "integer", "optional": true, "description": "The ordinal of the frame to resolve as a <code>PageFrameHandle</code>." },

Please mention the thing it's an ordinal index into. I think it's window.frames ?

> Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.cpp:283
> +    WebCore::Frame* coreChildFrame = coreMainFrame->tree().child(ordinal);

I think this is the incorrect API, it should FrameTree::scopedChild(ordinal). window.frames is a list of all frames in the page's frame tree, not the child ordinal of the main frame.

In bool JSDOMWindow::getOwnPropertySlotByIndex(JSObject* object, ExecState* exec, unsigned index, PropertySlot& slot), it implements it with the following

    // (1) First, indexed properties.
    // These are also allowed cross-orgin, so come before the access check.
    if (frame && index < frame->tree().scopedChildCount()) {
        slot.setValue(thisObject, ReadOnly | DontDelete | DontEnum, toJS(exec, frame->tree().scopedChild(index)->document()->domWindow()));
        return true;
    }

The frame check is not needed, but we are missing the bounds check and calling a different API.

> Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.cpp:285
> +        WebProcess::singleton().parentProcessConnection()->send(Messages::WebAutomationSession::DidResolveFrame(callbackID, 0, frameNotFoundErrorType), 0);

If the frame is out of bounds then scopedChild returns nullptr so we can just keep this branch and not add an explicit bound check on the ordinal.

> Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.cpp:341
> +        WebProcess::singleton().parentProcessConnection()->send(Messages::WebAutomationSession::DidResolveFrame(callbackID, frameFromElement->frameID(), frameNotFoundErrorType), 0);

This should pass emptyString() instead of frameNotFound since it's the happy path.

> Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.h:33
> +

Nit: extra newlines not needed.

> Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.js:62
> +    nodeForIdentifier(identifier)

It might be good to document the inputs and outputs of these functions a little more. For example, script execution throws an exception on failure but nodeforIdentifier returns null.

[Timothy Hatcher]

Created attachment 274442 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 274429 [details]
Patch

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

> Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.cpp:185
> +    return &JSC::jsCast<WebCore::JSElement*>(toJS(element))->wrapped();

Is it possible for a NodeHandle to be a non-Element object? For instance a WebCore::Node* that is not an element?

[Timothy Hatcher]

Comment on attachment 274429 [details]
Patch

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

>> Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.cpp:185
>> +    return &JSC::jsCast<WebCore::JSElement*>(toJS(element))->wrapped();
> 
> Is it possible for a NodeHandle to be a non-Element object? For instance a WebCore::Node* that is not an element?

Oops, yes. I lost my check for inherits(JSElement::info()) when I moved this to a helper function. I'll add it back.

[Timothy Hatcher]

Created attachment 274447 [details]
Patch

[Timothy Hatcher]

Created attachment 274458 [details]
Patch

[Timothy Hatcher]

Created attachment 274600 [details]
Patch

[Blaze Burg]

Comment on attachment 274600 [details]
Patch

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

r=me

> Source/WebKit2/UIProcess/Automation/Automation.json:188
> +                { "name": "frameHandle", "$ref": "FrameHandle", "optional": true, "description": "The handle for the frame in which to search for the frame. The main frame is used if this parameter empty string or excluded." },

Please add 'child' and 'parent' to the description.

[Timothy Hatcher]

https://trac.webkit.org/r198738