[webkit-reviews] review granted: [Bug 179499] Add a FairPlay Streaming based CDM for Modern EME : [Attachment 326601] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 179499: Add a FairPlay Streaming based CDM for Modern EME
https://bugs.webkit.org/show_bug.cgi?id=179499

Attachment 326601: Patch

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




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

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

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:88
> +    SuccessValue setStorageDirectory(const String&) override;

Nit: final

> Source/WebCore/platform/graphics/avfoundation/CDMFairPlayStreaming.cpp:170
> +    factories.append(&CDMFactoryClearKey::singleton());

Nit: is there any reason to not add the Clear Key factory in
CDMFactory::registeredFactories instead of requiring every port to add it?

> Source/WebCore/platform/graphics/avfoundation/CDMFairPlayStreaming.cpp:229
> +	   && !WTF::anyOf(configuration.audioCapabilities, [](auto& capability)
{
> +	       return
CDMInstanceFairPlayStreamingAVFObjC::mimeTypeIsPlayable(capability.contentType)
;
> +	   }))
> +	   return false;

Do you want to only reject when none of the configurations is supported, or
should it fail if any is unsupported?

> Source/WebCore/platform/graphics/avfoundation/CDMFairPlayStreaming.cpp:235
> +	   && !WTF::anyOf(configuration.videoCapabilities, [](auto& capability)
{
> +	       return
CDMInstanceFairPlayStreamingAVFObjC::mimeTypeIsPlayable(capability.contentType)
;
> +	   }))
> +	   return false;

Ditto.

> Source/WebCore/platform/graphics/avfoundation/CDMFairPlayStreaming.h:40
> +    std::unique_ptr<CDMPrivate> createCDM(const String&) override;
> +    bool supportsKeySystem(const String&) override;

Nit: "override" => "final"

> Source/WebCore/platform/graphics/avfoundation/CDMFairPlayStreaming.h:66
> +    bool supportsInitDataType(const AtomicString&) const override;
> +    bool supportsConfiguration(const CDMKeySystemConfiguration&) const
override;
> +    bool supportsConfigurationWithRestrictions(const
CDMKeySystemConfiguration&, const CDMRestrictions&) const override;
> +    bool supportsSessionTypeWithConfiguration(CDMSessionType&, const
CDMKeySystemConfiguration&) const override;
> +    bool supportsRobustness(const String&) const override;
> +    CDMRequirement distinctiveIdentifiersRequirement(const
CDMKeySystemConfiguration&, const CDMRestrictions&) const override;
> +    CDMRequirement persistentStateRequirement(const
CDMKeySystemConfiguration&, const CDMRestrictions&) const override;
> +    bool distinctiveIdentifiersAreUniquePerOriginAndClearable(const
CDMKeySystemConfiguration&) const override;
> +    RefPtr<CDMInstance> createInstance() override;
> +    void loadAndInitialize() override;
> +    bool supportsServerCertificates() const override;
> +    bool supportsSessions() const override;
> +    bool supportsInitData(const AtomicString&, const SharedBuffer&) const
override;
> +    RefPtr<SharedBuffer> sanitizeResponse(const SharedBuffer&) const
override;
> +    std::optional<String> sanitizeSessionId(const String&) const override;

Ditto.

>
Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreaming
AVFObjC.mm:273
> +    RetainPtr<NSData> appIdentifier = m_serverCertificate ?
m_serverCertificate->createNSData() : nullptr;

Nit: auto appIdentifier = ...