Bug 166749 – Add support for MediaKeySystemAccess.createMediaKeys()

• Component: Media
• Status: NEW

[Jer Noble]

Add support for MediaKeySystemAccess.createMediaKeys()

[Jer Noble]

Created attachment 298158 [details]
Patch

[Eric Carlson]

Comment on attachment 298158 [details]
Patch

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

> Source/WebCore/Modules/encryptedmedia/CDMInstance.h:35
> +struct MediaKeysRestrictions;

Nit: "MediaKeysRestrictions" is unnecessary.

> Source/WebCore/Modules/encryptedmedia/CDMPrivate.h:40
> +

Nit: extra blank line.

[Jer Noble]

Created attachment 298170 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 298170 [details]
Patch for landing

Clearing flags on attachment: 298170

Committed r210445: <http://trac.webkit.org/changeset/210445>

[Darin Adler]

Comment on attachment 298170 [details]
Patch for landing

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

Some post-landing comments.

> Source/WebCore/Modules/encryptedmedia/CDMInstance.h:43
> +    enum SuccessValue {
> +        Failed,
> +        Succeeded,
> +    };

One liner maybe? I don’t see a reason to spread this over four lines.

I am not sure this idiom is great. Is there some less wordy way we could do this? I might just prefer a success boolean rather than a special type, even though it would require a comment defining the meaning of the boolean.

> Source/WebCore/Modules/encryptedmedia/MediaKeySystemAccess.cpp:77
> +        std::unique_ptr<CDMInstance> instance = m_implementation->createInstance();

auto would be good here

> Source/WebCore/testing/MockCDMFactory.cpp:127
> +    return std::unique_ptr<CDMInstance>(new MockCDMInstance(m_weakPtrFactory.createWeakPtr()));

Can we find a way to use make_unique instead of new here? I know there are possibly privacy reason to not do that.

> Source/WebCore/testing/MockCDMFactory.cpp:167
> +    MockCDMFactory* factory = m_cdm ? m_cdm->factory() : nullptr;

auto*

> Source/WebCore/testing/MockCDMFactory.h:78
>      MockCDM(WeakPtr<MockCDMFactory>);

Should take a const WeakPtr<MockCDMFactory>& for better efficiency. Should probably be marked explicit, too.

> Source/WebCore/testing/MockCDMFactory.h:102
> +    MockCDMInstance(WeakPtr<MockCDM>);

Should take a const WeakPtr<MockCDM>& for better efficiency. Should probably be marked explicit, too.

[Jer Noble]

(In reply to comment #5)
> Comment on attachment 298170 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=298170&action=review
> 
> Some post-landing comments.
> 
> > Source/WebCore/Modules/encryptedmedia/CDMInstance.h:43
> > +    enum SuccessValue {
> > +        Failed,
> > +        Succeeded,
> > +    };
> 
> One liner maybe? I don’t see a reason to spread this over four lines.
> 
> I am not sure this idiom is great. Is there some less wordy way we could do
> this? I might just prefer a success boolean rather than a special type, even
> though it would require a comment defining the meaning of the boolean.

Eh, the usefulness of this idiom is that it makes sense in the calling context. The boolean return value idiom would need a comment everywhere it's called to be as clear.

I think this is better than a boolean return, but maybe not as good as it good be.

> > Source/WebCore/Modules/encryptedmedia/MediaKeySystemAccess.cpp:77
> > +        std::unique_ptr<CDMInstance> instance = m_implementation->createInstance();
> 
> auto would be good here

Ok.

> > Source/WebCore/testing/MockCDMFactory.cpp:127
> > +    return std::unique_ptr<CDMInstance>(new MockCDMInstance(m_weakPtrFactory.createWeakPtr()));
> 
> Can we find a way to use make_unique instead of new here? I know there are
> possibly privacy reason to not do that.

This issue is that the compiler can't find a good way to cast a unique_ptr<MockCDMInstance> to a unique_ptr<CDMInstance>.

> > Source/WebCore/testing/MockCDMFactory.cpp:167
> > +    MockCDMFactory* factory = m_cdm ? m_cdm->factory() : nullptr;
> 
> auto*

Ok.

> > Source/WebCore/testing/MockCDMFactory.h:78
> >      MockCDM(WeakPtr<MockCDMFactory>);
> 
> Should take a const WeakPtr<MockCDMFactory>& for better efficiency. Should
> probably be marked explicit, too.

Not a WeakPtr<MockCDMFactory>&&?

> > Source/WebCore/testing/MockCDMFactory.h:102
> > +    MockCDMInstance(WeakPtr<MockCDM>);
> 
> Should take a const WeakPtr<MockCDM>& for better efficiency. Should probably
> be marked explicit, too.