[webkit-reviews] review denied: [Bug 212457] [EME] Get CDMProxy and friends ready for more CDMs : [Attachment 400485] Patch

[bugzilla-daemon at webkit.org]

Charlie Turner <cturner at igalia.com> has denied	review:
Bug 212457: [EME] Get CDMProxy and friends ready for more CDMs
https://bugs.webkit.org/show_bug.cgi?id=212457

Attachment 400485: Patch

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




--- Comment #10 from Charlie Turner <cturner at igalia.com> ---
Comment on attachment 400485
  --> https://bugs.webkit.org/attachment.cgi?id=400485
Patch

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

Overall I think this might need splitting further, bugs with their tests first,
then OpenCDM with the scaffolding needed for it.

> Source/WebCore/ChangeLog:33
> +	   No new tests, just a rework covered by the existing ones.

There are bug fixes mixed into reworks here. For example, the session tracking
bugs you found should be testable, since you noticed them during a manual test
not in the test suite? What tests make sure the weakptr tracking code in this
patch won't break in the future? Can we test that? If not an explanation here
would be helpful.

> Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:118
> +bool KeyHandle::takeValue(KeyHandleValueVariant&& value)

takeValueIfDifferent

> Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:127
> +bool KeyHandle::takeData(const RefPtr<KeyHandle>& other, bool shouldCheckID)

This looks like a copy ctor in disguise.

shouldCheckID is also always false in this patch, so that should be left for a
later patch when it's used. For example, with no ChangeLog information, it's
not at all obvious why we copy ids sometimes but not in others.

> Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:149
> +void KeyStore::setEqualIDMergeStrategy(EqualIDMergeStrategy strategy)

This pattern boilerplate is not achieving anything? I think this patch needs
splitting up further, and things like Variant's and strategy patterns can be
included in a patch where they are used and reward with clarity.

> Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:223
> +	   didStoreChange = keyWithMatchingKeyID->takeData(key);

It doesn't make sense for the "key store" to have changed when you "take data"
from a key. The "did change" logic should be kept in the store, not in the keys
themselves.

> Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:271
> +void KeyStore::replaceEqualIDMergeStrategy(RefPtr<KeyHandle>& destination,
const RefPtr<KeyHandle>& origin)

This "replaces" data based on creation times, not IDs. It's misnamed and
confusing why creation times are even of interest.

> Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:275
> +	   destination->takeData(origin);

Here the return value is unused.

> Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:335
> +RefPtr<KeyHandle> CDMProxy::tryWaitForKeyHandle(const KeyIDType& keyID)
const

Why do move away from Optional<> and back to NULLs?

> Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:373
> +RefPtr<KeyHandle> CDMProxy::getOrWaitForKeyHandle(const KeyIDType& keyID)
const

Optional<> seems better

> Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:386
> +    if (keyHandle)

if (auto keyHandle = ...)

> Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:446
> +    purgeNullSessions();

You need to clearly explain this in the ChangeLog, the session lifetime issues
you found, etc. Also, a test should be provided to check it's working.

> Source/WebCore/platform/encryptedmedia/CDMProxy.h:44
> +    Vector<uint8_t>

Only 1 type in a Variant?

> Source/WebCore/platform/encryptedmedia/CDMProxy.h:57
> +    bool takeData(const RefPtr<KeyHandle>&, bool shouldCheckID = false);

As I note in the call-sites, this seems wrong. It's used inconsistently and
takes responsibility of key store changes, which it should not know about.

> Source/WebCore/platform/encryptedmedia/CDMProxy.h:107
> +    WallTime m_creationTime { WallTime::now() };

Please explain the need for that in the ChangeLog.

> Source/WebCore/platform/encryptedmedia/CDMProxy.h:159
> +    static constexpr Seconds MaxKeyWaitTimeSeconds = 7_s;

What caused that? 5 seemed like an eternity.

> Source/WebCore/platform/encryptedmedia/CDMProxy.h:200
> +    // registered CDMProxyFactory objects is queried for the first time.

Better to explain why it's needed than what it is.

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:465
> +    CDMInstanceSessionClearKey* newSession = new
CDMInstanceSessionClearKey(*this);

auto*

> Source/WebCore/platform/graphics/gstreamer/eme/CDMProxyClearKey.cpp:132
> +    auto keyDataValue = WTF::get<Vector<uint8_t>>(keyData.value());

I don't know the details of Variant's implementation, but I'm scared this a
copy. auto&?