Summary: | Web Automation: Add Automation protocol commands to resolve frames as handles | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Timothy Hatcher <timothy> | ||||||||||||||
Component: | Web Inspector | Assignee: | Timothy Hatcher <timothy> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | bburg, graouts, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | All | ||||||||||||||||
OS: | All | ||||||||||||||||
Attachments: |
|
Description
Timothy Hatcher
2016-03-18 10:35:57 PDT
Created attachment 274428 [details]
Patch
Created attachment 274429 [details]
Patch
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. Created attachment 274442 [details]
Patch
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 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. Created attachment 274447 [details]
Patch
Created attachment 274458 [details]
Patch
Created attachment 274600 [details]
Patch
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. |