[webkit-reviews] review granted: [Bug 133527] [iOS]=?UTF-8?Q?=20Client=2Dcertificate=20authentication=20isn=E2=80=99t=20working=20?=: [Attachment 232511] Add encoding support for credentials with identities

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted mitz at webkit.org <mitz at webkit.org>'s
request for review:
Bug 133527: [iOS] Client-certificate authentication isn’t working
https://bugs.webkit.org/show_bug.cgi?id=133527

Attachment 232511: Add encoding support for credentials with identities
https://bugs.webkit.org/attachment.cgi?id=232511&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=232511&action=review


> Source/WebCore/WebCore.exp.in:2919
> +#if PLATFORM(COCOA)

This file is only used for PLATFORM(COCOA), so I suggest removing this #if and
re-sorting the file.

> Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:488
> +#endif
>      encoder << credential.user() << credential.password();

I suggest adding a blank line here.

> Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:505
> +	   RetainPtr<CFArrayRef> certificates;

I’d move this down past the decoding of hasCertificates.

> Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:522
> +#endif
>      String user;

I suggest adding a blank line here.

> Source/WebKit2/Shared/cf/ArgumentCodersCF.cpp:587
> +    SecIdentityCopyCertificate(identity, &certificate);

Can this ever fail? If so, the CFRelease below will crash. Same pattern happens
3 times. Is crashing the right behavior or do we want something else?