Bug 154953 – Add _WKAutomationSessionDelegate methods for opening, closing, and switching windows

• Component: WebKit2
• Status: RESOLVED FIXED

[Blaze Burg]

Start fleshing out the commands that are currently stubbed.

[Radar WebKit Bug Importer]

<rdar://problem/24947489>

[Blaze Burg]

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

[Timothy Hatcher]

Created attachment 273087 [details]
Patch 1/2

[Timothy Hatcher]

Created attachment 273088 [details]
Patch 2/2

[Blaze Burg]

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

[Blaze Burg]

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)?

[Timothy Hatcher]

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.

[Blaze Burg]

Comment on attachment 273088 [details]
Patch 2/2

r=me

[Timothy Hatcher]

https://trac.webkit.org/r197620
https://trac.webkit.org/r197621