[webkit-reviews] review granted: [Bug 218752] Add a WebRTC SFrame transform : [Attachment 414038] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted youenn fablet
<youennf at gmail.com>'s request for review:
Bug 218752: Add a WebRTC SFrame transform
https://bugs.webkit.org/show_bug.cgi?id=218752

Attachment 414038: Patch

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




--- Comment #10 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 414038
  --> https://bugs.webkit.org/attachment.cgi?id=414038
Patch

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

> Source/WebCore/Modules/mediastream/RTCRtpSFrameTransform.cpp:56
> +	   promise.reject(Exception { TypeError, "Invalid key"_s });
> +	   return;
> +    }
> +
> +    if (WTF::get<CryptoKeyAlgorithm>(algorithm).name != "HKDF") {
> +	   promise.reject(Exception { TypeError, "Invalid key"_s });

It might be helpful to have different error strings to help a developer
understand the failure.

> Source/WebCore/Modules/mediastream/RTCRtpSFrameTransform.cpp:75
> +    backend.setTransformableFrameCallback([transformer = m_transformer,
backend = makeRef(backend)](auto&& frame) {
> +	   auto chunk = frame.data();

Is there no chance the transform can be destroyed before the callback fires?

> Source/WebCore/Modules/mediastream/RTCRtpSFrameTransformer.cpp:89
> +    if (firstByte & 0x80)

Nit: it might be nice to put this and the other bit tests below into a methods
to make it clear what they do.

> Source/WebCore/Modules/mediastream/RTCRtpSFrameTransformer.h:2
> + * Copyright (C) 2018 Apple Inc. All rights reserved.

s/2018/2020/

> Tools/TestWebKitAPI/Tests/WebCore/RTCRtpSFrameTransformerTests.cpp:2
> + * Copyright (C) 2014-2016 Apple Inc. All rights reserved.

s/2014-2016/2020/