Bug 167888 - [EME] Implement MediaKeySession::updateKeyStatuses(), MediaKeyStatusMap
Summary: [EME] Implement MediaKeySession::updateKeyStatuses(), MediaKeyStatusMap
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords:
Depends on:
Blocks: 158841
  Show dependency treegraph
 
Reported: 2017-02-06 10:06 PST by Zan Dobersek
Modified: 2017-02-10 02:24 PST (History)
3 users (show)

See Also:


Attachments
WIP. (27.52 KB, patch)
2017-02-06 10:55 PST, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (35.86 KB, patch)
2017-02-08 04:40 PST, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch for landing (35.81 KB, patch)
2017-02-09 10:51 PST, Zan Dobersek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2017-02-06 10:06:58 PST
SSIA.
Comment 1 Zan Dobersek 2017-02-06 10:55:36 PST
Created attachment 300738 [details]
WIP.
Comment 2 Zan Dobersek 2017-02-06 10:57:48 PST
(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.
Comment 3 Zan Dobersek 2017-02-08 04:40:32 PST
Created attachment 300890 [details]
Patch
Comment 4 Xabier Rodríguez Calvar 2017-02-09 07:27:48 PST
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
Comment 5 Zan Dobersek 2017-02-09 08:18:49 PST
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.
Comment 6 Xabier Rodríguez Calvar 2017-02-09 08:27:42 PST
(In reply to comment #5)

LGTM
Comment 7 Zan Dobersek 2017-02-09 10:51:36 PST
Created attachment 301055 [details]
Patch for landing
Comment 8 WebKit Commit Bot 2017-02-09 10:52:56 PST
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.
Comment 9 Zan Dobersek 2017-02-10 02:24:25 PST
Comment on attachment 301055 [details]
Patch for landing

Clearing flags on attachment: 301055

Committed r212107: <http://trac.webkit.org/changeset/212107>
Comment 10 Zan Dobersek 2017-02-10 02:24:34 PST
All reviewed patches have been landed.  Closing bug.