[webkit-reviews] review granted: [Bug 196168] Web Automation: clean up some WebAutomationSession methods to use modern async IPC : [Attachment 365891] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Brian Burg <bburg at apple.com>'s
request for review:
Bug 196168: Web Automation: clean up some WebAutomationSession methods to use
modern async IPC
https://bugs.webkit.org/show_bug.cgi?id=196168

Attachment 365891: Patch

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




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

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

r=me, most of my comments are applicable in more than just that spot, so if
they are addressed please try to address them everywhere :)

> Source/WebKit/ChangeLog:9
> +	   So, most messages between WebAutomationSession and its proxy can use
this facility, and stop

NIT: unnecessary comma => "facility and stop".

> Source/WebKit/ChangeLog:16
> +	   be able to cancel all pending replies when a page navigates away,
the web process crashes,
> +	   or when handling an alert.

NIT: I normally indent subsequent lines to make it clear where each bullet
point begins/ends.

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:973
> +    WTF::CompletionHandler<void(Optional<String>, uint64_t)>
completionHandler = [this, protectedThis = makeRef(*this), callback =
callback.copyRef()](Optional<String> errorType, uint64_t frameID) mutable {

Given that the `callback` is a `&&`, should we move it into the lambda (e.g.
`callback = WTFMove(callback)`)?

Also, can you use `auto completionHandler` or is it necessary that you specify
the type?

Style: there should be a space between the "](".

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:975
> +	      
callback->sendFailure(STRING_FOR_PREDEFINED_ERROR_MESSAGE(*errorType));

Style: I prefer using `errorType.value()` for `Optional`s.

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1010
> +    WTF::CompletionHandler<void(Optional<String>, uint64_t)>
completionHandler = [this, protectedThis = makeRef(*this), callback =
callback.copyRef()](Optional<String> errorType, uint64_t frameID) mutable {

Could you inline this, since it's only used once (and that usage is a
`WTFMove`)?