[webkit-reviews] review granted: [Bug 223017] Web Automation: Get Title command sometimes hangs inside evaluateJavaScriptFunction : [Attachment 422806] Patch v1.0

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted BJ Burg <bburg at apple.com>'s
request for review:
Bug 223017: Web Automation: Get Title command sometimes hangs inside
evaluateJavaScriptFunction
https://bugs.webkit.org/show_bug.cgi?id=223017

Attachment 422806: Patch v1.0

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




--- Comment #4 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 422806
  --> https://bugs.webkit.org/attachment.cgi?id=422806
Patch v1.0

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

r=me, nice fix :)

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:771
> +	   // would no longer be valid and the command would also fail also
with WindowNotFound, just earlier.

"also fail also"

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:774
> +	       for (auto key :
copyToVector(m_evaluateJavaScriptFunctionCallbacks.keys())) {
> +		   auto callback =
m_evaluateJavaScriptFunctionCallbacks.take(key);

Rather than `copyToVector`, can you `std::exchange`?
```
    for (auto&& [key, callback] :
std::exchange(m_evaluateJavaScriptFunctionCallbacks, { }))
	ASYNC_FAIL_WITH_PREDEFINED_ERROR(WindowNotFound);
```
I think you could also drop the `isEmpty` check if you do this too.

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:775
> +		   ASYNC_FAIL_WITH_PREDEFINED_ERROR(WindowNotFound);

I believe this will `return`.  Based on your comment it sounds like we want
that, but I just want to make sure.