[webkit-reviews] review granted: [Bug 175315] WebDriver: timeout when JavaScript alert is shown in onload handler : [Attachment 318038] Updated patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 175315: WebDriver: timeout when JavaScript alert is shown in onload handler
https://bugs.webkit.org/show_bug.cgi?id=175315

Attachment 318038: Updated patch

https://bugs.webkit.org/attachment.cgi?id=318038&action=review




--- Comment #8 from Brian Burg <bburg at apple.com> ---
Comment on attachment 318038
  --> https://bugs.webkit.org/attachment.cgi?id=318038
Updated patch

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

r=me

> Source/WebKit/ChangeLog:14
> +	   expects an alert it will just work, otherwise it will fail with
UnexpectedAlertOpen error when trying ot handle

Nit: trying to

> Source/WebKit/ChangeLog:29
> +	   wewait until the next run loop iteration to give time for the client
to show the dialog, then check if page is

Nit: we wait

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:415
> +    // we return without waiting since we know it will timeout or sure. We
want to check

Nit: for sure

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:417
> +    bool shouldTimeoutDueToUnexpectedAlert = pageLoadStrategy.value() ==
Inspector::Protocol::Automation::PageLoadStrategy::Normal

Nice.

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:504
> +	   for (auto& process : m_processPool->processes()) {

This could be its own static method to keep the loop simple.

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:512
> +	       callback->sendSuccess(InspectorObject::create());

Will this return "data null" in the driver endpoint per step 8?

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:532
> +	   if (!page->isValid() || !page->pageLoadState().isLoading() ||
!m_client->isShowingJavaScriptDialogOnPage(*this, page))

What if the session was destroyed? Do we need a guard for that and a protectRef
for |this|?