[webkit-reviews] review granted: [Bug 176791] [EME] Implement CDMInstanceClearKey::updateLicense() : [Attachment 320556] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has granted Zan Dobersek
<zan at falconsigh.net>'s request for review:
Bug 176791: [EME] Implement CDMInstanceClearKey::updateLicense()
https://bugs.webkit.org/show_bug.cgi?id=176791

Attachment 320556: Patch

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




--- Comment #3 from Xabier Rodríguez Calvar <calvaris at igalia.com> ---
Comment on attachment 320556
  --> https://bugs.webkit.org/attachment.cgi?id=320556
Patch

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

Really nice!

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:47
> +    using KeyStore = HashMap<String, Vector<CDMInstanceClearKey::Key>>;

I would leave blank lines before and after this line.

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:345
> +    auto dispatchCallback =
> +	   [this, &callback](bool sessionWasClosed,
std::optional<KeyStatusVector>&& changedKeys, SuccessValue succeeded) {
> +	       callOnMainThread(
> +		   [weakThis = m_weakPtrFactory.createWeakPtr(), callback =
WTFMove(callback), sessionWasClosed, changedKeys = WTFMove(changedKeys),
succeeded] () mutable {
> +		       if (!weakThis)
> +			   return;
> +
> +		       callback(sessionWasClosed, WTFMove(changedKeys),
std::nullopt, std::nullopt, succeeded);
> +		   });
> +	   };

Two lambdas in the same variable declaration cause you can! :))) I'd add a
comment explaining that the inner lambda is the one executed in the main thread
and the outter one is a way to avoid having that line duplicated several times
in the following code.

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:373
> +		   if (existingKey->size() != proposedKey->size() ||
std::memcmp(existingKey->data(), proposedKey->data(), existingKey->size())) {

I'd add a comment here cause we actually try to move the memory if sizes are
equal but we do it in the if clause, and then if that fails, we move the key. I
think this way is the most elegant way of doing it, but I think it still
requires some clarification.