[webkit-reviews] review granted: [Bug 227731] PCM: Add error logging for CryptoKit operations : [Attachment 433106] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has granted John Wilander
<wilander at apple.com>'s request for review:
Bug 227731: PCM: Add error logging for CryptoKit operations
https://bugs.webkit.org/show_bug.cgi?id=227731

Attachment 433106: Patch

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




--- Comment #6 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 433106
  --> https://bugs.webkit.org/attachment.cgi?id=433106
Patch

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

r=me, but consider my suggestion about the test case.

> Tools/TestWebKitAPI/Tests/WebCore/PrivateClickMeasurement.cpp:323
> +   
EXPECT_FALSE(pcm.calculateAndUpdateSourceUnlinkableToken(serverPublicKeyBase64U
RL));

I think these tests would look better if you asserted
EXPECT_FALSE(pcm.calculateAndUpdateSourceUnlinkableToken(serverPublicKeyBase64U
RL).has_value());

The fact that the return value is an error message (or no-value if successful)
isn't obvious from the name, so it looks like we are expecting the
calculateAndUpdate to fail.