<rdar://36878038>
Created attachment 441312 [details] Patch v1.0
Created attachment 441976 [details] Patch v1.1
Comment on attachment 441976 [details] 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.
Comment on attachment 441976 [details] Patch v1.1 View in context: https://bugs.webkit.org/attachment.cgi?id=441976&action=review >> 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. The other capability forwarding checks below actually have their `sessionCapabilities` properties implemented as optional booleans, but the existing `sessionCapabilities.acceptInsecureCertificates` is implemented a boolean, so the outer `if` check isn't necessary here.
Created attachment 442056 [details] Patch v1.2 - Address BJ's review notes
Comment on attachment 442056 [details] Patch v1.2 - Address BJ's review notes View in context: https://bugs.webkit.org/attachment.cgi?id=442056&action=review > Source/WebKit/NetworkProcess/NetworkSession.h:146 > + void acceptInsecureCertificatesForAutomation() { m_acceptInsecureCertificates = true; } The naming is a bit confusing. This looks a lot like a getter name. > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h:89 > +- (void)_acceptInsecureCertificatesForAutomation WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA)); shouldn't it be a flag on the _WKWebsiteDataStoreConfiguration so that you could only set it at creation time. Seems weird to have a method on the WKWebsiteDataStore when you can only set it to YES (never back to NO).
Comment on attachment 442056 [details] Patch v1.2 - Address BJ's review notes View in context: https://bugs.webkit.org/attachment.cgi?id=442056&action=review > Source/WebKit/NetworkProcess/NetworkProcess.cpp:2164 > +void NetworkProcess::acceptInsecureCertificatesForAutomation(PAL::SessionID sessionID) This isn't needed, as discussed offline. This state can be kept in the UI process. Please remove.
Created attachment 442062 [details] Patch v1.3 - Remove NetworkProcess changes
Comment on attachment 442062 [details] Patch v1.3 - Remove NetworkProcess changes r=me
Committed r285164 (243800@main): <https://commits.webkit.org/243800@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 442062 [details].