Bug 154953

Summary: Add _WKAutomationSessionDelegate methods for opening, closing, and switching windows
Product: WebKit Reporter: BJ Burg <bburg>
Component: WebKit2Assignee: 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 Flags
WIP
none
Patch 1/2
bburg: review+, timothy: commit-queue-
Patch 2/2 bburg: review+, timothy: commit-queue-

Description BJ Burg 2016-03-02 21:03:05 PST
Start fleshing out the commands that are currently stubbed.
Comment 1 Radar WebKit Bug Importer 2016-03-02 21:03:40 PST
<rdar://problem/24947489>
Comment 2 BJ Burg 2016-03-02 21:16:14 PST
Created attachment 272728 [details]
WIP

Start making the protocol specification closer to what is needed. Add some TODOs with blind code in WebAutomationSession for what needs to be done
Comment 3 Timothy Hatcher 2016-03-05 09:25:07 PST
Created attachment 273087 [details]
Patch 1/2
Comment 4 Timothy Hatcher 2016-03-05 09:25:29 PST
Created attachment 273088 [details]
Patch 2/2
Comment 5 BJ Burg 2016-03-05 09:46:34 PST
Comment on attachment 273087 [details]
Patch 1/2

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

r=me

> Source/WebKit2/ChangeLog:6
> +

Please put the subtitle text after Reviewed By
Comment 6 BJ Burg 2016-03-05 10:10:28 PST
Comment on attachment 273088 [details]
Patch 2/2

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

Other than concern about page map getting out of sync it looks good.

> Source/WebKit2/UIProcess/Automation/WebAutomationSession.cpp:105
> +    auto iter = m_handleWebPageMap.find(handle);

Nit: usually call this it or result

> Source/WebKit2/UIProcess/Automation/WebAutomationSession.cpp:108
> +    return WebProcessProxy::webPage(iter->value);

Does this return null for pages that died?

> Source/WebKit2/UIProcess/Automation/WebAutomationSession.cpp:117
> +    String handle = WebCore::createCanonicalUUIDString().convertToASCIIUppercase();

Nice!

> Source/WebKit2/UIProcess/Automation/WebAutomationSession.cpp:133
> +

Usually I try to never assign the out parameter until the very end so it is mutually exclusive to errorString being set. Not super important here though since there is no error case.

> Source/WebKit2/UIProcess/Automation/WebAutomationSession.cpp:159
> +        FAIL_WITH_PREDEFINED_ERROR_MESSAGE(InternalError);

You could add a more specific error code for this though its not required by spec.

> Source/WebKit2/UIProcess/Automation/WebAutomationSession.cpp:175
> +    page->tryClose();

There is likely some extra steps to take in case beforeunload dialog happens but we can punt for now.

> Source/WebKit2/UIProcess/Automation/WebAutomationSession.h:99
> +    HandleWebPageMap m_handleWebPageMap;

I think these maps could get out of sync if a process crashes or page dies in some other way not through this API.  Should we be more paranoid (I.e., recheck the process pool page list before handing out a WebPageProxy ref)?
Comment 7 Timothy Hatcher 2016-03-05 10:15:15 PST
Comment on attachment 273088 [details]
Patch 2/2

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

>> Source/WebKit2/UIProcess/Automation/WebAutomationSession.cpp:105
>> +    auto iter = m_handleWebPageMap.find(handle);
> 
> Nit: usually call this it or result

The other places I saw find being used in WebCore/WebKit mostly used iter.

>> Source/WebKit2/UIProcess/Automation/WebAutomationSession.cpp:108
>> +    return WebProcessProxy::webPage(iter->value);
> 
> Does this return null for pages that died?

Yep, it is a global map that is maintained by the lifetime of the page.

>> Source/WebKit2/UIProcess/Automation/WebAutomationSession.cpp:175
>> +    page->tryClose();
> 
> There is likely some extra steps to take in case beforeunload dialog happens but we can punt for now.

I figured the beforeunload would be covered by the alert endpoints.

>> Source/WebKit2/UIProcess/Automation/WebAutomationSession.h:99
>> +    HandleWebPageMap m_handleWebPageMap;
> 
> I think these maps could get out of sync if a process crashes or page dies in some other way not through this API.  Should we be more paranoid (I.e., recheck the process pool page list before handing out a WebPageProxy ref)?

The maps only store the uint64_t pageID for a WebPageProxy. We use WebProcessProxy::webPage() to look up the ID. It will not return a bad WebPageProxy ref.
Comment 8 BJ Burg 2016-03-05 10:19:27 PST
Comment on attachment 273088 [details]
Patch 2/2

r=me