Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16583004:
Add Stubbed implementation of WebCDM* interfaces for unprefixed EME APIs. (Closed)
|
Created:
7 years, 6 months ago by ddorwin Modified:
7 years, 6 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd Stubbed implementation of WebContentDecryptionModule* interfaces for unprefixed EME APIs.
BUG=224791, 250048
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=206603
Patch Set 1 #
Total comments: 28
Patch Set 2 : Renamed files and classes only #Patch Set 3 : review feedback #
Total comments: 12
Patch Set 4 : Dropped "Renderer" #
Total comments: 11
Patch Set 5 : addressed feedback #Patch Set 6 : Use uint8 and size_t #Patch Set 7 : Change in platform API #
Total comments: 2
Patch Set 8 : feedback #Patch Set 9 : rebase #Patch Set 10 : Fix gcc build. #
Messages
Total messages: 21 (0 generated)
The corresponding Blink CL is https://codereview.chromium.org/16387015/ I need to land the public/platform files from that CL before this one.
https://codereview.chromium.org/16583004/diff/1/content/renderer/media/render... File content/renderer/media/renderer_webcdm_impl.cc (right): https://codereview.chromium.org/16583004/diff/1/content/renderer/media/render... content/renderer/media/renderer_webcdm_impl.cc:18: // WebKit::WebCDMSession nit: ... implementation. https://codereview.chromium.org/16583004/diff/1/content/renderer/media/render... content/renderer/media/renderer_webcdm_impl.cc:20: virtual void generateKeyRequest(const WebKit::WebString& mimeType, unix_hacker var names https://codereview.chromium.org/16583004/diff/1/content/renderer/media/render... content/renderer/media/renderer_webcdm_impl.cc:23: virtual void update(const unsigned char* key, unsigned keyLength); unix_hacker var names https://codereview.chromium.org/16583004/diff/1/content/renderer/media/render... content/renderer/media/renderer_webcdm_impl.cc:35: : client_(client) { does session_id_ need to be init'd to anything in particular? https://codereview.chromium.org/16583004/diff/1/content/renderer/media/render... content/renderer/media/renderer_webcdm_impl.cc:42: const WebKit::WebString& mimeType, unix_hacker var names https://codereview.chromium.org/16583004/diff/1/content/renderer/media/render... content/renderer/media/renderer_webcdm_impl.cc:44: remove line https://codereview.chromium.org/16583004/diff/1/content/renderer/media/render... content/renderer/media/renderer_webcdm_impl.cc:48: remove line https://codereview.chromium.org/16583004/diff/1/content/renderer/media/render... content/renderer/media/renderer_webcdm_impl.cc:52: unsigned keyLength) { unix_hacker var names https://codereview.chromium.org/16583004/diff/1/content/renderer/media/render... content/renderer/media/renderer_webcdm_impl.cc:75: return new RendererWebCDMImpl(keySystem); unix_hacker var names https://codereview.chromium.org/16583004/diff/1/content/renderer/media/render... content/renderer/media/renderer_webcdm_impl.cc:78: RendererWebCDMImpl::RendererWebCDMImpl(const string16& keySystem) { unix_hacker var names https://codereview.chromium.org/16583004/diff/1/content/renderer/media/render... File content/renderer/media/renderer_webcdm_impl.h (right): https://codereview.chromium.org/16583004/diff/1/content/renderer/media/render... content/renderer/media/renderer_webcdm_impl.h:13: class MIDIMessageFilter; lol https://codereview.chromium.org/16583004/diff/1/content/renderer/media/render... content/renderer/media/renderer_webcdm_impl.h:17: static RendererWebCDMImpl* create(const string16& keySystem); unix_hacker var names https://codereview.chromium.org/16583004/diff/1/content/renderer/media/render... content/renderer/media/renderer_webcdm_impl.h:21: // WebKit::WebCDM nit: "WebKit::WebCDM implementation." https://codereview.chromium.org/16583004/diff/1/content/renderer/media/render... content/renderer/media/renderer_webcdm_impl.h:25: RendererWebCDMImpl(const string16& keySystem); indent
PTAL. I renamed the classes per the request in https://codereview.chromium.org/16387015/. https://codereview.chromium.org/16583004/diff/1/content/renderer/media/render... File content/renderer/media/renderer_webcdm_impl.cc (right): https://codereview.chromium.org/16583004/diff/1/content/renderer/media/render... content/renderer/media/renderer_webcdm_impl.cc:18: // WebKit::WebCDMSession On 2013/06/07 18:07:47, scherkus wrote: > nit: ... implementation. Done. https://codereview.chromium.org/16583004/diff/1/content/renderer/media/render... content/renderer/media/renderer_webcdm_impl.cc:20: virtual void generateKeyRequest(const WebKit::WebString& mimeType, On 2013/06/07 18:07:47, scherkus wrote: > unix_hacker var names Done. https://codereview.chromium.org/16583004/diff/1/content/renderer/media/render... content/renderer/media/renderer_webcdm_impl.cc:23: virtual void update(const unsigned char* key, unsigned keyLength); On 2013/06/07 18:07:47, scherkus wrote: > unix_hacker var names Done. https://codereview.chromium.org/16583004/diff/1/content/renderer/media/render... content/renderer/media/renderer_webcdm_impl.cc:35: : client_(client) { On 2013/06/07 18:07:47, scherkus wrote: > does session_id_ need to be init'd to anything in particular? It will. I added a TODO to make this obvious. https://codereview.chromium.org/16583004/diff/1/content/renderer/media/render... content/renderer/media/renderer_webcdm_impl.cc:42: const WebKit::WebString& mimeType, On 2013/06/07 18:07:47, scherkus wrote: > unix_hacker var names Done. https://codereview.chromium.org/16583004/diff/1/content/renderer/media/render... content/renderer/media/renderer_webcdm_impl.cc:44: On 2013/06/07 18:07:47, scherkus wrote: > remove line Done. https://codereview.chromium.org/16583004/diff/1/content/renderer/media/render... content/renderer/media/renderer_webcdm_impl.cc:48: On 2013/06/07 18:07:47, scherkus wrote: > remove line Done. https://codereview.chromium.org/16583004/diff/1/content/renderer/media/render... content/renderer/media/renderer_webcdm_impl.cc:52: unsigned keyLength) { On 2013/06/07 18:07:47, scherkus wrote: > unix_hacker var names Done. https://codereview.chromium.org/16583004/diff/1/content/renderer/media/render... content/renderer/media/renderer_webcdm_impl.cc:75: return new RendererWebCDMImpl(keySystem); On 2013/06/07 18:07:47, scherkus wrote: > unix_hacker var names Done. https://codereview.chromium.org/16583004/diff/1/content/renderer/media/render... content/renderer/media/renderer_webcdm_impl.cc:78: RendererWebCDMImpl::RendererWebCDMImpl(const string16& keySystem) { On 2013/06/07 18:07:47, scherkus wrote: > unix_hacker var names Done. https://codereview.chromium.org/16583004/diff/1/content/renderer/media/render... File content/renderer/media/renderer_webcdm_impl.h (right): https://codereview.chromium.org/16583004/diff/1/content/renderer/media/render... content/renderer/media/renderer_webcdm_impl.h:13: class MIDIMessageFilter; On 2013/06/07 18:07:47, scherkus wrote: > lol Done. https://codereview.chromium.org/16583004/diff/1/content/renderer/media/render... content/renderer/media/renderer_webcdm_impl.h:17: static RendererWebCDMImpl* create(const string16& keySystem); On 2013/06/07 18:07:47, scherkus wrote: > unix_hacker var names Done. copypasta. https://codereview.chromium.org/16583004/diff/1/content/renderer/media/render... content/renderer/media/renderer_webcdm_impl.h:21: // WebKit::WebCDM On 2013/06/07 18:07:47, scherkus wrote: > nit: "WebKit::WebCDM implementation." Done. https://codereview.chromium.org/16583004/diff/1/content/renderer/media/render... content/renderer/media/renderer_webcdm_impl.h:25: RendererWebCDMImpl(const string16& keySystem); On 2013/06/07 18:07:47, scherkus wrote: > indent Done.
https://codereview.chromium.org/16583004/diff/15001/content/renderer/media/re... File content/renderer/media/renderer_webcontentdecryptionmodule_impl.h (right): https://codereview.chromium.org/16583004/diff/15001/content/renderer/media/re... content/renderer/media/renderer_webcontentdecryptionmodule_impl.h:13: class RendererWebContentDecryptionModuleImpl if we have the "Renderer" prefix do we really need the "Impl" suffix?
jamesr, please do an OWNERS review of content/renderer. Also, can you answer scherkus' comment about having both renderer_ and _impl? I just followed what appears to be the existing pattern.
having a prefix like 'renderer' is useful to disambiguate multiple different implementations of an interface. for instance, in addition to RendererWebKitPlatformSupportImpl we also have a BrowserWebKitPlatformSupportImpl, WorkerWebKitPlatformSupportImpl, PpapiWebKitPlatformSupportImpl, etc. In your case it doesn't appear you have this need to disambiguate so the extra verbiage does not appear useful What's the intended staging of this patch? It doesn't seem to do anything by itself. https://codereview.chromium.org/16583004/diff/15001/content/renderer/media/re... File content/renderer/media/renderer_webcontentdecryptionmodule_impl.cc (right): https://codereview.chromium.org/16583004/diff/15001/content/renderer/media/re... content/renderer/media/renderer_webcontentdecryptionmodule_impl.cc:13: class RendererWebContentDecryptionModuleSessionImpl ditto on the redundant class names https://codereview.chromium.org/16583004/diff/15001/content/renderer/media/re... content/renderer/media/renderer_webcontentdecryptionmodule_impl.cc:29: WebKit::WebContentDecryptionModuleSession::Client* const client_; the constness here is a bit unusual. is there a type system need for this or does it just happen to be true in the current implementation? https://codereview.chromium.org/16583004/diff/15001/content/renderer/media/re... content/renderer/media/renderer_webcontentdecryptionmodule_impl.cc:62: if (key_length != 128 / 8) { 128 / 8? how are these magical numbers derived? https://codereview.chromium.org/16583004/diff/15001/content/renderer/media/re... content/renderer/media/renderer_webcontentdecryptionmodule_impl.cc:90: ) { very odd formatting https://codereview.chromium.org/16583004/diff/15001/content/renderer/media/re... File content/renderer/media/renderer_webcontentdecryptionmodule_impl.h (right): https://codereview.chromium.org/16583004/diff/15001/content/renderer/media/re... content/renderer/media/renderer_webcontentdecryptionmodule_impl.h:13: class RendererWebContentDecryptionModuleImpl why "Renderer"? is there any possible implementation of the WebContentDecryptionModule interface? if not, then just WebContentDecyptionModuleImpl sounds better https://codereview.chromium.org/16583004/diff/15001/content/renderer/renderer... File content/renderer/renderer_webkitplatformsupport_impl.h (right): https://codereview.chromium.org/16583004/diff/15001/content/renderer/renderer... content/renderer/renderer_webkitplatformsupport_impl.h:107: virtual WebKit::WebContentDecryptionModule*createContentDecryptionModule( missing a space
I removed "Renderer" and addressed the other comments except the magical numbers. On 2013/06/12 20:40:49, jamesr wrote: > What's the intended staging of this patch? It doesn't seem to do anything by > itself. Sorry, the context was in the original message and Blink CL. This CL provides a Chromium implementation for the Blink CL. After this lands, I can land the Blink CL and the layout tests will continue to pass with the new Blink implementation. I'll then hook this CL up to src/media in another CL that I am currently working on. The corresponding Blink CL is https://codereview.chromium.org/16387015/ I need to land the public/platform files from that CL before this one. https://codereview.chromium.org/16583004/diff/15001/content/renderer/media/re... File content/renderer/media/renderer_webcontentdecryptionmodule_impl.cc (right): https://codereview.chromium.org/16583004/diff/15001/content/renderer/media/re... content/renderer/media/renderer_webcontentdecryptionmodule_impl.cc:29: WebKit::WebContentDecryptionModuleSession::Client* const client_; On 2013/06/12 20:40:49, jamesr wrote: > the constness here is a bit unusual. is there a type system need for this or > does it just happen to be true in the current implementation? Done. https://codereview.chromium.org/16583004/diff/15001/content/renderer/media/re... content/renderer/media/renderer_webcontentdecryptionmodule_impl.cc:62: if (key_length != 128 / 8) { On 2013/06/12 20:40:49, jamesr wrote: > 128 / 8? how are these magical numbers derived? These are temporary magical numbers (per the TODO above). Keys are always 128-bits in the containers we support, and there are similar checks further down the stack. key_length is in 8-bit bytes. This CL is meant to hook up Chrome to Blink so I can then hook up the lower levels of Chrome. There is a layout tests for successful and failing update() calls. It uses the key length to cause the failure. https://codereview.chromium.org/16583004/diff/15001/content/renderer/media/re... content/renderer/media/renderer_webcontentdecryptionmodule_impl.cc:90: ) { On 2013/06/12 20:40:49, jamesr wrote: > very odd formatting Done. (It didn't fit before renaming and seemed more natural than breaking at "::".) https://codereview.chromium.org/16583004/diff/15001/content/renderer/media/re... File content/renderer/media/renderer_webcontentdecryptionmodule_impl.h (right): https://codereview.chromium.org/16583004/diff/15001/content/renderer/media/re... content/renderer/media/renderer_webcontentdecryptionmodule_impl.h:13: class RendererWebContentDecryptionModuleImpl On 2013/06/12 20:40:49, jamesr wrote: > why "Renderer"? is there any possible implementation of the > WebContentDecryptionModule interface? > > if not, then just WebContentDecyptionModuleImpl sounds better Done. https://codereview.chromium.org/16583004/diff/15001/content/renderer/renderer... File content/renderer/renderer_webkitplatformsupport_impl.h (right): https://codereview.chromium.org/16583004/diff/15001/content/renderer/renderer... content/renderer/renderer_webkitplatformsupport_impl.h:107: virtual WebKit::WebContentDecryptionModule*createContentDecryptionModule( On 2013/06/12 20:40:49, jamesr wrote: > missing a space Done.
https://codereview.chromium.org/16583004/diff/24001/content/renderer/media/we... File content/renderer/media/webcontentdecryptionmodule_impl.cc (right): https://codereview.chromium.org/16583004/diff/24001/content/renderer/media/we... content/renderer/media/webcontentdecryptionmodule_impl.cc:16: WebContentDecryptionModuleSessionImpl( one-argument constructors need explicit https://codereview.chromium.org/16583004/diff/24001/content/renderer/media/we... content/renderer/media/webcontentdecryptionmodule_impl.cc:23: const unsigned char* init_data, assuming that you are using this type as a pointer to a bag of bytes, use uint8 from base/basictypes.h https://codereview.chromium.org/16583004/diff/24001/content/renderer/media/we... content/renderer/media/webcontentdecryptionmodule_impl.cc:24: unsigned init_data_length); if this is supposed to be the length of a buffer of bytes, why isn't it size_t ? https://codereview.chromium.org/16583004/diff/24001/content/renderer/media/we... content/renderer/media/webcontentdecryptionmodule_impl.cc:25: virtual void update(const unsigned char* key, unsigned key_length); uint8*, size_t https://codereview.chromium.org/16583004/diff/24001/content/renderer/media/we... content/renderer/media/webcontentdecryptionmodule_impl.cc:61: if (key_length != 128 / 8) { surely this isn't the only place in the code that has to care about key lengths. is this not a named constant somewhere? if it isn't, could it be? https://codereview.chromium.org/16583004/diff/24001/content/renderer/media/we... File content/renderer/media/webcontentdecryptionmodule_impl.h (right): https://codereview.chromium.org/16583004/diff/24001/content/renderer/media/we... content/renderer/media/webcontentdecryptionmodule_impl.h:16: static WebContentDecryptionModuleImpl* create(const string16& key_system); function names start with an uppercase letter in Chromium style, so this would be Create
Thanks. The types are related to the Blink platform APIs. Let me know if they need to be changed. https://codereview.chromium.org/16583004/diff/24001/content/renderer/media/we... File content/renderer/media/webcontentdecryptionmodule_impl.cc (right): https://codereview.chromium.org/16583004/diff/24001/content/renderer/media/we... content/renderer/media/webcontentdecryptionmodule_impl.cc:16: WebContentDecryptionModuleSessionImpl( On 2013/06/12 21:18:49, jamesr wrote: > one-argument constructors need explicit Done. https://codereview.chromium.org/16583004/diff/24001/content/renderer/media/we... content/renderer/media/webcontentdecryptionmodule_impl.cc:23: const unsigned char* init_data, On 2013/06/12 21:18:49, jamesr wrote: > assuming that you are using this type as a pointer to a bag of bytes, use uint8 > from base/basictypes.h This the implementation of a Blink platform API. We seem to use unsigned char for all these. We can't rely on src/base and we try to avoid uint8_t. https://codereview.chromium.org/16583004/diff/24001/content/renderer/media/we... content/renderer/media/webcontentdecryptionmodule_impl.cc:24: unsigned init_data_length); On 2013/06/12 21:18:49, jamesr wrote: > if this is supposed to be the length of a buffer of bytes, why isn't it size_t ? As above, just following existing examples in Blink. The length is always small, and unsigned is used through the Blink stack. Should that be changed? https://codereview.chromium.org/16583004/diff/24001/content/renderer/media/we... content/renderer/media/webcontentdecryptionmodule_impl.cc:25: virtual void update(const unsigned char* key, unsigned key_length); On 2013/06/12 21:18:49, jamesr wrote: > uint8*, size_t Same. https://codereview.chromium.org/16583004/diff/24001/content/renderer/media/we... content/renderer/media/webcontentdecryptionmodule_impl.cc:61: if (key_length != 128 / 8) { On 2013/06/12 21:18:49, jamesr wrote: > surely this isn't the only place in the code that has to care about key lengths. > is this not a named constant somewhere? if it isn't, could it be? Done.
On 2013/06/12 21:37:22, ddorwin wrote: > Thanks. The types are related to the Blink platform APIs. Let me know if they > need to be changed. They do > https://codereview.chromium.org/16583004/diff/24001/content/renderer/media/we... > content/renderer/media/webcontentdecryptionmodule_impl.cc:23: const unsigned > char* init_data, > On 2013/06/12 21:18:49, jamesr wrote: > > assuming that you are using this type as a pointer to a bag of bytes, use > uint8 > > from base/basictypes.h > > This the implementation of a Blink platform API. We seem to use unsigned char > for all these. We can't rely on src/base and we try to avoid uint8_t. You aren't in the blink platform API here, you're in chromium code. Use the base/basictypes.h types. They're typedefs for humans - the compiler will still see that the underlying type matches. > > https://codereview.chromium.org/16583004/diff/24001/content/renderer/media/we... > content/renderer/media/webcontentdecryptionmodule_impl.cc:24: unsigned > init_data_length); > On 2013/06/12 21:18:49, jamesr wrote: > > if this is supposed to be the length of a buffer of bytes, why isn't it size_t > ? > > As above, just following existing examples in Blink. The length is always small, > and unsigned is used through the Blink stack. Should that be changed? Yes. Lengths of byte buffers should be represented by size_t.
Thanks - good to know. I also changed https://codereview.chromium.org/16387015/.
The platform APIs are now in https://codereview.chromium.org/16841007/, which is in the CQ. This can land after that lands and DEPS roll.
lgtm https://codereview.chromium.org/16583004/diff/43001/content/renderer/media/we... File content/renderer/media/webcontentdecryptionmodule_impl.cc (right): https://codereview.chromium.org/16583004/diff/43001/content/renderer/media/we... content/renderer/media/webcontentdecryptionmodule_impl.cc:34: DISALLOW_IMPLICIT_CONSTRUCTORS(WebContentDecryptionModuleSessionImpl); this class has a constructor (it's public, even), so just DISALLOW_ASSIGN_AND_COPY should do everything you need here
creis, OWNERS approval for content/content_renderer.gypi please. https://codereview.chromium.org/16583004/diff/43001/content/renderer/media/we... File content/renderer/media/webcontentdecryptionmodule_impl.cc (right): https://codereview.chromium.org/16583004/diff/43001/content/renderer/media/we... content/renderer/media/webcontentdecryptionmodule_impl.cc:34: DISALLOW_IMPLICIT_CONSTRUCTORS(WebContentDecryptionModuleSessionImpl); On 2013/06/13 02:04:18, jamesr wrote: > this class has a constructor (it's public, even), so just > DISALLOW_ASSIGN_AND_COPY should do everything you need here Done.
On Wed, Jun 12, 2013 at 10:51 PM, <ddorwin@chromium.org> wrote: > creis, OWNERS approval for content/content_renderer.gypi please. http://src.chromium.org/viewvc/chrome/trunk/src/content/OWNERS line 19: 19per-file content_renderer.gypi=jamesr@chromium.org you should be good to go > > > https://codereview.chromium.**org/16583004/diff/43001/** > content/renderer/media/**webcontentdecryptionmodule_**impl.cc<https://codereview.chromium.org/16583004/diff/43001/content/renderer/media/webcontentdecryptionmodule_impl.cc> > File content/renderer/media/**webcontentdecryptionmodule_**impl.cc > (right): > > https://codereview.chromium.**org/16583004/diff/43001/** > content/renderer/media/**webcontentdecryptionmodule_**impl.cc#newcode34<https://codereview.chromium.org/16583004/diff/43001/content/renderer/media/webcontentdecryptionmodule_impl.cc#newcode34> > content/renderer/media/**webcontentdecryptionmodule_**impl.cc:34: > DISALLOW_IMPLICIT_**CONSTRUCTORS(**WebContentDecryptionModuleSess** > ionImpl); > On 2013/06/13 02:04:18, jamesr wrote: > >> this class has a constructor (it's public, even), so just >> DISALLOW_ASSIGN_AND_COPY should do everything you need here >> > > Done. > > https://codereview.chromium.**org/16583004/<https://codereview.chromium.org/1... >
Oh, good. I had looked before rebasing when I didn't have that line yet. Now, I just need to wait for Blink DEPS to roll to 152323.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ddorwin@chromium.org/16583004/52001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ddorwin@chromium.org/16583004/77001
Retried try job too often on win7_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ddorwin@chromium.org/16583004/77001
Message was sent while issue was closed.
Change committed as 206603 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
