[webkit-reviews] review granted: [Bug 189725] [EME] Introduce the concept of CDMInstanceSession. : [Attachment 350127] 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 189725: [EME] Introduce the concept of CDMInstanceSession.
https://bugs.webkit.org/show_bug.cgi?id=189725

Attachment 350127: Patch

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




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

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

r=me once the bots are happy

> Source/WebCore/platform/encryptedmedia/CDMInstanceSession.h:72
> +    virtual void updateLicense(const String& sessionId, LicenseType, const
SharedBuffer& response, LicenseUpdateCallback) = 0;

Nit: LicenseUpdateCallback&&

> Source/WebCore/platform/encryptedmedia/CDMInstanceSession.h:83
> +    virtual void loadSession(LicenseType, const String& sessionId, const
String& origin, LoadSessionCallback) = 0;

Ditto.

> Source/WebCore/platform/encryptedmedia/CDMInstanceSession.h:86
> +    virtual void closeSession(const String& sessionId, CloseSessionCallback)
= 0;

Nit: CloseSessionCallback&&

> Source/WebCore/platform/encryptedmedia/CDMInstanceSession.h:89
> +    virtual void removeSessionData(const String& sessionId, LicenseType,
RemoveSessionDataCallback) = 0;

Nit: RemoveSessionDataCallback&&

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:497
> +    static const NeverDestroyed<String> s_keySystem =
MAKE_STATIC_STRING_IMPL("org.w3.clearkey");

Nit: you use the brace-initialization style to
CDMInstanceFairPlayStreamingAVFObjC::keySystem, both should use the same.

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:510
> +    Vector<CDMInstanceSessionClearKey::Key> allKeys { };

Nit: allKeys.reserveInitialCapacity( ... )

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:537
> +void CDMInstanceSessionClearKey::updateLicense(const String& sessionId,
LicenseType, const SharedBuffer& response, LicenseUpdateCallback callback)

LicenseUpdateCallback&&

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:638
> +void CDMInstanceSessionClearKey::loadSession(LicenseType, const String&
sessionId, const String&, LoadSessionCallback callback)

Ditto.

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:671
> +void CDMInstanceSessionClearKey::closeSession(const String&,
CloseSessionCallback callback)

CloseSessionCallback&&

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:682
> +void CDMInstanceSessionClearKey::removeSessionData(const String& sessionId,
LicenseType, RemoveSessionDataCallback callback)

RemoveSessionDataCallback&&

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:108
> +    void requestLicense(LicenseType, const AtomicString& initDataType,
Ref<SharedBuffer>&& initData, LicenseCallback) final;
> +    void updateLicense(const String&, LicenseType, const SharedBuffer&,
LicenseUpdateCallback) final;
> +    void loadSession(LicenseType, const String&, const String&,
LoadSessionCallback) final;
> +    void closeSession(const String&, CloseSessionCallback) final;
> +    void removeSessionData(const String&, LicenseType,
RemoveSessionDataCallback) final;

Ditto.

>
Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreaming
AVFObjC.h:112
> +    void requestLicense(LicenseType, const AtomicString& initDataType,
Ref<SharedBuffer>&& initData, LicenseCallback) final;
> +    void updateLicense(const String&, LicenseType, const SharedBuffer&,
LicenseUpdateCallback) final;
> +    void loadSession(LicenseType, const String&, const String&,
LoadSessionCallback) final;
> +    void closeSession(const String&, CloseSessionCallback) final;
> +    void removeSessionData(const String&, LicenseType,
RemoveSessionDataCallback) final;

Ditto.

>
Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreaming
AVFObjC.h:123
> +    void didProvideRequest(AVContentKeyRequest *);
> +    void didProvideRenewingRequest(AVContentKeyRequest *);
> +    void didProvidePersistableRequest(AVContentKeyRequest *);
> +    void didFailToProvideRequest(AVContentKeyRequest *, NSError *);
> +    void requestDidSucceed(AVContentKeyRequest *);
> +    bool shouldRetryRequestForReason(AVContentKeyRequest *, NSString *);
> +    void sessionIdentifierChanged(NSData *);

Nit: our pedantic style guide wants no space between type and * in .cpp code.

>
Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreaming
AVFObjC.mm:247
> +    static NeverDestroyed<String> s_keySystem { "com.apple.fps"_s };

Initialization style.

>
Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreaming
AVFObjC.mm:402
> +void CDMInstanceSessionFairPlayStreamingAVFObjC::updateLicense(const
String&, LicenseType, const SharedBuffer& responseData, LicenseUpdateCallback
callback)

LicenseUpdateCallback&&

>
Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreaming
AVFObjC.mm:448
> +void CDMInstanceSessionFairPlayStreamingAVFObjC::loadSession(LicenseType
licenseType, const String& sessionId, const String& origin, LoadSessionCallback
callback)

Ditto.

>
Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreaming
AVFObjC.mm:487
> +void CDMInstanceSessionFairPlayStreamingAVFObjC::closeSession(const String&,
CloseSessionCallback callback)

CloseSessionCallback&&

>
Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreaming
AVFObjC.mm:505
> +void CDMInstanceSessionFairPlayStreamingAVFObjC::removeSessionData(const
String& sessionId, LicenseType licenseType, RemoveSessionDataCallback callback)

RemoveSessionDataCallback&&

> Source/WebCore/testing/MockCDMFactory.cpp:274
> +    static const NeverDestroyed<String> s_keySystem =
MAKE_STATIC_STRING_IMPL("org.webkit.mock");

Initialization style.

> Source/WebCore/testing/MockCDMFactory.cpp:315
> +void MockCDMInstanceSession::updateLicense(const String& sessionID,
LicenseType, const SharedBuffer& response, LicenseUpdateCallback callback)

LicenseUpdateCallback&&

> Source/WebCore/testing/MockCDMFactory.cpp:349
> +void MockCDMInstanceSession::loadSession(LicenseType, const String&, const
String&, LoadSessionCallback callback)

Ditto.

> Source/WebCore/testing/MockCDMFactory.cpp:365
> +void MockCDMInstanceSession::closeSession(const String& sessionID,
CloseSessionCallback callback)

Ditto.

> Source/WebCore/testing/MockCDMFactory.cpp:377
> +void MockCDMInstanceSession::removeSessionData(const String& id,
LicenseType, RemoveSessionDataCallback callback)

Ditto.

> Source/WebCore/testing/MockCDMFactory.h:155
>      void requestLicense(LicenseType, const AtomicString& initDataType,
Ref<SharedBuffer>&& initData, LicenseCallback) final;
>      void updateLicense(const String&, LicenseType, const SharedBuffer&,
LicenseUpdateCallback) final;
>      void loadSession(LicenseType, const String&, const String&,
LoadSessionCallback) final;

Ditto.

> Source/WebCore/testing/MockCDMFactory.h:157
>      void removeSessionData(const String&, LicenseType,
RemoveSessionDataCallback) final;

Ditto.