Bug 155650 - Web Automation: Add Automation protocol commands to resolve frames as handles
Summary: Web Automation: Add Automation protocol commands to resolve frames as handles
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Timothy Hatcher
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-03-18 10:35 PDT by Timothy Hatcher
Modified: 2016-03-28 09:41 PDT (History)
7 users (show)

See Also:


Attachments
Patch (33.07 KB, patch)
2016-03-18 10:47 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch (32.84 KB, patch)
2016-03-18 10:48 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch (32.72 KB, patch)
2016-03-18 11:59 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch (32.82 KB, patch)
2016-03-18 12:24 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch (37.34 KB, patch)
2016-03-18 15:01 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch (39.15 KB, patch)
2016-03-21 10:14 PDT, Timothy Hatcher
bburg: review+
timothy: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Hatcher 2016-03-18 10:35:57 PDT
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.
Comment 1 Radar WebKit Bug Importer 2016-03-18 10:37:24 PDT
<rdar://problem/25242422>
Comment 2 Timothy Hatcher 2016-03-18 10:47:09 PDT
Created attachment 274428 [details]
Patch
Comment 3 Timothy Hatcher 2016-03-18 10:48:22 PDT
Created attachment 274429 [details]
Patch
Comment 4 BJ Burg 2016-03-18 11:40:41 PDT
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.
Comment 5 Timothy Hatcher 2016-03-18 11:59:15 PDT
Created attachment 274442 [details]
Patch
Comment 6 Joseph Pecoraro 2016-03-18 12:09:06 PDT
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?
Comment 7 Timothy Hatcher 2016-03-18 12:11:55 PDT
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.
Comment 8 Timothy Hatcher 2016-03-18 12:24:59 PDT
Created attachment 274447 [details]
Patch
Comment 9 Timothy Hatcher 2016-03-18 15:01:55 PDT
Created attachment 274458 [details]
Patch
Comment 10 Timothy Hatcher 2016-03-21 10:14:57 PDT
Created attachment 274600 [details]
Patch
Comment 11 BJ Burg 2016-03-25 17:12:09 PDT
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.
Comment 12 Timothy Hatcher 2016-03-28 09:41:38 PDT
https://trac.webkit.org/r198738