Bug 176791 – [EME] Implement CDMInstanceClearKey::updateLicense()

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Zan Dobersek]

[EME] Implement CDMInstanceClearKey::updateLicense()

[Zan Dobersek]

Created attachment 320556 [details]
Patch

[Build Bot]

Attachment 320556 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:357:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Xabier Rodríguez Calvar]

Comment on attachment 320556 [details]
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.

[Zan Dobersek]

Comment on attachment 320556 [details]
Patch

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

>> 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.

I'd say only after, leaving an empty line before would look odd.

>> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:345
>> +        };
> 
> 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.

I'll add the comment.

>> 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.

Only the Key object is moved, in case there's difference between the existing and proposed keys' values, in either size or data.

[Zan Dobersek]

Created attachment 320623 [details]
Patch for landing

[Build Bot]

Attachment 320623 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:360:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zan Dobersek]

Comment on attachment 320623 [details]
Patch for landing

Clearing flags on attachment: 320623

Committed r221961: <http://trac.webkit.org/changeset/221961>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34693406>