Bug 166880 – Add support for MediaKeys.generateRequest().

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

Add support for MediaKeys.generateRequest().

[Jer Noble]

Created attachment 298448 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 298448 [details]
Patch

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

I see this patch mostly ok that's why I r+. I don't mind another pair of eyes though.

> Source/WebCore/Modules/encryptedmedia/InitDataRegistry.cpp:85
> +    auto object = InspectorObject::create();
> +    auto kidsArray = InspectorArray::create();

I usually don't like the auto for plain variables, I would type the object explicitly.

> Source/WebCore/Modules/encryptedmedia/MediaKeys.cpp:93
> +    auto certificate = SharedBuffer::create(serverCertificate.data(), serverCertificate.length());

Specify the type, please.

Yes, I hate autos :)

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:5720
> +		CD063F821E23FA8900812BE3 /* InitDataRegistry.cpp in Sources */ = {isa = PBXBuildFile; fileRef = CD063F801E23FA8900812BE3 /* InitDataRegistry.cpp */; };
> +		CD063F831E23FA8900812BE3 /* InitDataRegistry.h in Headers */ = {isa = PBXBuildFile; fileRef = CD063F811E23FA8900812BE3 /* InitDataRegistry.h */; };

When will you move to CMake? Seven lines vs 1 in cmake ;-p

> Source/WebCore/testing/MockCDMFactory.cpp:246
> +    auto keyIDs = InitDataRegistry::shared().extractKeyIDs(initDataType, initData);

Please specify the type.

> LayoutTests/media/encrypted-media/mock-MediaKeySession-generateRequest.html:60
> +    function shouldResolve(promise) {
> +        promise.then(mediaKeySystemAccess => {
> +            logResult(Success, 'Promise resolved');
> +            next();
> +        }, () => {
> +            logResult(Failed, 'Promise rejected');
> +            next();
> +        });
> +    }
> +
> +    function shouldReject(promise) {
> +        promise.then(() => {
> +            logResult(Failed, 'Promise resolved incorrectly');
> +            next();
> +        }, exceptionCode => {
> +            logResult(Success, 'Promise rejected correctly');
> +            next();
> +        });
> +    }

It would be nice to have these in a different file cause I am sure they will be needed somewhere else too.

[Jer Noble]

Comment on attachment 298448 [details]
Patch

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

Thanks! Comments below.

>> Source/WebCore/Modules/encryptedmedia/InitDataRegistry.cpp:85
>> +    auto kidsArray = InspectorArray::create();
> 
> I usually don't like the auto for plain variables, I would type the object explicitly.

I've received precisely the opposite review feedback from Darin.

>> Source/WebCore/Modules/encryptedmedia/MediaKeys.cpp:93
>> +    auto certificate = SharedBuffer::create(serverCertificate.data(), serverCertificate.length());
> 
> Specify the type, please.
> 
> Yes, I hate autos :)

I see. :)

But auto is the future, and in situations where the type is immediately inferable (like in SharedBuffer::create()), appropriate..

>> Source/WebCore/WebCore.xcodeproj/project.pbxproj:5720
>> +		CD063F831E23FA8900812BE3 /* InitDataRegistry.h in Headers */ = {isa = PBXBuildFile; fileRef = CD063F811E23FA8900812BE3 /* InitDataRegistry.h */; };
> 
> When will you move to CMake? Seven lines vs 1 in cmake ;-p

Xcode makes this automatic from File->New File. It's only for other build systems that I have to explicitly edit a build file.

>> LayoutTests/media/encrypted-media/mock-MediaKeySession-generateRequest.html:60
>> +    }
> 
> It would be nice to have these in a different file cause I am sure they will be needed somewhere else too.

Sure, i'm using them a lot in these mock EME tests.

[Eric Carlson]

Comment on attachment 298448 [details]
Patch

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

>> Source/WebCore/Modules/encryptedmedia/InitDataRegistry.cpp:85
>> +    auto kidsArray = InspectorArray::create();
> 
> I usually don't like the auto for plain variables, I would type the object explicitly.

While I am not in the "always use auto" camp, I think it makes sense in cases like this where the type is clear from the context.

> Source/WebCore/Modules/encryptedmedia/InitDataRegistry.cpp:91
> +    String json = object->toJSONString();
> +    CString jsonData = json.utf8();

Nit: do you need these local variables?

> Source/WebCore/Modules/encryptedmedia/MediaKeySession.cpp:224
> +            m_taskQueue.enqueueTask([this, weakThis, promise = WTFMove(promise), message = WTFMove(message), messageType, sessionId, succeeded] () mutable {

Looks like you don't use weakThis.

>> Source/WebCore/Modules/encryptedmedia/MediaKeys.cpp:93
>> +    auto certificate = SharedBuffer::create(serverCertificate.data(), serverCertificate.length());
> 
> Specify the type, please.
> 
> Yes, I hate autos :)

I disagree in this case, "SharedBuffer::create" makes the type obvious.

[Jer Noble]

Comment on attachment 298448 [details]
Patch

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

>> Source/WebCore/Modules/encryptedmedia/InitDataRegistry.cpp:91
>> +    CString jsonData = json.utf8();
> 
> Nit: do you need these local variables?

I can probably drop the "json" variable.

>> Source/WebCore/Modules/encryptedmedia/MediaKeySession.cpp:224
>> +            m_taskQueue.enqueueTask([this, weakThis, promise = WTFMove(promise), message = WTFMove(message), messageType, sessionId, succeeded] () mutable {
> 
> Looks like you don't use weakThis.

Oh you know, since it's a task, it'll never be run after the this object is destroyed; so yeah, I don't need weakThis. Good catch!

[Jer Noble]

Created attachment 298481 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 298481 [details]
Patch for landing

Rejecting attachment 298481 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 298481, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/2865310

[Jer Noble]

Created attachment 298493 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 298493 [details]
Patch for landing

Clearing flags on attachment: 298493

Committed r210555: <http://trac.webkit.org/changeset/210555>

[Xabier Rodríguez Calvar]

Closing the bug as patch has landed.

(In reply to comment #3)
> >> Source/WebCore/Modules/encryptedmedia/InitDataRegistry.cpp:85
> >> +    auto kidsArray = InspectorArray::create();
> > 
> > I usually don't like the auto for plain variables, I would type the object explicitly.
> 
> I've received precisely the opposite review feedback from Darin.
> 
> >> Source/WebCore/Modules/encryptedmedia/MediaKeys.cpp:93
> >> +    auto certificate = SharedBuffer::create(serverCertificate.data(), serverCertificate.length());
> > 
> > Specify the type, please.
> > 
> > Yes, I hate autos :)
> 
> I see. :)
> 
> But auto is the future, and in situations where the type is immediately
> inferable (like in SharedBuffer::create()), appropriate..

(In reply to comment #4)
> >> Source/WebCore/Modules/encryptedmedia/InitDataRegistry.cpp:85
> >> +    auto kidsArray = InspectorArray::create();
> > 
> > I usually don't like the auto for plain variables, I would type the object explicitly.
> 
> While I am not in the "always use auto" camp, I think it makes sense in
> cases like this where the type is clear from the context.

Well, I guess it is a matter of taste/readability thing and that's why I opened the discussion, cause I think we need consensus or guidelines on how to write it.