Add support for MediaKeys.setServerCertificate()
Created attachment 298221 [details] Patch
Comment on attachment 298221 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298221&action=review > Source/WebCore/Modules/encryptedmedia/CDM.cpp:629 > + return m_private ? m_private->supportsServerCertificates() : false; I prefer "a && b" over "a ? b : false". > Source/WebCore/Modules/encryptedmedia/CDM.h:74 > + bool supportsServerCertificates(); const? > Source/WebCore/Modules/encryptedmedia/CDMPrivate.h:54 > + virtual bool supportsServerCertificates() = 0; const? > Source/WebCore/Modules/encryptedmedia/MediaKeys.cpp:79 > + Ref<ArrayBuffer> certificate = ArrayBuffer::create(serverCertificate.data(), serverCertificate.length()); please use auto here > Source/WebCore/Modules/encryptedmedia/MediaKeys.cpp:93 > + promise->resolve<IDLBoolean>(true); Is this really the right way to resolve a promise with a boolean true? It seems strange that "IDL" is mentioned here. > Source/WebCore/testing/MockCDMFactory.cpp:138 > + return m_factory ? m_factory->supportsServerCertificates() : false; Same comment about "&&" as above. > Source/WebCore/testing/MockCDMFactory.cpp:184 > + static ASCIILiteral validCertificate { "valid" }; An ASCIILiteral is just a pointer. There is no good reason to ever have a global of this type, especially a non-const global. > Source/WebCore/testing/MockCDMFactory.cpp:185 > + String certificateString(static_cast<const char*>(certificate.data()), certificate.byteLength()); Doesn’t seem smart to convert an array buffer into a string, copying its content, just to compare it with 5 bytes. I think we should write an overload of equalLettersIgnoringASCIICase that takes a pointer and a length, if we don’t already have one, and use that. > Source/WebCore/testing/MockCDMFactory.cpp:187 > + if (equalIgnoringASCIICase(certificateString, String(validCertificate))) Even without the issue above, this should be: equalLettersIgnoringASCIICase(certificateString, "valid") Significantly more efficient than equalIgnoringASCIICase when we are comparing with a string that has only appropriate characters in it (such as all letters). No need to create and destroy a String, for example, and we ever have a tighter comparison loop for this case.
Comment on attachment 298221 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298221&action=review >> Source/WebCore/testing/MockCDMFactory.cpp:185 >> + String certificateString(static_cast<const char*>(certificate.data()), certificate.byteLength()); > > Doesn’t seem smart to convert an array buffer into a string, copying its content, just to compare it with 5 bytes. I think we should write an overload of equalLettersIgnoringASCIICase that takes a pointer and a length, if we don’t already have one, and use that. Oh, another idea that would be almost as efficient would be to just use StringView.
Comment on attachment 298221 [details] Patch Attachment 298221 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2844957 New failing tests: media/encrypted-media/mock-MediaKeys-setServerCertificate.html
Created attachment 298225 [details] Archive of layout-test-results from ews100 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 298221 [details] Patch Attachment 298221 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2844965 New failing tests: media/encrypted-media/mock-MediaKeys-setServerCertificate.html
Created attachment 298226 [details] Archive of layout-test-results from ews107 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 298221 [details] Patch Attachment 298221 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2844975 New failing tests: media/encrypted-media/mock-MediaKeys-setServerCertificate.html
Created attachment 298227 [details] Archive of layout-test-results from ews115 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 298221 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298221&action=review >> Source/WebCore/Modules/encryptedmedia/CDM.cpp:629 >> + return m_private ? m_private->supportsServerCertificates() : false; > > I prefer "a && b" over "a ? b : false". Ok. >> Source/WebCore/Modules/encryptedmedia/CDM.h:74 >> + bool supportsServerCertificates(); > > const? Yes! >> Source/WebCore/Modules/encryptedmedia/CDMPrivate.h:54 >> + virtual bool supportsServerCertificates() = 0; > > const? Yes! >> Source/WebCore/Modules/encryptedmedia/MediaKeys.cpp:79 >> + Ref<ArrayBuffer> certificate = ArrayBuffer::create(serverCertificate.data(), serverCertificate.length()); > > please use auto here Ok. >> Source/WebCore/Modules/encryptedmedia/MediaKeys.cpp:93 >> + promise->resolve<IDLBoolean>(true); > > Is this really the right way to resolve a promise with a boolean true? It seems strange that "IDL" is mentioned here. I've found that the Promise template machinery can't infer the type based on just a boolean. This may just be a pattern that's never come up before, since it seems really weird to push a boolean true into a promise, but ¯\_(ツ)_/¯ >> Source/WebCore/testing/MockCDMFactory.cpp:138 >> + return m_factory ? m_factory->supportsServerCertificates() : false; > > Same comment about "&&" as above. Ok. >> Source/WebCore/testing/MockCDMFactory.cpp:184 >> + static ASCIILiteral validCertificate { "valid" }; > > An ASCIILiteral is just a pointer. There is no good reason to ever have a global of this type, especially a non-const global. Ok. >>> Source/WebCore/testing/MockCDMFactory.cpp:185 >>> + String certificateString(static_cast<const char*>(certificate.data()), certificate.byteLength()); >> >> Doesn’t seem smart to convert an array buffer into a string, copying its content, just to compare it with 5 bytes. I think we should write an overload of equalLettersIgnoringASCIICase that takes a pointer and a length, if we don’t already have one, and use that. > > Oh, another idea that would be almost as efficient would be to just use StringView. StringView! There's a great idea. >> Source/WebCore/testing/MockCDMFactory.cpp:187 >> + if (equalIgnoringASCIICase(certificateString, String(validCertificate))) > > Even without the issue above, this should be: > > equalLettersIgnoringASCIICase(certificateString, "valid") > > Significantly more efficient than equalIgnoringASCIICase when we are comparing with a string that has only appropriate characters in it (such as all letters). No need to create and destroy a String, for example, and we ever have a tighter comparison loop for this case. Will do.
Created attachment 298231 [details] Patch for landing
Comment on attachment 298231 [details] Patch for landing Clearing flags on attachment: 298231 Committed r210549: <http://trac.webkit.org/changeset/210549>