Add support for MediaKeySystemAccess.createMediaKeys()
Created attachment 298158 [details] Patch
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.
Created attachment 298170 [details] Patch for landing
Comment on attachment 298170 [details] Patch for landing Clearing flags on attachment: 298170 Committed r210445: <http://trac.webkit.org/changeset/210445>
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.
(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.