[webkit-reviews] review granted: [Bug 231789] WebDriver: [Cocoa] support `acceptInsecureCerts` capability : [Attachment 441976] Patch v1.1

[bugzilla-daemon at webkit.org]

BJ Burg <bburg at apple.com> has granted Patrick Angle <pangle at apple.com>'s
request for review:
Bug 231789: WebDriver: [Cocoa] support `acceptInsecureCerts` capability
https://bugs.webkit.org/show_bug.cgi?id=231789

Attachment 441976: Patch v1.1

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




--- Comment #3 from BJ Burg <bburg at apple.com> ---
Comment on attachment 441976
  --> https://bugs.webkit.org/attachment.cgi?id=441976
Patch v1.1

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

The code changes look good to me. Would be good to get signoff from Alex, John,
or another NetworkProcess expert.

I think the changelog should explain why this is a one-way setting, as that
differs from the other capabilities we handle in the same way
(AllowInsecureMediaCaptureCapabilityKey).

> Source/WebKit/ChangeLog:12
> +	   to allow the load to continue as if the certificate is trusted,
regardless of the actual trustworthyness of the

Nit: trustworthiness

> Source/WebKit/NetworkProcess/NetworkProcess.h:462
> +    void acceptInsecureCertificates(PAL::SessionID);

(applies throughout) 

I would add a suffix like ForWebDriver or ForAutomation to make it clear that
this is edge-case territory.

> Source/WebKit/UIProcess/Cocoa/AutomationClient.mm:81
> +    [configuration
setAcceptInsecureCertificates:sessionCapabilities.acceptInsecureCertificates];

Nit: doing an if check would make this line up with the other capability
forwarding below.