[webkit-reviews] review granted: [Bug 97037] Bring WebKit up to speed with latest Encrypted Media spec. : [Attachment 187344] 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 97037: Bring WebKit up to speed with latest Encrypted Media spec.
https://bugs.webkit.org/show_bug.cgi?id=97037

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

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=187344&action=review


The WebCore ChangeLog entry is brilliant, the best ever in a patch I have
reviewed. thanks!

> Source/WebCore/ChangeLog:73
> +	   (WebCore::MediaKeySession::addKeyTimerFired): Follow the steps in
the spec; provide the key data t othe CDM.

Nit: "key data t othe CDM" -> "key data to the CDM"

> Source/WebCore/Modules/encryptedmedia/CDM.cpp:2
> + * Copyright (C) 2012 Apple Inc. All rights reserved.

2012 is so last year.

> Source/WebCore/Modules/encryptedmedia/CDM.cpp:27
> +#include "CDM.h"

Nit: this can be inside of the ENCRYPTED_MEDIA_V2 guard.

> Source/WebCore/Modules/encryptedmedia/CDM.cpp:58
> +	   // TODO: initialize specific UA CDMs.

Nit: "TODO" -> "FIXME", with bug number(s) if possible.

> Source/WebCore/Modules/encryptedmedia/CDM.cpp:76
> +    Vector<CDMFactory*>& cdmFactories = installedCDMFactories();
> +    for (size_t i = 0; i < cdmFactories.size(); ++i) {
> +	   if (cdmFactories[i]->supportsKeySystem(keySystem))
> +	       return true;
> +    }
> +    return false;

Couldn't this be reduced to "return CDMFactoryForKeySystem(keySystem)" ?

> Source/WebCore/Modules/encryptedmedia/CDM.cpp:79
> +static CDMFactory* CDMFactoryForKeySystem(const String& keySystem)

Does returned factory need to be mutable?

> Source/WebCore/Modules/encryptedmedia/CDM.cpp:109
> +    return m_private->supportsMIMEType(mimeType);

Won't m_private be NULL if it was initialized with an unsupported key system?

> Source/WebCore/Modules/encryptedmedia/CDM.cpp:114
> +    return m_private->createSession();

Ditto.

> Source/WebCore/Modules/encryptedmedia/CDM.h:2
> + * Copyright (C) 2012 Apple Inc. All rights reserved.

2013.

> Source/WebCore/Modules/encryptedmedia/CDM.h:44
> +class MediaKeyError;
> +class MediaKeySession;
> +class MediaKeys;
> +
> +class CDM;
> +class CDMPrivateInterface;
> +class CDMSession;

Nit: these could be alphabetized.

> Source/WebCore/Modules/encryptedmedia/CDMPrivate.h:2
> + * Copyright (C) 2012 Apple Inc. All rights reserved.

2013.

> Source/WebCore/Modules/encryptedmedia/MediaKeyMessageEvent.h:57
> +    virtual const AtomicString& interfaceName() const;

OVERRIDE.

> Source/WebCore/Modules/encryptedmedia/MediaKeyNeededEvent.h:56
> +    virtual const AtomicString& interfaceName() const;

OVERRIDE.

> Source/WebCore/Modules/encryptedmedia/MediaKeySession.cpp:2
> + * Copyright (C) 2012 Apple Inc. All rights reserved.

2013.

> Source/WebCore/Modules/encryptedmedia/MediaKeySession.h:2
> + * Copyright (C) 2012 Apple Inc. All rights reserved.

2013.

> Source/WebCore/Modules/encryptedmedia/MediaKeySession.idl:2
> + * Copyright (C) 2012 Apple Inc.  All rights reserved.

2013.

> Source/WebCore/Modules/encryptedmedia/MediaKeys.cpp:2
> + * Copyright (C) 2012 Apple Inc. All rights reserved.

2013.

> Source/WebCore/Modules/encryptedmedia/MediaKeys.h:2
> + * Copyright (C) 2012 Apple Inc. All rights reserved.

2013.

> Source/WebCore/Modules/encryptedmedia/MediaKeys.idl:2
> + * Copyright (C) 2012 Apple Inc.  All rights reserved.

2013.

> Source/WebCore/html/HTMLMediaElement.cpp:1971
>  }

It is probably worth logging a "deprecated!" warning to console the first time
this is used in a build which defines ENCRYPTED_MEDIA_V2.

> Source/WebCore/html/HTMLMediaElement.h:454
>      virtual bool mediaPlayerKeyNeeded(MediaPlayer*, const String& keySystem,
const String& sessionId, const unsigned char* initData, unsigned
initDataLength) OVERRIDE;

OVERRIDE.

> Source/WebCore/html/HTMLMediaElement.h:458
> +    virtual void mediaPlayerKeyNeeded(MediaPlayer*, Uint8Array*);

OVERRIDE.

> Source/WebCore/platform/graphics/MediaPlayer.cpp:1082
>  }
>  #endif

Log a "deprecated!" warning to console the first time this is used in a build
which defines ENCRYPTED_MEDIA_V2.

> Source/WebCore/testing/MockCDM.cpp:2
> + * Copyright (C) 2012 Apple Inc. All rights reserved.

2013.

> Source/WebCore/testing/MockCDM.h:2
> + * Copyright (C) 2012 Apple Inc. All rights reserved.

2013.

> LayoutTests/media/encrypted-media/encrypted-media-v2-events.html:11
> +		   for(var i=0,j=str.length;i<j;++i){
> +		     arr[i]=str.charCodeAt(i);

Nit: braces aren't needed.

> LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:12
> +		   for(var i=0,j=str.length;i<j;++i){
> +		     arr[i]=str.charCodeAt(i);
> +		   }

Ditto.