Bug 167888 – [EME] Implement MediaKeySession::updateKeyStatuses(), MediaKeyStatusMap

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Zan Dobersek]

SSIA.

[Zan Dobersek]

Created attachment 300738 [details]
WIP.

[Zan Dobersek]

(In reply to comment #1)
> Created attachment 300738 [details]
> WIP.

Pretty much ready, just needs to use TextEncoder and the CDMInstance::KeyStatus -> MediaKeyStatus converter in a consolidated conversion header.

[Zan Dobersek]

Created attachment 300890 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 300890 [details]
Patch

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

> Source/WebCore/Modules/encryptedmedia/MediaKeyStatusMap.cpp:56
> +    if (!m_session)
> +        return 0;
> +    return m_session->statuses().size();

Matter of preference, do it if you want: return m_session ? m_session->statuses().size() : 0;

> Source/WebCore/Modules/encryptedmedia/MediaKeyStatusMap.cpp:70
> +            if (!length || length != keyId.length())
> +                return false;
> +            return !std::memcmp(it.first->data(), keyId.data(), length);

Ditto.

> Source/WebCore/Modules/encryptedmedia/MediaKeyStatusMap.cpp:86
> +        [&keyId] (auto& it) {
> +            auto length = it.first->size();
> +            if (!length || length != keyId.length())
> +                return false;
> +            return !std::memcmp(it.first->data(), keyId.data(), length);
> +        });

This is the second time I see this lambda. Maybe it would be better to turn it into a proper function. I know you want to capture keyId but you might use std::bind for it, right?

> Source/WebCore/testing/MockCDMFactory.h:76
> +    std::optional<const Vector<Ref<SharedBuffer>>&> keysForSessionWithID(const String& id) const;
>      Vector<Ref<SharedBuffer>> removeKeysFromSessionWithID(const String& id);

You don't need the parameter name

[Zan Dobersek]

Comment on attachment 300890 [details]
Patch

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

>> Source/WebCore/Modules/encryptedmedia/MediaKeyStatusMap.cpp:86
>> +        });
> 
> This is the second time I see this lambda. Maybe it would be better to turn it into a proper function. I know you want to capture keyId but you might use std::bind for it, right?

std::bind() is wasteful here. Lambda should still be used, since it's easy to inline directly into the std::find_if() call, but the 'matching logic' could be abstracted into a single function and then called from both lambdas.

>> Source/WebCore/testing/MockCDMFactory.h:76
>>      Vector<Ref<SharedBuffer>> removeKeysFromSessionWithID(const String& id);
> 
> You don't need the parameter name

It's used in every other function in this block. But it's true that below the parameters aren't verbosely listed anymore.

[Xabier Rodríguez Calvar]

(In reply to comment #5)

LGTM

[Zan Dobersek]

Created attachment 301055 [details]
Patch for landing

[WebKit Commit Bot]

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


ERROR: Source/WebCore/Modules/encryptedmedia/MediaKeyStatusMap.cpp:74:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/Modules/encryptedmedia/MediaKeyStatusMap.cpp:84:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 12 files


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

[Zan Dobersek]

Comment on attachment 301055 [details]
Patch for landing

Clearing flags on attachment: 301055

Committed r212107: <http://trac.webkit.org/changeset/212107>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.