Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
555223004:
Update MediaKeys interface for EME (Closed)
DescriptionUpdate MediaKeys interface for EME
To support CDM_6, make the following changes:
- add SetServerCertificate
- add GetUsableKeyIds
- rename ReleaseSession to CloseSession
- add RemoveSession
- add SessionKeysChange event
- add SessionExpirationChange event
This gets the new functionality up to the blink boundary. Changes to
use these new interfaces in blink in a future CL.
For backwards compatibility with existing prefixed EME code, calls
to CancelKeyRequest() call RemoveSession() instead of CloseSession().
BUG=358271, 417481
TEST=existing EME tests still pass + manual testing
Committed: https://crrev.com/80428d2eb359a2448c67ae45ece3ea5dbc2f8cef
Cr-Commit-Position: refs/heads/master@{#296838}
Patch Set 1 #Patch Set 2 : rebase + Android changes #
Total comments: 27
Patch Set 3 : reorder #
Total comments: 53
Patch Set 4 : helpers #
Total comments: 14
Patch Set 5 : CdmPromise subclasses #Patch Set 6 : rebase #
Total comments: 26
Patch Set 7 : BlinkCdmPromiseTemplate #
Total comments: 13
Patch Set 8 : WebCdmPromiseTemplate #
Total comments: 11
Patch Set 9 : CdmResultPromise #
Total comments: 4
Patch Set 10 : rename #
Total comments: 53
Patch Set 11 : base::Time #
Total comments: 12
Patch Set 12 : comments #Messages
Total messages: 30 (3 generated)
jrummell@chromium.org changed reviewers: + ddorwin@chromium.org, xhwang@chromium.org
PTAL.
Initial feedback. I need to look into the result helper more. https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/c... File content/renderer/media/crypto/ppapi_decryptor.cc (right): https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/c... content/renderer/media/crypto/ppapi_decryptor.cc:496: ResumePlayback(); Why is this here instead of in a more general location (WebCDMImpl?)? That's why we pass the value to the CB below. https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/c... File content/renderer/media/crypto/proxy_media_keys.cc (right): https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/c... content/renderer/media/crypto/proxy_media_keys.cc:56: INVALID_ACCESS_ERROR, 0, "SetServerCertificate() is not supported."); There is a NOT_SUPPORTED_ERROR. Note: Eventually, the Android implementation should accept and use this. https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/c... content/renderer/media/crypto/proxy_media_keys.cc:120: promise->reject(INVALID_ACCESS_ERROR, 0, "CloseSession() is not supported."); ditto on NOT_SUPPORTED_ERROR. The comment should be something like "Not yet implemented." https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/c... content/renderer/media/crypto/proxy_media_keys.cc:133: manager_->ReleaseSession(cdm_id_, session_id); Which function should actually be calling this? On Android, it seems more like Close() - I don't think we have any uses of CKR on Android. https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/c... content/renderer/media/crypto/proxy_media_keys.cc:139: INVALID_ACCESS_ERROR, 0, "GetUsableKeyIds() is not supported."); ditto https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/c... File content/renderer/media/crypto/proxy_media_keys.h (right): https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/c... content/renderer/media/crypto/proxy_media_keys.h:31: const media::SessionReadyCB& session_ready_cb, What about adding the CB support? TODO? https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/w... File content/renderer/media/webcontentdecryptionmodule_impl.cc (right): https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:107: uint32 result_index = outstanding_results_.AddResult(result); The rest of this code is unclear to me. https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/w... File content/renderer/media/webcontentdecryptionmodule_impl.h (right): https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.h:78: // Used when setServerCertificate() promise is resolved/rejected. Why isn't this handled as a "Result"? https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.h:79: void CertificateSet(uint32 result_index); On...? ditto below https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.h:80: void SessionError(uint32 result_index, Why does the CDM object handle a session error? https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/w... File content/renderer/media/webcontentdecryptionmodulesession_impl.h (right): https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodulesession_impl.h:75: void SessionCreated(uint32 result_index, const std::string& web_session_id); Hmm, I wonder why these aren't On... too. I know we discussed this, but it sure seems correct here. https://codereview.chromium.org/555223004/diff/20001/media/base/media_keys.h File media/base/media_keys.h (right): https://codereview.chromium.org/555223004/diff/20001/media/base/media_keys.h#... media/base/media_keys.h:132: typedef base::Callback<void(const std::string& web_session_id, nit: For the methods, we have GetUsableKeyIds() at the end. We should probably follow a similar pattern with events/CBs. WDYT? This would apply to many of the earlier files
Updated. https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/c... File content/renderer/media/crypto/ppapi_decryptor.cc (right): https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/c... content/renderer/media/crypto/ppapi_decryptor.cc:496: ResumePlayback(); On 2014/09/10 22:58:38, ddorwin wrote: > Why is this here instead of in a more general location (WebCDMImpl?)? That's why > we pass the value to the CB below. Maybe the name ResumePlayback is wrong? It simply calls the CBs registered when RegisterNewKeyCB() is called. RegisterNewKeyCB() is defined on the Decryptor interface, so the decoders register their callbacks here. Originally they were called in OnSessionReady(), and when I added promises SessionUpdatedPromise and SessionLoadedPromise were added to intercept the success and also call the CBs. If we move this up to WebCDMImpl it will require a lot of work. https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/c... File content/renderer/media/crypto/proxy_media_keys.cc (right): https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/c... content/renderer/media/crypto/proxy_media_keys.cc:56: INVALID_ACCESS_ERROR, 0, "SetServerCertificate() is not supported."); On 2014/09/10 22:58:38, ddorwin wrote: > There is a NOT_SUPPORTED_ERROR. > Note: Eventually, the Android implementation should accept and use this. Done. https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/c... content/renderer/media/crypto/proxy_media_keys.cc:120: promise->reject(INVALID_ACCESS_ERROR, 0, "CloseSession() is not supported."); On 2014/09/10 22:58:38, ddorwin wrote: > ditto on NOT_SUPPORTED_ERROR. The comment should be something like "Not yet > implemented." See comment below. https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/c... content/renderer/media/crypto/proxy_media_keys.cc:133: manager_->ReleaseSession(cdm_id_, session_id); On 2014/09/10 22:58:38, ddorwin wrote: > Which function should actually be calling this? On Android, it seems more like > Close() - I don't think we have any uses of CKR on Android. Since v0.1b CKR calls turn into RemoveSession() currently, I was doing it this way to match existing code. Changed so both methods call ReleaseSession(). Unprefixed EME will let close() work, and remove() will fail (with session not found), which makes sense since LoadSession() is not supported, so no persistent data. Prefixed EME calls (which call RemoveSession()) will work as well. https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/c... content/renderer/media/crypto/proxy_media_keys.cc:139: INVALID_ACCESS_ERROR, 0, "GetUsableKeyIds() is not supported."); On 2014/09/10 22:58:38, ddorwin wrote: > ditto Done. https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/c... File content/renderer/media/crypto/proxy_media_keys.h (right): https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/c... content/renderer/media/crypto/proxy_media_keys.h:31: const media::SessionReadyCB& session_ready_cb, On 2014/09/10 22:58:38, ddorwin wrote: > What about adding the CB support? TODO? Added them along with a TODO. Much more work is required on the Android side. https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/w... File content/renderer/media/webcontentdecryptionmodule_impl.cc (right): https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:107: uint32 result_index = outstanding_results_.AddResult(result); On 2014/09/10 22:58:38, ddorwin wrote: > The rest of this code is unclear to me. The code below simply creates a CdmPromise that is passed through the Chromium layers. When it is resolved/rejected, the corresponding method is called, which will resolve/reject the Result. https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/w... File content/renderer/media/webcontentdecryptionmodule_impl.h (right): https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.h:78: // Used when setServerCertificate() promise is resolved/rejected. On 2014/09/10 22:58:38, ddorwin wrote: > Why isn't this handled as a "Result"? On the Chromium side we switch to CdmPromise, so these are simply the functions that are called when the promise is resolved/rejected. Renaming the methods below. https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.h:79: void CertificateSet(uint32 result_index); On 2014/09/10 22:58:38, ddorwin wrote: > On...? > ditto below Done. https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.h:80: void SessionError(uint32 result_index, On 2014/09/10 22:58:39, ddorwin wrote: > Why does the CDM object handle a session error? This is the promise rejected. Renamed. https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/w... File content/renderer/media/webcontentdecryptionmodulesession_impl.h (right): https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodulesession_impl.h:75: void SessionCreated(uint32 result_index, const std::string& web_session_id); On 2014/09/10 22:58:39, ddorwin wrote: > Hmm, I wonder why these aren't On... too. I know we discussed this, but it sure > seems correct here. I think the discussion had to do with the other callbacks above. Would something like OnPromiseResolvedWithSession, OnPromiseRejected, ... be better? It at least makes it clear it is called when a promise is resolved/rejected? https://codereview.chromium.org/555223004/diff/20001/media/base/media_keys.h File media/base/media_keys.h (right): https://codereview.chromium.org/555223004/diff/20001/media/base/media_keys.h#... media/base/media_keys.h:132: typedef base::Callback<void(const std::string& web_session_id, On 2014/09/10 22:58:39, ddorwin wrote: > nit: For the methods, we have GetUsableKeyIds() at the end. We should probably > follow a similar pattern with events/CBs. WDYT? > > This would apply to many of the earlier files Done.
Reviewed the same subset of files. https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/c... File content/renderer/media/crypto/ppapi_decryptor.cc (right): https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/c... content/renderer/media/crypto/ppapi_decryptor.cc:496: ResumePlayback(); On 2014/09/11 21:21:54, jrummell wrote: > On 2014/09/10 22:58:38, ddorwin wrote: > > Why is this here instead of in a more general location (WebCDMImpl?)? That's > why > > we pass the value to the CB below. > > Maybe the name ResumePlayback is wrong? It simply calls the CBs registered when > RegisterNewKeyCB() is called. RegisterNewKeyCB() is defined on the Decryptor > interface, so the decoders register their callbacks here. Originally they were > called in OnSessionReady(), and when I added promises SessionUpdatedPromise and > SessionLoadedPromise were added to intercept the success and also call the CBs. > If we move this up to WebCDMImpl it will require a lot of work. As discussed offline, the intent is that we can resume at one place (possibly in Blink) rather than in each MediaKeys implementation. (For example, we have similar code in aes_decryptor.) Please file a bug and include it here with a TODO to remove ResumePlayback(). https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/c... File content/renderer/media/crypto/proxy_media_keys.cc (right): https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/c... content/renderer/media/crypto/proxy_media_keys.cc:133: manager_->ReleaseSession(cdm_id_, session_id); On 2014/09/11 21:21:54, jrummell wrote: > On 2014/09/10 22:58:38, ddorwin wrote: > > Which function should actually be calling this? On Android, it seems more like > > Close() - I don't think we have any uses of CKR on Android. > > Since v0.1b CKR calls turn into RemoveSession() currently, I was doing it this > way to match existing code. Changed so both methods call ReleaseSession(). > Unprefixed EME will let close() work, and remove() will fail (with session not > found), which makes sense since LoadSession() is not supported, so no persistent > data. Prefixed EME calls (which call RemoveSession()) will work as well. See comment in new PS. https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/w... File content/renderer/media/webcontentdecryptionmodulesession_impl.h (right): https://codereview.chromium.org/555223004/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodulesession_impl.h:75: void SessionCreated(uint32 result_index, const std::string& web_session_id); On 2014/09/11 21:21:55, jrummell wrote: > On 2014/09/10 22:58:39, ddorwin wrote: > > Hmm, I wonder why these aren't On... too. I know we discussed this, but it > sure > > seems correct here. > > I think the discussion had to do with the other callbacks above. Would something > like OnPromiseResolvedWithSession, OnPromiseRejected, ... be better? It at least > makes it clear it is called when a promise is resolved/rejected? Let's stick with the current pattern for now. The names are not good. We already have an OnSessionError, though. I think that might be misnamed, though - see next file. Maybe we should come back through and clean these all up, but let's be consistent for now. https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/c... File content/renderer/media/crypto/ppapi_decryptor.cc (right): https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/c... content/renderer/media/crypto/ppapi_decryptor.cc:233: base::Bind(&PpapiDecryptor::ResumePlayback, This should not be necessary once OnSessionKeysChange() is being called in all cases. Please add a TODO and refer to the same bug as below. https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/c... content/renderer/media/crypto/ppapi_decryptor.cc:511: ResumePlayback(); I think we should be able to remove this once OnSessionKeysChange() is always being called. Please add a TODO. https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/c... File content/renderer/media/crypto/proxy_media_keys.cc (right): https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/c... content/renderer/media/crypto/proxy_media_keys.cc:141: // Both CloseSession() and RemoveSession() call ReleaseSession(), so calling Remove is not implemented on Android because persistent sessions are not currently supported on Android. I think we should make CloseSession the single working implementation and return NOT_SUPPORTED from RemoveSession. What does ReleaseSession() do? I'm guessing it calls MediaDrm.CKR(), but I don't know what that does either. Since there are no uses of webkitCKR on Android, I think it's fine to fail (you said in the previous PS that it calls RemoveSession()). https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/w... File content/renderer/media/webcontentdecryptionmodule_impl.h (right): https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.h:79: void OnPromiseResolved(uint32 result_index); We should be consistent. If we don't intend to change the others, we should drop "On" here. https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/w... File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right): https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:117: SessionError(result_index, This is the first instance of the one with "On". IS this supposed to be RejectPromise too?
I've reviewed everything except the Helper class, which I hope we can eliminate. https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/w... File content/renderer/media/webcontentdecryptionmodule_impl.h (right): https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.h:79: void OnPromiseResolved(uint32 result_index); On 2014/09/11 23:31:11, ddorwin wrote: > We should be consistent. If we don't intend to change the others, we should drop > "On" here. It appears we *do* want to use "On", which would mean no change here. https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/w... File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right): https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:102: adapter_->RemoveSession(web_session_id_, promise.Pass()); CloseSession() is probably more appropriate, though I don't know what the Blink conversion is. https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:117: SessionError(result_index, On 2014/09/11 23:31:11, ddorwin wrote: > This is the first instance of the one with "On". IS this supposed to be > RejectPromise too? OnPromiseRejected https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:166: uint32 result_index = outstanding_results_.AddResult(result); This is a lot of boilerplate code. It'd be nice to clean up at some point. For example, what if the Result Helper returned a Promise in exchange for the result? https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:167: scoped_ptr<media::SimpleCdmPromise> promise(new media::SimpleCdmPromise( Related to above, can we eliminate outstanding_results_ by storing a reference to/copy of blink::WebContentDecryptionModuleResult in CdmSession? The rest of Chromium wouldn't need to know about this type. Since we have an object on both sides, I'm not sure why we need to switch to an ID. We use IDs when there are no objects on one side (Pepper, media_keys.h). https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:216: adapter_->RemoveSession(web_session_id_, promise.Pass()); ditto https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:267: void WebContentDecryptionModuleSessionImpl::SessionCreated( This will need to be renamed. It's OnSessionInitialized or something. https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:274: status = blink::WebContentDecryptionModuleResult::SessionNotFound; We should know that we called load() and check that here. !web_session_id_.empty() ??? ^ If so, this and line 276 can be collapsed into a single check at 272. https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:289: void WebContentDecryptionModuleSessionImpl::SessionUpdatedOrReleased( This function needs a new name. OnPromiseResolved? https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/w... File content/renderer/media/webcontentdecryptionmodulesession_impl.h (right): https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodulesession_impl.h:38: // TODO(jrummell): Remove the next 3 methods once blink updated. Is there a reason Blink hasn't been updated? Chromium already has the replacements. https://codereview.chromium.org/555223004/diff/40001/content/renderer/pepper/... File content/renderer/pepper/content_decryptor_delegate.cc (right): https://codereview.chromium.org/555223004/diff/40001/content/renderer/pepper/... content/renderer/pepper/content_decryptor_delegate.cc:820: if (session_keys_change_cb_.is_null()) Why would this happen? I see it above as well. https://codereview.chromium.org/555223004/diff/40001/media/cdm/aes_decryptor.cc File media/cdm/aes_decryptor.cc (right): https://codereview.chromium.org/555223004/diff/40001/media/cdm/aes_decryptor.... media/cdm/aes_decryptor.cc:246: promise->resolve(); Reject with not supported. CK key system does not support this. https://dvcs.w3.org/hg/html-media/raw-file/default/encrypted-media/encrypted-... https://codereview.chromium.org/555223004/diff/40001/media/cdm/aes_decryptor.... media/cdm/aes_decryptor.cc:334: session_keys_change_cb_.Run(web_session_id, true); We should at least comment that we are assuming key(s) have been added and thus sending true. (Ideally, we would check, but it's probably not worth the effort.) https://codereview.chromium.org/555223004/diff/40001/media/cdm/aes_decryptor.... media/cdm/aes_decryptor.cc:339: // Validate that this is a reference to an active session and then forget it. Since this either comes from line 360 or unprefixed, it must always be valid. Thus, we can DCHECK like 371. https://codereview.chromium.org/555223004/diff/40001/media/cdm/aes_decryptor.... media/cdm/aes_decryptor.cc:357: // AesDecryptor doesn't keep any persistent data, so no work to do. This should be rejected: Step 2 of https://dvcs.w3.org/hg/html-media/raw-file/default/encrypted-media/encrypted-... Really, we should reject at a higher level and NOTREACHED() here. Maybe file a bug or update the "implement session types" bug and reference it here in a TODO that says to NOTREACHED() here. EXCEPT: We need this for ProxyDecryptor::CancelKeyRequest(). :( So, add "and prefixed is removed" to the TODO. https://codereview.chromium.org/555223004/diff/40001/media/cdm/aes_decryptor.... media/cdm/aes_decryptor.cc:364: promise->resolve(); If it's not found (only prefixed case), we should reject. https://codereview.chromium.org/555223004/diff/40001/media/cdm/aes_decryptor_... File media/cdm/aes_decryptor_unittest.cc (right): https://codereview.chromium.org/555223004/diff/40001/media/cdm/aes_decryptor_... media/cdm/aes_decryptor_unittest.cc:319: // CloseSession(). Note that this should be updated when prefixed is removed along with that bug. https://codereview.chromium.org/555223004/diff/40001/media/cdm/aes_decryptor_... media/cdm/aes_decryptor_unittest.cc:329: PromiseResult result) { expected_result? https://codereview.chromium.org/555223004/diff/40001/media/cdm/aes_decryptor_... media/cdm/aes_decryptor_unittest.cc:680: TEST_F(AesDecryptorTest, RemoveSession) { ditto https://codereview.chromium.org/555223004/diff/40001/media/cdm/ppapi/external... File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right): https://codereview.chromium.org/555223004/diff/40001/media/cdm/ppapi/external... media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:357: decryptor_.RemoveSession(web_session_str, promise.Pass()); We're implementing persistent session support on a decryptor that does not support it. I think we will need to call CloseSession() in the future. Maybe a TODO just so we don't forget this discussion. Before we didn't even call the decryptor_ - I'm not sure how that worked. https://codereview.chromium.org/555223004/diff/40001/media/cdm/ppapi/external... media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:360: host_->OnRejectPromise(promise_id, This should be a DCHECK once session type is moved up.
Updated. https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/c... File content/renderer/media/crypto/ppapi_decryptor.cc (right): https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/c... content/renderer/media/crypto/ppapi_decryptor.cc:233: base::Bind(&PpapiDecryptor::ResumePlayback, On 2014/09/11 23:31:11, ddorwin wrote: > This should not be necessary once OnSessionKeysChange() is being called in all > cases. Please add a TODO and refer to the same bug as below. Done. https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/c... content/renderer/media/crypto/ppapi_decryptor.cc:511: ResumePlayback(); On 2014/09/11 23:31:11, ddorwin wrote: > I think we should be able to remove this once OnSessionKeysChange() is always > being called. Please add a TODO. Done. https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/c... File content/renderer/media/crypto/proxy_media_keys.cc (right): https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/c... content/renderer/media/crypto/proxy_media_keys.cc:141: // Both CloseSession() and RemoveSession() call ReleaseSession(), so calling On 2014/09/11 23:31:11, ddorwin wrote: > Remove is not implemented on Android because persistent sessions are not > currently supported on Android. I think we should make CloseSession the single > working implementation and return NOT_SUPPORTED from RemoveSession. > > What does ReleaseSession() do? I'm guessing it calls MediaDrm.CKR(), but I don't > know what that does either. Since there are no uses of webkitCKR on Android, I > think it's fine to fail (you said in the previous PS that it calls > RemoveSession()). Done. I was more concerned about v0.1b calling CKR, which is converted to call RemoveSession() today. But if nobody uses it, it should be fine to return not implemented. https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/w... File content/renderer/media/webcontentdecryptionmodule_impl.h (right): https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.h:79: void OnPromiseResolved(uint32 result_index); On 2014/09/12 21:46:29, ddorwin wrote: > On 2014/09/11 23:31:11, ddorwin wrote: > > We should be consistent. If we don't intend to change the others, we should > drop > > "On" here. > > It appears we *do* want to use "On", which would mean no change here. Acknowledged. https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/w... File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right): https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:102: adapter_->RemoveSession(web_session_id_, promise.Pass()); On 2014/09/12 21:46:29, ddorwin wrote: > CloseSession() is probably more appropriate, though I don't know what the Blink > conversion is. Blink doesn't call this. So it doesn't matter. Changed to close. https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:117: SessionError(result_index, On 2014/09/11 23:31:11, ddorwin wrote: > This is the first instance of the one with "On". IS this supposed to be > RejectPromise too? Done. https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:117: SessionError(result_index, On 2014/09/12 21:46:29, ddorwin wrote: > On 2014/09/11 23:31:11, ddorwin wrote: > > This is the first instance of the one with "On". IS this supposed to be > > RejectPromise too? > > OnPromiseRejected Done. https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:166: uint32 result_index = outstanding_results_.AddResult(result); On 2014/09/12 21:46:30, ddorwin wrote: > This is a lot of boilerplate code. It'd be nice to clean up at some point. > For example, what if the Result Helper returned a Promise in exchange for the > result? Would work well for most of these calls. However, OnSessionInitialized() would be a problem since we need to register this class with cdm_adapter once we know the session_id. https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:167: scoped_ptr<media::SimpleCdmPromise> promise(new media::SimpleCdmPromise( On 2014/09/12 21:46:29, ddorwin wrote: > Related to above, can we eliminate outstanding_results_ by storing a reference > to/copy of blink::WebContentDecryptionModuleResult in CdmSession? The rest of > Chromium wouldn't need to know about this type. > > Since we have an object on both sides, I'm not sure why we need to switch to an > ID. We use IDs when there are no objects on one side (Pepper, media_keys.h). (I assume you mean CdmPromise.) Have a separate CL out to allow for easier overloading of the CdmPromise classes. Will have to update this after that lands. Moved most of this code into webcontentdecryptionmoduleresult_helper.cc to make this change isolated. https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:216: adapter_->RemoveSession(web_session_id_, promise.Pass()); On 2014/09/12 21:46:30, ddorwin wrote: > ditto This one is called. Changed to close(). https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:267: void WebContentDecryptionModuleSessionImpl::SessionCreated( On 2014/09/12 21:46:29, ddorwin wrote: > This will need to be renamed. It's OnSessionInitialized or something. Done. https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:274: status = blink::WebContentDecryptionModuleResult::SessionNotFound; On 2014/09/12 21:46:30, ddorwin wrote: > We should know that we called load() and check that here. > !web_session_id_.empty() ??? > ^ If so, this and line 276 can be collapsed into a single check at 272. load() hasn't been implemented here yet -- coming soon. https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:289: void WebContentDecryptionModuleSessionImpl::SessionUpdatedOrReleased( On 2014/09/12 21:46:30, ddorwin wrote: > This function needs a new name. OnPromiseResolved? Done. https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/w... File content/renderer/media/webcontentdecryptionmodulesession_impl.h (right): https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodulesession_impl.h:38: // TODO(jrummell): Remove the next 3 methods once blink updated. On 2014/09/12 21:46:30, ddorwin wrote: > Is there a reason Blink hasn't been updated? Chromium already has the > replacements. Not really. Just haven't gotten around to doing the cleanup. I have removed them in https://codereview.chromium.org/568033003/, and then they can be removed from here once that is in. (However, that review depends on this one, so it will have to be done in a 3rd patch). https://codereview.chromium.org/555223004/diff/40001/content/renderer/pepper/... File content/renderer/pepper/content_decryptor_delegate.cc (right): https://codereview.chromium.org/555223004/diff/40001/content/renderer/pepper/... content/renderer/pepper/content_decryptor_delegate.cc:820: if (session_keys_change_cb_.is_null()) On 2014/09/12 21:46:30, ddorwin wrote: > Why would this happen? I see it above as well. It shouldn't happen. I just copied the existing pattern (no DCHECK in the constructor that this is supplied). But I think the goal of pepper code is to "work" with flawed data, so this check makes sure we don't crash. https://codereview.chromium.org/555223004/diff/40001/media/cdm/aes_decryptor.cc File media/cdm/aes_decryptor.cc (right): https://codereview.chromium.org/555223004/diff/40001/media/cdm/aes_decryptor.... media/cdm/aes_decryptor.cc:246: promise->resolve(); On 2014/09/12 21:46:30, ddorwin wrote: > Reject with not supported. CK key system does not support this. > https://dvcs.w3.org/hg/html-media/raw-file/default/encrypted-media/encrypted-... Done. https://codereview.chromium.org/555223004/diff/40001/media/cdm/aes_decryptor.... media/cdm/aes_decryptor.cc:334: session_keys_change_cb_.Run(web_session_id, true); On 2014/09/12 21:46:30, ddorwin wrote: > We should at least comment that we are assuming key(s) have been added and thus > sending true. (Ideally, we would check, but it's probably not worth the effort.) Done. https://codereview.chromium.org/555223004/diff/40001/media/cdm/aes_decryptor.... media/cdm/aes_decryptor.cc:339: // Validate that this is a reference to an active session and then forget it. On 2014/09/12 21:46:30, ddorwin wrote: > Since this either comes from line 360 or unprefixed, it must always be valid. > Thus, we can DCHECK like 371. Done. https://codereview.chromium.org/555223004/diff/40001/media/cdm/aes_decryptor.... media/cdm/aes_decryptor.cc:357: // AesDecryptor doesn't keep any persistent data, so no work to do. On 2014/09/12 21:46:30, ddorwin wrote: > This should be rejected: Step 2 of > https://dvcs.w3.org/hg/html-media/raw-file/default/encrypted-media/encrypted-... > > Really, we should reject at a higher level and NOTREACHED() here. Maybe file a > bug or update the "implement session types" bug and reference it here in a TODO > that says to NOTREACHED() here. > > EXCEPT: We need this for ProxyDecryptor::CancelKeyRequest(). :( > So, add "and prefixed is removed" to the TODO. Done. https://codereview.chromium.org/555223004/diff/40001/media/cdm/aes_decryptor.... media/cdm/aes_decryptor.cc:364: promise->resolve(); On 2014/09/12 21:46:30, ddorwin wrote: > If it's not found (only prefixed case), we should reject. Done. https://codereview.chromium.org/555223004/diff/40001/media/cdm/aes_decryptor_... File media/cdm/aes_decryptor_unittest.cc (right): https://codereview.chromium.org/555223004/diff/40001/media/cdm/aes_decryptor_... media/cdm/aes_decryptor_unittest.cc:319: // CloseSession(). On 2014/09/12 21:46:31, ddorwin wrote: > Note that this should be updated when prefixed is removed along with that bug. Done. https://codereview.chromium.org/555223004/diff/40001/media/cdm/aes_decryptor_... media/cdm/aes_decryptor_unittest.cc:329: PromiseResult result) { On 2014/09/12 21:46:31, ddorwin wrote: > expected_result? Done. https://codereview.chromium.org/555223004/diff/40001/media/cdm/aes_decryptor_... media/cdm/aes_decryptor_unittest.cc:680: TEST_F(AesDecryptorTest, RemoveSession) { On 2014/09/12 21:46:31, ddorwin wrote: > ditto Done. https://codereview.chromium.org/555223004/diff/40001/media/cdm/ppapi/external... File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right): https://codereview.chromium.org/555223004/diff/40001/media/cdm/ppapi/external... media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:357: decryptor_.RemoveSession(web_session_str, promise.Pass()); On 2014/09/12 21:46:31, ddorwin wrote: > We're implementing persistent session support on a decryptor that does not > support it. I think we will need to call CloseSession() in the future. Maybe a > TODO just so we don't forget this discussion. > > Before we didn't even call the decryptor_ - I'm not sure how that worked. The problem with calling close() is that it will reject the promise if the session is already closed. Since aes_decryptor doesn't support persistent session, I have it close the session if it exists, but it always resolves the promise successfully. https://codereview.chromium.org/555223004/diff/40001/media/cdm/ppapi/external... media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:360: host_->OnRejectPromise(promise_id, On 2014/09/12 21:46:31, ddorwin wrote: > This should be a DCHECK once session type is moved up. Done.
Looking good. We need to resolve the promises thing in the other CL. Two non-actionable comments in the previous PS. https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/w... File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right): https://codereview.chromium.org/555223004/diff/40001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:167: scoped_ptr<media::SimpleCdmPromise> promise(new media::SimpleCdmPromise( On 2014/09/15 18:22:40, jrummell wrote: > On 2014/09/12 21:46:29, ddorwin wrote: > > Related to above, can we eliminate outstanding_results_ by storing a reference > > to/copy of blink::WebContentDecryptionModuleResult in CdmSession? The rest of > > Chromium wouldn't need to know about this type. > > > > Since we have an object on both sides, I'm not sure why we need to switch to > an > > ID. We use IDs when there are no objects on one side (Pepper, media_keys.h). > > (I assume you mean CdmPromise.) Have a separate CL out to allow for easier > overloading of the CdmPromise classes. Will have to update this after that > lands. Moved most of this code into webcontentdecryptionmoduleresult_helper.cc > to make this change isolated. Okay, I think we'll need to wait for that before completing this review. This is already a big improvement, though. https://codereview.chromium.org/555223004/diff/40001/content/renderer/pepper/... File content/renderer/pepper/content_decryptor_delegate.cc (right): https://codereview.chromium.org/555223004/diff/40001/content/renderer/pepper/... content/renderer/pepper/content_decryptor_delegate.cc:820: if (session_keys_change_cb_.is_null()) On 2014/09/15 18:22:40, jrummell wrote: > On 2014/09/12 21:46:30, ddorwin wrote: > > Why would this happen? I see it above as well. > > It shouldn't happen. I just copied the existing pattern (no DCHECK in the > constructor that this is supplied). But I think the goal of pepper code is to > "work" with flawed data, so this check makes sure we don't crash. Oh, yes. Good point. We need to handle unexpected calls from a (compromised) plugin process. https://codereview.chromium.org/555223004/diff/60001/content/renderer/media/w... File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right): https://codereview.chromium.org/555223004/diff/60001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:103: adapter_->CloseSession(web_session_id_, promise.Pass()); On 2014/09/15 18:22:39, jrummell wrote: > On 2014/09/12 21:46:29, ddorwin wrote: > > CloseSession() is probably more appropriate, though I don't know what the > Blink > > conversion is. > > Blink doesn't call this. So it doesn't matter. Changed to close. If this is never called, NOTREACHED()? https://codereview.chromium.org/555223004/diff/60001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:131: scoped_ptr<media::NewSessionCdmPromise> promise( For consistency (and less code here), does it make sense to have something like CreateSimpleCdmPromise that takes a callback (OnSessionInitialized)? https://codereview.chromium.org/555223004/diff/60001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:236: uint32 result_index, This code doesn't care about result_index. This is another reason to address the comment at l131. https://codereview.chromium.org/555223004/diff/60001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:242: status = blink::WebContentDecryptionModuleResult::SessionNotFound; On 2014/09/15 18:22:40, jrummell wrote: > On 2014/09/12 21:46:30, ddorwin wrote: > > We should know that we called load() and check that here. > > !web_session_id_.empty() ??? > > ^ If so, this and line 276 can be collapsed into a single check at 272. > > load() hasn't been implemented here yet -- coming soon. But doesn't this code only exist for that? Let's assume it is implemented. :) https://codereview.chromium.org/555223004/diff/60001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:257: void WebContentDecryptionModuleSessionImpl::OnPromiseRejected( It would be nice for this to go away as well (related to commant at l131). https://codereview.chromium.org/555223004/diff/60001/media/cdm/aes_decryptor.cc File media/cdm/aes_decryptor.cc (right): https://codereview.chromium.org/555223004/diff/60001/media/cdm/aes_decryptor.... media/cdm/aes_decryptor.cc:353: // AesDecryptor doesn't keep any persistent data, so this should be FYI, "persistent" is optional for CK. We should replace the hacky ECK implementation with a real CK implementation. File a bug? (No need to reference here.) https://codereview.chromium.org/555223004/diff/60001/media/cdm/ppapi/external... File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right): https://codereview.chromium.org/555223004/diff/60001/media/cdm/ppapi/external... media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:357: decryptor_.RemoveSession(web_session_str, promise.Pass()); On 2014/09/15 18:22:41, jrummell wrote: > On 2014/09/12 21:46:31, ddorwin wrote: > > We're implementing persistent session support on a decryptor that does not > > support it. I think we will need to call CloseSession() in the future. Maybe a > > TODO just so we don't forget this discussion. > > > > Before we didn't even call the decryptor_ - I'm not sure how that worked. > > The problem with calling close() is that it will reject the promise if the > session is already closed. Since aes_decryptor doesn't support persistent > session, I have it close the session if it exists, but it always resolves the > promise successfully. Is this due to the prefixed support? If so, add a comment here about how to fix once prefixed is removed?
PS5 is the changes, PS6 is a rebase since the original is 2 weeks old. https://codereview.chromium.org/555223004/diff/60001/content/renderer/media/w... File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right): https://codereview.chromium.org/555223004/diff/60001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:103: adapter_->CloseSession(web_session_id_, promise.Pass()); On 2014/09/17 23:54:34, ddorwin wrote: > On 2014/09/15 18:22:39, jrummell wrote: > > On 2014/09/12 21:46:29, ddorwin wrote: > > > CloseSession() is probably more appropriate, though I don't know what the > > Blink > > > conversion is. > > > > Blink doesn't call this. So it doesn't matter. Changed to close. > > If this is never called, NOTREACHED()? Updated all 3 old methods to NOTREACHED(). https://codereview.chromium.org/555223004/diff/60001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:131: scoped_ptr<media::NewSessionCdmPromise> promise( On 2014/09/17 23:54:34, ddorwin wrote: > For consistency (and less code here), does it make sense to have something like > CreateSimpleCdmPromise that takes a callback (OnSessionInitialized)? Done. https://codereview.chromium.org/555223004/diff/60001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:236: uint32 result_index, On 2014/09/17 23:54:34, ddorwin wrote: > This code doesn't care about result_index. This is another reason to address the > comment at l131. Although the status is needed. Done. https://codereview.chromium.org/555223004/diff/60001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:242: status = blink::WebContentDecryptionModuleResult::SessionNotFound; On 2014/09/17 23:54:34, ddorwin wrote: > On 2014/09/15 18:22:40, jrummell wrote: > > On 2014/09/12 21:46:30, ddorwin wrote: > > > We should know that we called load() and check that here. > > > !web_session_id_.empty() ??? > > > ^ If so, this and line 276 can be collapsed into a single check at 272. > > > > load() hasn't been implemented here yet -- coming soon. > > But doesn't this code only exist for that? Let's assume it is implemented. :) I see. You're assuming that load() sets web_session_id_ on the call, which I wasn't. Right now the WebCDMResult::completeWithSession() for generateRequest() fails if it doesn't get NewSession. https://codereview.chromium.org/555223004/diff/60001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:257: void WebContentDecryptionModuleSessionImpl::OnPromiseRejected( On 2014/09/17 23:54:34, ddorwin wrote: > It would be nice for this to go away as well (related to commant at l131). Done. https://codereview.chromium.org/555223004/diff/60001/media/cdm/aes_decryptor.cc File media/cdm/aes_decryptor.cc (right): https://codereview.chromium.org/555223004/diff/60001/media/cdm/aes_decryptor.... media/cdm/aes_decryptor.cc:353: // AesDecryptor doesn't keep any persistent data, so this should be On 2014/09/17 23:54:34, ddorwin wrote: > FYI, "persistent" is optional for CK. We should replace the hacky ECK > implementation with a real CK implementation. File a bug? (No need to reference > here.) Updated issue 384152 to note that if AesDecryptor needs to support persistent sessions, it should be removed from ECK. https://codereview.chromium.org/555223004/diff/60001/media/cdm/ppapi/external... File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right): https://codereview.chromium.org/555223004/diff/60001/media/cdm/ppapi/external... media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:357: decryptor_.RemoveSession(web_session_str, promise.Pass()); On 2014/09/17 23:54:34, ddorwin wrote: > On 2014/09/15 18:22:41, jrummell wrote: > > On 2014/09/12 21:46:31, ddorwin wrote: > > > We're implementing persistent session support on a decryptor that does not > > > support it. I think we will need to call CloseSession() in the future. Maybe > a > > > TODO just so we don't forget this discussion. > > > > > > Before we didn't even call the decryptor_ - I'm not sure how that worked. > > > > The problem with calling close() is that it will reject the promise if the > > session is already closed. Since aes_decryptor doesn't support persistent > > session, I have it close the session if it exists, but it always resolves the > > promise successfully. > > Is this due to the prefixed support? If so, add a comment here about how to fix > once prefixed is removed? This is all part of ECK creating a simulated persistent session (and having tests manipulate it). AesDecryptor::remove just calls close anyway to make this work. When we cleanup AesDecryptor to "support" persistent sessions, then the check and else part will go away, and it will be up to AesDecryptor to handle it appropriately.
https://codereview.chromium.org/555223004/diff/100001/content/renderer/media/... File content/renderer/media/webcontentdecryptionmodule_impl.cc (right): https://codereview.chromium.org/555223004/diff/100001/content/renderer/media/... content/renderer/media/webcontentdecryptionmodule_impl.cc:83: : adapter_(adapter), weak_ptr_factory_(this) { weak_ptr_factory_ is no longer used. https://codereview.chromium.org/555223004/diff/100001/content/renderer/media/... File content/renderer/media/webcontentdecryptionmoduleresult_helper.cc (right): https://codereview.chromium.org/555223004/diff/100001/content/renderer/media/... content/renderer/media/webcontentdecryptionmoduleresult_helper.cc:37: class WebSimpleCdmPromise : public media::SimpleCdmPromise { Can we eliminate a lot of duplication using templates? I thought that was the plan. (The base classes here are just aliases for templates.) That would appear to eliminate most duplication. WebNewSessionCdmPromise seems to be the exception due to the CB and uma_name - see comments in header file. The latter we can address. The former we should discuss. We might need a concrete subclass of the templated one to handle that. See the next .cc file for more comments. https://codereview.chromium.org/555223004/diff/100001/content/renderer/media/... File content/renderer/media/webcontentdecryptionmoduleresult_helper.h (right): https://codereview.chromium.org/555223004/diff/100001/content/renderer/media/... content/renderer/media/webcontentdecryptionmoduleresult_helper.h:12: #include "base/memory/weak_ptr.h" not used https://codereview.chromium.org/555223004/diff/100001/content/renderer/media/... content/renderer/media/webcontentdecryptionmoduleresult_helper.h:13: #include "media/base/media_keys.h" used? https://codereview.chromium.org/555223004/diff/100001/content/renderer/media/... content/renderer/media/webcontentdecryptionmoduleresult_helper.h:23: // Take a copy of |result| and keep it around until needed. Returns a s/Take/Make/ https://codereview.chromium.org/555223004/diff/100001/content/renderer/media/... content/renderer/media/webcontentdecryptionmoduleresult_helper.h:23: // Take a copy of |result| and keep it around until needed. Returns a The copy is an impl detail. The first sentence should describe the primary behavior. The copy should come later. Much of these descriptions are common. Maybe there can be a general comment followed by specifics about the specific Complete method that is called. https://codereview.chromium.org/555223004/diff/100001/content/renderer/media/... content/renderer/media/webcontentdecryptionmoduleresult_helper.h:40: const NewSessionCreatedCB& new_session_created_cb, Is it a goal to eliminate this CB someday? https://codereview.chromium.org/555223004/diff/100001/content/renderer/media/... content/renderer/media/webcontentdecryptionmoduleresult_helper.h:41: std::string uma_name); Why does only this one have a UMA? Do we not report UMAs for others? From an API design perspective, these should all be consistent, even if we don't currently pass uma names for all of them. In that case, you might swap the last two parameters here. https://codereview.chromium.org/555223004/diff/100001/content/renderer/media/... content/renderer/media/webcontentdecryptionmoduleresult_helper.h:52: DISALLOW_COPY_AND_ASSIGN(WebContentDecryptionModuleResultHelper); DISALLOW_IMPLICIT_CONSTRUCTORS - it should never be instantiated. (It's probably debatable whether it should be in a class.) https://codereview.chromium.org/555223004/diff/100001/content/renderer/media/... File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right): https://codereview.chromium.org/555223004/diff/100001/content/renderer/media/... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:38: blink::WebString WebContentDecryptionModuleSessionImpl::sessionId() const { Per our conversation last week, we want to eliminate this. https://codereview.chromium.org/555223004/diff/100001/content/renderer/media/... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:102: DCHECK(response); DCHECK(!web_session_id_.empty()) here and below. https://codereview.chromium.org/555223004/diff/100001/content/renderer/media/... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:132: scoped_ptr<media::SimpleCdmPromise> promise = just call close(). Can we remove this? If not, is it covered by a TODO? https://codereview.chromium.org/555223004/diff/100001/content/renderer/media/... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:188: // CDM will return NULL if the session to be loaded can't be found. Following up on my previous comment: We probably do need this CB. Since the CB is an impl detail of this class, we should probably define the concrete child in this file/class rather than the common one.
Updated to use BlinkCdmPromiseTemplate. Didn't rename the file in case the name needs to change. https://codereview.chromium.org/555223004/diff/100001/content/renderer/media/... File content/renderer/media/webcontentdecryptionmodule_impl.cc (right): https://codereview.chromium.org/555223004/diff/100001/content/renderer/media/... content/renderer/media/webcontentdecryptionmodule_impl.cc:83: : adapter_(adapter), weak_ptr_factory_(this) { On 2014/09/23 18:57:31, ddorwin wrote: > weak_ptr_factory_ is no longer used. Done. https://codereview.chromium.org/555223004/diff/100001/content/renderer/media/... File content/renderer/media/webcontentdecryptionmoduleresult_helper.cc (right): https://codereview.chromium.org/555223004/diff/100001/content/renderer/media/... content/renderer/media/webcontentdecryptionmoduleresult_helper.cc:37: class WebSimpleCdmPromise : public media::SimpleCdmPromise { On 2014/09/23 18:57:31, ddorwin wrote: > Can we eliminate a lot of duplication using templates? I thought that was the > plan. (The base classes here are just aliases for templates.) That would appear > to eliminate most duplication. > > WebNewSessionCdmPromise seems to be the exception due to the CB and uma_name - > see comments in header file. The latter we can address. The former we should > discuss. We might need a concrete subclass of the templated one to handle that. > See the next .cc file for more comments. Done. https://codereview.chromium.org/555223004/diff/100001/content/renderer/media/... File content/renderer/media/webcontentdecryptionmoduleresult_helper.h (right): https://codereview.chromium.org/555223004/diff/100001/content/renderer/media/... content/renderer/media/webcontentdecryptionmoduleresult_helper.h:12: #include "base/memory/weak_ptr.h" On 2014/09/23 18:57:31, ddorwin wrote: > not used Done. https://codereview.chromium.org/555223004/diff/100001/content/renderer/media/... content/renderer/media/webcontentdecryptionmoduleresult_helper.h:13: #include "media/base/media_keys.h" On 2014/09/23 18:57:31, ddorwin wrote: > used? Needed for SimpleCdmPromise, NewSessionCdmPromise, etc. https://codereview.chromium.org/555223004/diff/100001/content/renderer/media/... content/renderer/media/webcontentdecryptionmoduleresult_helper.h:23: // Take a copy of |result| and keep it around until needed. Returns a On 2014/09/23 18:57:31, ddorwin wrote: > s/Take/Make/ Done. https://codereview.chromium.org/555223004/diff/100001/content/renderer/media/... content/renderer/media/webcontentdecryptionmoduleresult_helper.h:23: // Take a copy of |result| and keep it around until needed. Returns a On 2014/09/23 18:57:31, ddorwin wrote: > The copy is an impl detail. The first sentence should describe the primary > behavior. The copy should come later. Much of these descriptions are common. > Maybe there can be a general comment followed by specifics about the specific > Complete method that is called. Done. https://codereview.chromium.org/555223004/diff/100001/content/renderer/media/... content/renderer/media/webcontentdecryptionmoduleresult_helper.h:40: const NewSessionCreatedCB& new_session_created_cb, On 2014/09/23 18:57:31, ddorwin wrote: > Is it a goal to eliminate this CB someday? Somebody has to decide if this is a new session, already existing session, or not found (for load()). As well, now that the session_id is known (for createSession()), a call needs to be made to the adapter to register it so that events can reach the correct object. https://codereview.chromium.org/555223004/diff/100001/content/renderer/media/... content/renderer/media/webcontentdecryptionmoduleresult_helper.h:41: std::string uma_name); On 2014/09/23 18:57:31, ddorwin wrote: > Why does only this one have a UMA? Do we not report UMAs for others? > From an API design perspective, these should all be consistent, even if we don't > currently pass uma names for all of them. In that case, you might swap the last > two parameters here. UMA is only done for CreateSession. However, all Create..Promise() methods take |uma_name|. https://codereview.chromium.org/555223004/diff/100001/content/renderer/media/... content/renderer/media/webcontentdecryptionmoduleresult_helper.h:52: DISALLOW_COPY_AND_ASSIGN(WebContentDecryptionModuleResultHelper); On 2014/09/23 18:57:31, ddorwin wrote: > DISALLOW_IMPLICIT_CONSTRUCTORS - it should never be instantiated. > (It's probably debatable whether it should be in a class.) Done. https://codereview.chromium.org/555223004/diff/100001/content/renderer/media/... File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right): https://codereview.chromium.org/555223004/diff/100001/content/renderer/media/... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:38: blink::WebString WebContentDecryptionModuleSessionImpl::sessionId() const { On 2014/09/23 18:57:31, ddorwin wrote: > Per our conversation last week, we want to eliminate this. Right now the session id is not passed back to blink, so it has to be obtained this way. Future CL (or three). https://codereview.chromium.org/555223004/diff/100001/content/renderer/media/... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:102: DCHECK(response); On 2014/09/23 18:57:31, ddorwin wrote: > DCHECK(!web_session_id_.empty()) here and below. Done. https://codereview.chromium.org/555223004/diff/100001/content/renderer/media/... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:132: scoped_ptr<media::SimpleCdmPromise> promise = On 2014/09/23 18:57:31, ddorwin wrote: > just call close(). Can we remove this? If not, is it covered by a TODO? Changed. Comment in the .h file about removing it. https://codereview.chromium.org/555223004/diff/100001/content/renderer/media/... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:188: // CDM will return NULL if the session to be loaded can't be found. On 2014/09/23 18:57:31, ddorwin wrote: > Following up on my previous comment: We probably do need this CB. Since the CB > is an impl detail of this class, we should probably define the concrete child in > this file/class rather than the common one. Done.
Thanks. Looking good, but one important question. https://codereview.chromium.org/555223004/diff/120001/content/renderer/media/... File content/renderer/media/webcontentdecryptionmodule_impl.cc (right): https://codereview.chromium.org/555223004/diff/120001/content/renderer/media/... content/renderer/media/webcontentdecryptionmodule_impl.cc:108: scoped_ptr<media::SimpleCdmPromise> promise = This might be simpler as: coped_ptr<media::SimpleCdmPromise> promise(new BlinkCdmPromiseTemplate<void>(result)); Or, just inline it with the call at line 112. https://codereview.chromium.org/555223004/diff/120001/content/renderer/media/... File content/renderer/media/webcontentdecryptionmoduleresult_helper.cc (right): https://codereview.chromium.org/555223004/diff/120001/content/renderer/media/... content/renderer/media/webcontentdecryptionmoduleresult_helper.cc:63: // This must be overridden in a subclass. Will it link if you leave this out? Actually, how does overriding it work? base::Bind() is given a specific function, not a vtable entry. https://codereview.chromium.org/555223004/diff/120001/content/renderer/media/... content/renderer/media/webcontentdecryptionmoduleresult_helper.cc:72: webCDMResult_.completeWithError( nit: This is a bit simpler if you just call OnReject. https://codereview.chromium.org/555223004/diff/120001/content/renderer/media/... File content/renderer/media/webcontentdecryptionmoduleresult_helper.h (right): https://codereview.chromium.org/555223004/diff/120001/content/renderer/media/... content/renderer/media/webcontentdecryptionmoduleresult_helper.h:34: template <> // Specialization for no parameter to resolve(). https://codereview.chromium.org/555223004/diff/120001/content/renderer/media/... File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right): https://codereview.chromium.org/555223004/diff/120001/content/renderer/media/... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:107: make_scoped_ptr<media::NewSessionCdmPromise>(new WebNewSessionCdmPromise( ditto. here and below. WDYT? https://codereview.chromium.org/555223004/diff/120001/content/renderer/media/... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:128: new BlinkCdmPromiseTemplate<void>(result)); Feel free to typedef this one. :)
Updated. Also kicked off trybots to make sure the binding works on all platforms. https://codereview.chromium.org/555223004/diff/120001/content/renderer/media/... File content/renderer/media/webcontentdecryptionmodule_impl.cc (right): https://codereview.chromium.org/555223004/diff/120001/content/renderer/media/... content/renderer/media/webcontentdecryptionmodule_impl.cc:108: scoped_ptr<media::SimpleCdmPromise> promise = On 2014/09/23 23:14:21, ddorwin wrote: > This might be simpler as: > coped_ptr<media::SimpleCdmPromise> promise(new > BlinkCdmPromiseTemplate<void>(result)); > > Or, just inline it with the call at line 112. Done. https://codereview.chromium.org/555223004/diff/120001/content/renderer/media/... File content/renderer/media/webcontentdecryptionmoduleresult_helper.cc (right): https://codereview.chromium.org/555223004/diff/120001/content/renderer/media/... content/renderer/media/webcontentdecryptionmoduleresult_helper.cc:63: // This must be overridden in a subclass. On 2014/09/23 23:14:21, ddorwin wrote: > Will it link if you leave this out? > Actually, how does overriding it work? base::Bind() is given a specific > function, not a vtable entry. Linking fails if I leave it out. Testing with the EME layout tests succeed, so it must be binding to the right thing. https://codereview.chromium.org/555223004/diff/120001/content/renderer/media/... content/renderer/media/webcontentdecryptionmoduleresult_helper.cc:72: webCDMResult_.completeWithError( On 2014/09/23 23:14:21, ddorwin wrote: > nit: This is a bit simpler if you just call OnReject. Fits on one line now. https://codereview.chromium.org/555223004/diff/120001/content/renderer/media/... File content/renderer/media/webcontentdecryptionmoduleresult_helper.h (right): https://codereview.chromium.org/555223004/diff/120001/content/renderer/media/... content/renderer/media/webcontentdecryptionmoduleresult_helper.h:34: template <> On 2014/09/23 23:14:21, ddorwin wrote: > // Specialization for no parameter to resolve(). Done. https://codereview.chromium.org/555223004/diff/120001/content/renderer/media/... File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right): https://codereview.chromium.org/555223004/diff/120001/content/renderer/media/... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:107: make_scoped_ptr<media::NewSessionCdmPromise>(new WebNewSessionCdmPromise( On 2014/09/23 23:14:21, ddorwin wrote: > ditto. here and below. WDYT? Done. https://codereview.chromium.org/555223004/diff/120001/content/renderer/media/... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:128: new BlinkCdmPromiseTemplate<void>(result)); On 2014/09/23 23:14:21, ddorwin wrote: > Feel free to typedef this one. :) Not sure there is much benefit now that it is inlined, but done as it makes for easier reading.
Looking good. A few cosmetic issues, plus we need to make sure we've handled prefixed correctly. https://codereview.chromium.org/555223004/diff/120001/content/renderer/media/... File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right): https://codereview.chromium.org/555223004/diff/120001/content/renderer/media/... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:128: new BlinkCdmPromiseTemplate<void>(result)); On 2014/09/24 00:49:32, jrummell wrote: > On 2014/09/23 23:14:21, ddorwin wrote: > > Feel free to typedef this one. :) > > Not sure there is much benefit now that it is inlined, but done as it makes for > easier reading. Agreed - it was mainly for readability. https://codereview.chromium.org/555223004/diff/140001/content/renderer/media/... File content/renderer/media/crypto/proxy_decryptor.cc (right): https://codereview.chromium.org/555223004/diff/140001/content/renderer/media/... content/renderer/media/crypto/proxy_decryptor.cc:221: media_keys_->RemoveSession(web_session_id, promise.Pass()); This may not be correct looking at the flow in ToT, which currently goes to CloseSession in the CDM. We need to discuss. https://codereview.chromium.org/555223004/diff/140001/content/renderer/media/... File content/renderer/media/webcontentdecryptionmodule_impl.cc (right): https://codereview.chromium.org/555223004/diff/140001/content/renderer/media/... content/renderer/media/webcontentdecryptionmodule_impl.cc:110: make_scoped_ptr<media::SimpleCdmPromise>( According to https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped..., you don't need the <>. That makes this simpler and probably makes the next comment obsolete. https://codereview.chromium.org/555223004/diff/140001/content/renderer/media/... content/renderer/media/webcontentdecryptionmodule_impl.cc:111: new WebSimpleCdmPromise(result))); Would this be simpler if we made a templated Create() method for the Web* promises? https://codereview.chromium.org/555223004/diff/140001/content/renderer/media/... File content/renderer/media/webcontentdecryptionmoduleresult_helper.h (right): https://codereview.chromium.org/555223004/diff/140001/content/renderer/media/... content/renderer/media/webcontentdecryptionmoduleresult_helper.h:22: class WebCdmPromiseTemplate : public media::CdmPromiseTemplate<T> { While thinking about how to name this file, I realized that this name is probably not accurate. (Sorry. :( ) WebCdmResultWrapper? WebCdmResultWrapperPromise? WDYT? https://codereview.chromium.org/555223004/diff/140001/content/renderer/media/... File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right): https://codereview.chromium.org/555223004/diff/140001/content/renderer/media/... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:111: make_scoped_ptr<media::NewSessionCdmPromise>(new WebNewSessionCdmPromise( ditto here and below.
Updated. Will rename webcontentdecryptionmoduleresult_helper if new name sticks. https://codereview.chromium.org/555223004/diff/140001/content/renderer/media/... File content/renderer/media/crypto/proxy_decryptor.cc (right): https://codereview.chromium.org/555223004/diff/140001/content/renderer/media/... content/renderer/media/crypto/proxy_decryptor.cc:221: media_keys_->RemoveSession(web_session_id, promise.Pass()); On 2014/09/24 17:37:46, ddorwin wrote: > This may not be correct looking at the flow in ToT, which currently goes to > CloseSession in the CDM. We need to discuss. Prior to https://codereview.chromium.org/496143002 (2 weeks ago) ReleaseSession() did call RemoveSession(). https://codereview.chromium.org/497153005 (4 weeks ago) description implies that RemoveSession() is what should be called. No idea why no bugs for this in the last 2 weeks. https://codereview.chromium.org/555223004/diff/140001/content/renderer/media/... File content/renderer/media/webcontentdecryptionmodule_impl.cc (right): https://codereview.chromium.org/555223004/diff/140001/content/renderer/media/... content/renderer/media/webcontentdecryptionmodule_impl.cc:110: make_scoped_ptr<media::SimpleCdmPromise>( On 2014/09/24 17:37:46, ddorwin wrote: > According to > https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped..., > you don't need the <>. That makes this simpler and probably makes the next > comment obsolete. Without the type make_scoped_ptr returns scoped_ptr<WebSimpleCdmPromise>, and it can't convert that to scoped_ptr<SimpleCdmPromise>. https://codereview.chromium.org/555223004/diff/140001/content/renderer/media/... content/renderer/media/webcontentdecryptionmodule_impl.cc:111: new WebSimpleCdmPromise(result))); On 2014/09/24 17:37:46, ddorwin wrote: > Would this be simpler if we made a templated Create() method for the Web* > promises? I could, but Create() would return scoped_ptr<WebSimpleCdmPromise>, so I would have to write WebSimpleCdmPromise::Create(result).PassAs<SimpleCdmPromise>(), which is no shorter. Or switch the template to <T, U>, with U being the type returned by Create(). https://codereview.chromium.org/555223004/diff/140001/content/renderer/media/... File content/renderer/media/webcontentdecryptionmoduleresult_helper.h (right): https://codereview.chromium.org/555223004/diff/140001/content/renderer/media/... content/renderer/media/webcontentdecryptionmoduleresult_helper.h:22: class WebCdmPromiseTemplate : public media::CdmPromiseTemplate<T> { On 2014/09/24 17:37:47, ddorwin wrote: > While thinking about how to name this file, I realized that this name is > probably not accurate. (Sorry. :( ) > WebCdmResultWrapper? WebCdmResultWrapperPromise? WDYT? CdmResultPromise it is. https://codereview.chromium.org/555223004/diff/140001/content/renderer/media/... File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right): https://codereview.chromium.org/555223004/diff/140001/content/renderer/media/... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:111: make_scoped_ptr<media::NewSessionCdmPromise>(new WebNewSessionCdmPromise( On 2014/09/24 17:37:47, ddorwin wrote: > ditto here and below. Acknowledged.
LGTM % minor naming issues. Go ahead and rename the two files too. https://codereview.chromium.org/555223004/diff/140001/content/renderer/media/... File content/renderer/media/crypto/proxy_decryptor.cc (right): https://codereview.chromium.org/555223004/diff/140001/content/renderer/media/... content/renderer/media/crypto/proxy_decryptor.cc:221: media_keys_->RemoveSession(web_session_id, promise.Pass()); On 2014/09/24 22:32:32, jrummell wrote: > On 2014/09/24 17:37:46, ddorwin wrote: > > This may not be correct looking at the flow in ToT, which currently goes to > > CloseSession in the CDM. We need to discuss. > > Prior to https://codereview.chromium.org/496143002 (2 weeks ago) > ReleaseSession() did call RemoveSession(). > https://codereview.chromium.org/497153005 (4 weeks ago) description implies that > RemoveSession() is what should be called. No idea why no bugs for this in the > last 2 weeks. Confirmed and filed bug 417481, which this CL fixes. https://codereview.chromium.org/555223004/diff/160001/content/renderer/media/... File content/renderer/media/webcontentdecryptionmoduleresult_helper.h (right): https://codereview.chromium.org/555223004/diff/160001/content/renderer/media/... content/renderer/media/webcontentdecryptionmoduleresult_helper.h:61: typedef CdmResultPromise<media::KeyIdsVector> KeyIdsResultPromise; Why does one have "Cdm" and the other not? They should be consistent (probably with "Cdm"). https://codereview.chromium.org/555223004/diff/160001/content/renderer/media/... File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right): https://codereview.chromium.org/555223004/diff/160001/content/renderer/media/... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:24: class NewSessionResultPromise : public CdmResultPromise<std::string> { ditto
jrummell@chromium.org changed reviewers: + dmichael@chromium.org
Updated. +dmichael@ for OWNERS review of content/renderer/pepper/content_decryptor_delegate https://codereview.chromium.org/555223004/diff/160001/content/renderer/media/... File content/renderer/media/webcontentdecryptionmoduleresult_helper.h (right): https://codereview.chromium.org/555223004/diff/160001/content/renderer/media/... content/renderer/media/webcontentdecryptionmoduleresult_helper.h:61: typedef CdmResultPromise<media::KeyIdsVector> KeyIdsResultPromise; On 2014/09/24 22:47:30, ddorwin wrote: > Why does one have "Cdm" and the other not? They should be consistent (probably > with "Cdm"). To match the CdmPromise typedefs. Changed to include Cdm. https://codereview.chromium.org/555223004/diff/160001/content/renderer/media/... File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right): https://codereview.chromium.org/555223004/diff/160001/content/renderer/media/... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:24: class NewSessionResultPromise : public CdmResultPromise<std::string> { On 2014/09/24 22:47:30, ddorwin wrote: > ditto Done.
LGTM, including new filenames. Some questions about possibly moving some declarations closer to where they are used. https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... File content/renderer/media/cdm_result_promise.cc (right): https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_result_promise.cc:111: template class CdmResultPromise<std::string>; ditto - and this might allow us to avoid anyone accidentally using line 59. https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_result_promise.cc:112: template class CdmResultPromise<media::KeyIdsVector>; ditto https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... File content/renderer/media/cdm_result_promise.h (right): https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_result_promise.h:61: typedef CdmResultPromise<media::KeyIdsVector> KeyIdsCdmResultPromise; Can we define this where it's used? This file probably doesn't need to expose this.
https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... File content/renderer/media/cdm_result_promise.cc (right): https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_result_promise.cc:111: template class CdmResultPromise<std::string>; On 2014/09/25 00:07:15, ddorwin wrote: > ditto - and this might allow us to avoid anyone accidentally using line 59. Nevermind, that doesn't work with the implementation in a .cc file. https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_result_promise.cc:112: template class CdmResultPromise<media::KeyIdsVector>; On 2014/09/25 00:07:15, ddorwin wrote: > ditto Nevermind. https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... File content/renderer/media/cdm_result_promise.h (right): https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_result_promise.h:61: typedef CdmResultPromise<media::KeyIdsVector> KeyIdsCdmResultPromise; On 2014/09/25 00:07:16, ddorwin wrote: > Can we define this where it's used? This file probably doesn't need to expose > this. We could move this, but we can't move the others. It doesn't matter either way.
https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... File content/renderer/media/cdm_result_promise.cc (right): https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_result_promise.cc:41: base::Bind(&CdmResultPromise::OnReject, base::Unretained(this))), This patten is a bit tricky. Might worth some documentation on how it works and why Unretained(this) is okay (because it's the same object...) Actually, CdmPromiseTemplate doesn't do much work: it calls the resolve/reject cb and report UMA. Can we make it an interface and move the work here? Then we don't need to use the base class to do work and the code would be simpler (I hope)... But I might be missing something here... let's discuss. https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_result_promise.cc:54: } Is it possible to only keep the version with uma_name? If uma_name is empty, then we don't report UMA... Will that work? https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_result_promise.cc:59: NOTIMPLEMENTED(); NOTREACHED https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... File content/renderer/media/cdm_result_promise.h (right): https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_result_promise.h:24: CdmResultPromise(blink::WebContentDecryptionModuleResult result); explicit https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_result_promise.h:25: CdmResultPromise(blink::WebContentDecryptionModuleResult result, Pass the |result| by const-ref? I remember we had the discussion about whether to pass the result by value or by ref. But I don't remember the details. If that's the case, we should document it here. https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_result_promise.h:26: const std::string& uma_name); Need a documentation about the uma_name version. https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_result_promise.h:27: Here and in CdmPromiseTemplate, we should declare the dtor explicitly, otherwise, the default dtor will be inlined, which is not recommended: http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in.... https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_result_promise.h:32: const std::string& error_message); We need some documentation here why we choose virtual but not pure virtual for OnResolve, and don't use virtual for OnReject... https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_result_promise.h:34: blink::WebContentDecryptionModuleResult webCDMResult_; use chromium style: web_cdm_result_; https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... File content/renderer/media/cdm_session_adapter.h (right): https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_session_adapter.h:50: // Provide a server certificate to be used to encrypt messages to the Provide_s_ https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_session_adapter.h:51: // license server. Takes ownership of |promise|. nit: "Takes ownership of |promise|." is redundant since it's a scoped_ptr. https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_session_adapter.h:80: // Takes ownership of |promise|. ditto https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_session_adapter.h:87: // Takes ownership of |promise|. ditto https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_session_adapter.h:91: // Remove stored session data associated with the session specified by Remove_s_ https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_session_adapter.h:92: // |web_session_id|. Takes ownership of |promise|. ditto https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_session_adapter.h:96: // Retrieve the key IDs for keys in the session that the CDM knows are Retrieve_s_ https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_session_adapter.h:131: double new_expiry_time); expiration is an attribute... and expirationchange isn't in the spec. Personally I like the implementation to be as close to the spec as possible... how about just OnSessionExpiration? Or OnSessionExpirationUpdate based on this: https://dvcs.w3.org/hg/html-media/raw-file/tip/encrypted-media/encrypted-medi... https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_session_adapter.h:131: double new_expiry_time); What is the unit of this time? I saw at one point we do ppapi::PPTimeToTime(new_expiry_time).ToDoubleT()), based on the doc: https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.h&r... the unit is seconds. But in the spec, The expiration attribute is the time, in milliseconds since since 01 January, 1970 UTC. So where do we do the second -> ms conversion? Without documentation, it's pretty hard to figure out what unit a specific class/callback is using. Since this is chromium code, how about we just use base::Time? Then we only need to do ppapi::PPTimeToTime() during pepper/render boundary, and ToDoubleT at chromium/blink boundary, and there's no ambiguity about what the unit of time is. https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right): https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:68: NOTREACHED(); Will these be removed? Add a comment? https://codereview.chromium.org/555223004/diff/180001/media/base/media_keys.h File media/base/media_keys.h (right): https://codereview.chromium.org/555223004/diff/180001/media/base/media_keys.h... media/base/media_keys.h:75: // Provide a server certificate to be used to encrypt messages to the _s_ https://codereview.chromium.org/555223004/diff/180001/media/base/media_keys.h... media/base/media_keys.h:107: // Remove stored session data associated with the session specified by ditto https://codereview.chromium.org/555223004/diff/180001/media/base/media_keys.h... media/base/media_keys.h:112: // Retrieve the key IDs for keys in the session that the CDM knows are ditto https://codereview.chromium.org/555223004/diff/180001/media/base/media_keys.h... media/base/media_keys.h:146: double new_expiry_time)> SessionExpirationChangeCB; unit? or use base::Time
pepper OWNERS lgtm, when xhwang is happy
Updated. https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... File content/renderer/media/cdm_result_promise.cc (right): https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_result_promise.cc:41: base::Bind(&CdmResultPromise::OnReject, base::Unretained(this))), On 2014/09/25 05:11:46, xhwang wrote: > This patten is a bit tricky. Might worth some documentation on how it works and > why Unretained(this) is okay (because it's the same object...) > > Actually, CdmPromiseTemplate doesn't do much work: it calls the resolve/reject > cb and report UMA. Can we make it an interface and move the work here? Then we > don't need to use the base class to do work and the code would be simpler (I > hope)... But I might be missing something here... let's discuss. Discussed offline. Will be cleaned up in a future CL. https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_result_promise.cc:54: } On 2014/09/25 05:11:46, xhwang wrote: > Is it possible to only keep the version with uma_name? If uma_name is empty, > then we don't report UMA... > Will that work? CdmPromise() does a DCHECK on uma_name if specified. Will clean it up in the future CL. https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_result_promise.cc:59: NOTIMPLEMENTED(); On 2014/09/25 05:11:46, xhwang wrote: > NOTREACHED Done. https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... File content/renderer/media/cdm_result_promise.h (right): https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_result_promise.h:24: CdmResultPromise(blink::WebContentDecryptionModuleResult result); On 2014/09/25 05:11:47, xhwang wrote: > explicit Done. https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_result_promise.h:25: CdmResultPromise(blink::WebContentDecryptionModuleResult result, On 2014/09/25 05:11:47, xhwang wrote: > Pass the |result| by const-ref? I remember we had the discussion about whether > to pass the result by value or by ref. But I don't remember the details. If > that's the case, we should document it here. Done. https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_result_promise.h:26: const std::string& uma_name); On 2014/09/25 05:11:46, xhwang wrote: > Need a documentation about the uma_name version. Done. https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_result_promise.h:27: On 2014/09/25 05:11:46, xhwang wrote: > Here and in CdmPromiseTemplate, we should declare the dtor explicitly, > otherwise, the default dtor will be inlined, which is not recommended: > > http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in.... Done. Will update CdmPromiseTemplate in a future CL. https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_result_promise.h:32: const std::string& error_message); On 2014/09/25 05:11:46, xhwang wrote: > We need some documentation here why we choose virtual but not pure virtual for > OnResolve, and don't use virtual for OnReject... Done. https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_result_promise.h:34: blink::WebContentDecryptionModuleResult webCDMResult_; On 2014/09/25 05:11:46, xhwang wrote: > use chromium style: web_cdm_result_; Done. https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_result_promise.h:61: typedef CdmResultPromise<media::KeyIdsVector> KeyIdsCdmResultPromise; On 2014/09/25 00:07:16, ddorwin wrote: > Can we define this where it's used? This file probably doesn't need to expose > this. Done. https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... File content/renderer/media/cdm_session_adapter.h (right): https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_session_adapter.h:50: // Provide a server certificate to be used to encrypt messages to the On 2014/09/25 05:11:47, xhwang wrote: > Provide_s_ Done. https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_session_adapter.h:51: // license server. Takes ownership of |promise|. On 2014/09/25 05:11:47, xhwang wrote: > nit: "Takes ownership of |promise|." is redundant since it's a scoped_ptr. Done. https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_session_adapter.h:80: // Takes ownership of |promise|. On 2014/09/25 05:11:47, xhwang wrote: > ditto Done. https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_session_adapter.h:87: // Takes ownership of |promise|. On 2014/09/25 05:11:47, xhwang wrote: > ditto Done. https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_session_adapter.h:91: // Remove stored session data associated with the session specified by On 2014/09/25 05:11:47, xhwang wrote: > Remove_s_ Done. https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_session_adapter.h:92: // |web_session_id|. Takes ownership of |promise|. On 2014/09/25 05:11:47, xhwang wrote: > ditto Done. https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_session_adapter.h:96: // Retrieve the key IDs for keys in the session that the CDM knows are On 2014/09/25 05:11:47, xhwang wrote: > Retrieve_s_ Done. https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_session_adapter.h:131: double new_expiry_time); On 2014/09/25 05:11:47, xhwang wrote: > expiration is an attribute... and expirationchange isn't in the spec. Personally > I like the implementation to be as close to the spec as possible... > > how about just OnSessionExpiration? Or OnSessionExpirationUpdate based on this: > > https://dvcs.w3.org/hg/html-media/raw-file/tip/encrypted-media/encrypted-medi... Changed to OnSessionExpirationUpdate. https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/cdm_session_adapter.h:131: double new_expiry_time); On 2014/09/25 05:11:47, xhwang wrote: > What is the unit of this time? I saw at one point we do > ppapi::PPTimeToTime(new_expiry_time).ToDoubleT()), > based on the doc: > https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.h&r... > the unit is seconds. > > But in the spec, The expiration attribute is the time, in milliseconds since > since 01 January, 1970 UTC. So where do we do the second -> ms conversion? > > Without documentation, it's pretty hard to figure out what unit a specific > class/callback is using. > > Since this is chromium code, how about we just use base::Time? Then we only need > to do ppapi::PPTimeToTime() during pepper/render boundary, and ToDoubleT at > chromium/blink boundary, and there's no ambiguity about what the unit of time > is. Good idea. Done. https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right): https://codereview.chromium.org/555223004/diff/180001/content/renderer/media/... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:68: NOTREACHED(); On 2014/09/25 05:11:47, xhwang wrote: > Will these be removed? Add a comment? Comment in the header, but copied here. https://codereview.chromium.org/555223004/diff/180001/media/base/media_keys.h File media/base/media_keys.h (right): https://codereview.chromium.org/555223004/diff/180001/media/base/media_keys.h... media/base/media_keys.h:75: // Provide a server certificate to be used to encrypt messages to the On 2014/09/25 05:11:47, xhwang wrote: > _s_ Done. https://codereview.chromium.org/555223004/diff/180001/media/base/media_keys.h... media/base/media_keys.h:107: // Remove stored session data associated with the session specified by On 2014/09/25 05:11:47, xhwang wrote: > ditto Done. https://codereview.chromium.org/555223004/diff/180001/media/base/media_keys.h... media/base/media_keys.h:112: // Retrieve the key IDs for keys in the session that the CDM knows are On 2014/09/25 05:11:47, xhwang wrote: > ditto Done. https://codereview.chromium.org/555223004/diff/180001/media/base/media_keys.h... media/base/media_keys.h:146: double new_expiry_time)> SessionExpirationChangeCB; On 2014/09/25 05:11:47, xhwang wrote: > unit? or use base::Time Done.
https://codereview.chromium.org/555223004/diff/200001/content/renderer/media/... File content/renderer/media/cdm_result_promise.h (right): https://codereview.chromium.org/555223004/diff/200001/content/renderer/media/... content/renderer/media/cdm_result_promise.h:35: // when a new session is created. nit: The base class shouldn't have such details. It's sufficient to say that derived classes need to handle it. https://codereview.chromium.org/555223004/diff/200001/content/renderer/media/... File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right): https://codereview.chromium.org/555223004/diff/200001/content/renderer/media/... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:24: typedef CdmResultPromise<media::KeyIdsVector> KeyIdsCdmResultPromise; We may not even need this. It's only used once. https://codereview.chromium.org/555223004/diff/200001/content/renderer/media/... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:181: // TODO(jrummell): Update this once Blink client supports this. You might add a reminder to use InMillisecondsF(). :) Separately, we should probably add a note to CDM.h that the expected *seconds* is different from the *milliseconds* in the spec. That difference is easy to overlook if you assume CDM.h reflects the spec. https://codereview.chromium.org/555223004/diff/200001/media/base/media_keys.h File media/base/media_keys.h (right): https://codereview.chromium.org/555223004/diff/200001/media/base/media_keys.h... media/base/media_keys.h:14: #include "base/time/time.h" This could be fwd decl'd.
Thanks! lgtm % nits + ddorwin's comments https://codereview.chromium.org/555223004/diff/200001/content/renderer/media/... File content/renderer/media/cdm_result_promise.h (right): https://codereview.chromium.org/555223004/diff/200001/content/renderer/media/... content/renderer/media/cdm_result_promise.h:31: ~CdmResultPromise(); virtual https://codereview.chromium.org/555223004/diff/200001/content/renderer/media/... content/renderer/media/cdm_result_promise.h:55: ~CdmResultPromise(); virtual
Thanks for the reviews. https://codereview.chromium.org/555223004/diff/200001/content/renderer/media/... File content/renderer/media/cdm_result_promise.h (right): https://codereview.chromium.org/555223004/diff/200001/content/renderer/media/... content/renderer/media/cdm_result_promise.h:31: ~CdmResultPromise(); On 2014/09/25 21:01:52, xhwang wrote: > virtual Done. https://codereview.chromium.org/555223004/diff/200001/content/renderer/media/... content/renderer/media/cdm_result_promise.h:35: // when a new session is created. On 2014/09/25 20:54:05, ddorwin wrote: > nit: The base class shouldn't have such details. It's sufficient to say that > derived classes need to handle it. Done. https://codereview.chromium.org/555223004/diff/200001/content/renderer/media/... content/renderer/media/cdm_result_promise.h:55: ~CdmResultPromise(); On 2014/09/25 21:01:52, xhwang wrote: > virtual Done. https://codereview.chromium.org/555223004/diff/200001/content/renderer/media/... File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right): https://codereview.chromium.org/555223004/diff/200001/content/renderer/media/... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:24: typedef CdmResultPromise<media::KeyIdsVector> KeyIdsCdmResultPromise; On 2014/09/25 20:54:05, ddorwin wrote: > We may not even need this. It's only used once. Done. https://codereview.chromium.org/555223004/diff/200001/content/renderer/media/... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:181: // TODO(jrummell): Update this once Blink client supports this. On 2014/09/25 20:54:05, ddorwin wrote: > You might add a reminder to use InMillisecondsF(). :) > > Separately, we should probably add a note to CDM.h that the expected *seconds* > is different from the *milliseconds* in the spec. That difference is easy to > overlook if you assume CDM.h reflects the spec. Comment added. https://codereview.chromium.org/555223004/diff/200001/media/base/media_keys.h File media/base/media_keys.h (right): https://codereview.chromium.org/555223004/diff/200001/media/base/media_keys.h... media/base/media_keys.h:14: #include "base/time/time.h" On 2014/09/25 20:54:05, ddorwin wrote: > This could be fwd decl'd. Done.
The CQ bit was checked by jrummell@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/555223004/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as 50169a153f03924388ce99132a4287015bfbe7b3
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/80428d2eb359a2448c67ae45ece3ea5dbc2f8cef Cr-Commit-Position: refs/heads/master@{#296838} |
