[webkit-reviews] review granted: [Bug 212695] [EME] Create CDMProxyFactory : [Attachment 401897] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Xabier Rodríguez Calvar
<calvaris at igalia.com>'s request for review:
Bug 212695: [EME] Create CDMProxyFactory
https://bugs.webkit.org/show_bug.cgi?id=212695

Attachment 401897: Patch

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




--- Comment #23 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 401897
  --> https://bugs.webkit.org/attachment.cgi?id=401897
Patch

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

> Source/WebCore/platform/encryptedmedia/CDMProxy.h:161
> +    virtual ~CDMProxyFactory() { };

We could add an ASSERT that the factory is not contained in the list of
factories.
Or call unregisterFactory.

> Source/WebCore/platform/encryptedmedia/CDMProxy.h:169
> +    WEBCORE_EXPORT static void
platformRegisterFactories(Vector<CDMProxyFactory*>&);

Should we make it private then?
Maybe rename it to initializeRegisteredFactories().
Also we do not really like out parameters.
We could change it to return a Vector<> that we would move into
NeverDestroyed<Vector<CDMProxyFactory*>> factories.

> Source/WebCore/platform/encryptedmedia/CDMProxy.h:186
> +    CDMInstanceProxy(const String& keySystem)

explicit

> Source/WebCore/platform/encryptedmedia/CDMProxy.h:-177
> -	       m_cdmProxy->updateKeyStore(m_keyStore);

We remove that code, is it ok?

> Source/WebCore/platform/graphics/gstreamer/eme/CDMProxyClearKey.cpp:222
> +	   m_isGCryptInitialized = true;

This is very similar to an Optional.
I would make m_gCryptHandle an Optional so that we are sure we never use it
uninitialised.

> Source/WebCore/platform/graphics/gstreamer/eme/CDMProxyClearKey.h:48
> +    virtual ~CDMProxyFactoryClearKey();

No need for virtual here.

> Source/WebCore/platform/graphics/gstreamer/eme/CDMProxyClearKey.h:51
> +    bool supportsKeySystem(const String&) final;

Can we make them private?