[webkit-reviews] review granted: [Bug 98090] Add LSKD support to MediaPlayerPrivateAVFoundation. : [Attachment 166576] Patch

[bugzilla-daemon at webkit.org]

Anders Carlsson <andersca at apple.com> has granted Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 98090: Add LSKD support to MediaPlayerPrivateAVFoundation.
https://bugs.webkit.org/show_bug.cgi?id=98090

Attachment 166576: Patch
https://bugs.webkit.org/attachment.cgi?id=166576&action=review

------- Additional Comments from Anders Carlsson <andersca at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=166576&action=review


>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:130
> +    static dispatch_queue_t globalQueue = 0;

No need to initialize the queue here.

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:133
> +	   globalQueue = dispatch_queue_create(0, DISPATCH_QUEUE_SERIAL);

I think you should give this queue a name.

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:789
> +    do {
> +	   // 1. Check whether the Key System is supported with the specified
container and codec type(s) by following the steps for the first matching
condition from the following list:
> +	   //	 If keySystem is null, continue to the next step.
> +	   if (keySystem.isNull() || keySystem.isEmpty())
> +	       break;
> +
> +	   // If keySystem contains an unrecognized or unsupported Key System,
return the empty string
> +	   if (!keySystemIsSupported(keySystem))
>	       return MediaPlayer::IsNotSupported;
> +
> +	   // If the Key System specified by keySystem does not support
decrypting the container and/or codec specified in the rest of the type string.

> +	   // (AVFoundation does not provide an API which would allow us to
determine this, so this is a no-op)
> +
> +    } while(false);

I think you should put this in a separate helper function instead of using do {
} while (false).

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:992
> +static bool extractKeyURIKeyIdAndCertificateFromInitData(Uint8Array*
initData, String& keyURI, String& keyId, RefPtr<Uint8Array>& certificate)

ID should be all caps.

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:1049
> +    String keyId;

keyID.

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:1084
> +MediaPlayer::MediaKeyException
MediaPlayerPrivateAVFoundationObjC::addKey(const String& keySystem, const
unsigned char* keyPtr, unsigned keyLength, const unsigned char* initDataPtr,
unsigned initDataLength, const String& sessionId)

sessionID.

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:1104
> +MediaPlayer::MediaKeyException
MediaPlayerPrivateAVFoundationObjC::cancelKeyRequest(const String& keySystem,
const String& sessionId)

sessionID.