Clear Key CDM should resolve the promise if session not found

Clear Key CDM should resolve the promise if session not found

Rather than rejecting the promise with an error, Clear Key should
resolve the promise with an empty string (which will get passed to
Blink as false). Also updates the browser test to verify that false
is actually returned.

For prefixed EME an error still needs to be returned.

BUG=507736
TEST=updated test passes

Committed: https://crrev.com/14fae6f63de53924e602035d8344dca6b19adf66
Cr-Commit-Position: refs/heads/master@{#338347}

[jrummell, 2015-07-08 18:45:20]

jrummell@chromium.org changed reviewers:
+ ddorwin@chromium.org, sandersd@chromium.org

[jrummell, 2015-07-08 18:45:20]

PTAL. Can only land after https://codereview.chromium.org/1219753005/.

[ddorwin, 2015-07-08 20:32:13]

https://codereview.chromium.org/1224053003/diff/1/media/cdm/proxy_decryptor.cc
File media/cdm/proxy_decryptor.cc (right):

https://codereview.chromium.org/1224053003/diff/1/media/cdm/proxy_decryptor.c...
media/cdm/proxy_decryptor.cc:184: possible_session_id, promise.Pass());
This appears to be a change in behavior.

https://codereview.chromium.org/1224053003/diff/1/media/cdm/proxy_decryptor.c...
media/cdm/proxy_decryptor.cc:392: const std::string& possible_session_id,
If keeping this, we should rename to requested_ or something like that.

https://codereview.chromium.org/1224053003/diff/1/media/cdm/proxy_decryptor.c...
media/cdm/proxy_decryptor.cc:398: key_error_cb_.Run(possible_session_id,
MediaKeys::kUnknownError, 0);
If the above is a change in behavior, perhaps we can avoid this mess by also
using string().

https://codereview.chromium.org/1224053003/diff/1/media/test/data/eme_player_...
File media/test/data/eme_player_js/player_utils.js (right):

https://codereview.chromium.org/1224053003/diff/1/media/test/data/eme_player_...
media/test/data/eme_player_js/player_utils.js:73: Utils.failTest('Session not
found.', EME_SESSION_NOT_FOUND);
failTest() is actually used to report expected "errors" in some cases. :(

[jrummell, 2015-07-08 20:58:17]

Comments only.

https://codereview.chromium.org/1224053003/diff/1/media/cdm/proxy_decryptor.cc
File media/cdm/proxy_decryptor.cc (right):

https://codereview.chromium.org/1224053003/diff/1/media/cdm/proxy_decryptor.c...
media/cdm/proxy_decryptor.cc:184: possible_session_id, promise.Pass());
On 2015/07/08 20:32:13, ddorwin wrote:
> This appears to be a change in behavior.

It shouldn't be. 2nd parameter was a std::string of stripped_init_data, and
possible_session_id is set to that on line 168 above. It's only set when
session_creation_type is set to LoadSession, and so it should be the same.

Passing it in the promise above could be considered a change in behavior, since
the session_id will get included in some errors from LoadSession() where it was
"" before, but I don't think that is an issue.

I could change line 168 to use the char* + size parameters if that would be
clearer. WDYT?

https://codereview.chromium.org/1224053003/diff/1/media/cdm/proxy_decryptor.c...
media/cdm/proxy_decryptor.cc:398: key_error_cb_.Run(possible_session_id,
MediaKeys::kUnknownError, 0);
On 2015/07/08 20:32:13, ddorwin wrote:
> If the above is a change in behavior, perhaps we can avoid this mess by also
> using string().

Looking at this part again this is a change in behavior. Previously a failed
promise would have called OnLegacySessionError with session_id empty. This
"improves" things by including the actual session_id. I can revert it if you'd
prefer that it doesn't change.

https://codereview.chromium.org/1224053003/diff/1/media/test/data/eme_player_...
File media/test/data/eme_player_js/player_utils.js (right):

https://codereview.chromium.org/1224053003/diff/1/media/test/data/eme_player_...
media/test/data/eme_player_js/player_utils.js:73: Utils.failTest('Session not
found.', EME_SESSION_NOT_FOUND);
On 2015/07/08 20:32:13, ddorwin wrote:
> failTest() is actually used to report expected "errors" in some cases. :(

I guess all that is required is a title change to end the test. I could simply
call Utils.setResultInTitle() here, but I don't think that makes things any
clearer.

[ddorwin, 2015-07-08 21:50:13]

https://codereview.chromium.org/1224053003/diff/1/media/cdm/proxy_decryptor.cc
File media/cdm/proxy_decryptor.cc (right):

https://codereview.chromium.org/1224053003/diff/1/media/cdm/proxy_decryptor.c...
media/cdm/proxy_decryptor.cc:180: weak_ptr_factory_.GetWeakPtr(),
possible_session_id)));
Correction: _This_ appears to be a change in behavior - reporting the requested
session ID instead of an empty string in the load case.

This callback should only apply to real errors and not the session-not-found
case, right?

https://codereview.chromium.org/1224053003/diff/1/media/cdm/proxy_decryptor.c...
media/cdm/proxy_decryptor.cc:184: possible_session_id, promise.Pass());
On 2015/07/08 20:58:17, jrummell wrote:
> On 2015/07/08 20:32:13, ddorwin wrote:
> > This appears to be a change in behavior.
> 
> It shouldn't be. 2nd parameter was a std::string of stripped_init_data, and
> possible_session_id is set to that on line 168 above. It's only set when
> session_creation_type is set to LoadSession, and so it should be the same.
> 
> Passing it in the promise above could be considered a change in behavior,
since
> the session_id will get included in some errors from LoadSession() where it
was
> "" before, but I don't think that is an issue.
> 
> I could change line 168 to use the char* + size parameters if that would be
> clearer. WDYT?

Oops, I commented on the wrong line. See my comment on the promise above (as you
suspected).

https://codereview.chromium.org/1224053003/diff/1/media/cdm/proxy_decryptor.c...
media/cdm/proxy_decryptor.cc:398: key_error_cb_.Run(possible_session_id,
MediaKeys::kUnknownError, 0);
On 2015/07/08 20:58:17, jrummell wrote:
> On 2015/07/08 20:32:13, ddorwin wrote:
> > If the above is a change in behavior, perhaps we can avoid this mess by also
> > using string().
> 
> Looking at this part again this is a change in behavior. Previously a failed
> promise would have called OnLegacySessionError with session_id empty. This
> "improves" things by including the actual session_id. I can revert it if you'd
> prefer that it doesn't change.

Since we're going to disable this very soon, I don't think we should change
behavior, even if it might be an "improvement."

https://codereview.chromium.org/1224053003/diff/1/media/test/data/eme_player_...
File media/test/data/eme_player_js/player_utils.js (right):

https://codereview.chromium.org/1224053003/diff/1/media/test/data/eme_player_...
media/test/data/eme_player_js/player_utils.js:73: Utils.failTest('Session not
found.', EME_SESSION_NOT_FOUND);
On 2015/07/08 20:58:17, jrummell wrote:
> On 2015/07/08 20:32:13, ddorwin wrote:
> > failTest() is actually used to report expected "errors" in some cases. :(
> 
> I guess all that is required is a title change to end the test. I could simply
> call Utils.setResultInTitle() here, but I don't think that makes things any
> clearer.

Hmm, that might be more consistent with the fact that this is not an "error" in
the spec.

More generally, "fail" depends on what the test that calls this expected.
There's nothing to do in this CL - it was just an observation.
Utils.ReportResult or something might be a better name. These "utils" functions
probably shouldn't ever "fail" unless it's something that would be considered a
DCHECK.

[jrummell, 2015-07-08 23:15:08]

Updated.

https://codereview.chromium.org/1224053003/diff/1/media/cdm/proxy_decryptor.cc
File media/cdm/proxy_decryptor.cc (right):

https://codereview.chromium.org/1224053003/diff/1/media/cdm/proxy_decryptor.c...
media/cdm/proxy_decryptor.cc:180: weak_ptr_factory_.GetWeakPtr(),
possible_session_id)));
On 2015/07/08 21:50:13, ddorwin wrote:
> Correction: _This_ appears to be a change in behavior - reporting the
requested
> session ID instead of an empty string in the load case.
> 
> This callback should only apply to real errors and not the session-not-found
> case, right?

Correct. Reverted.

https://codereview.chromium.org/1224053003/diff/1/media/cdm/proxy_decryptor.c...
media/cdm/proxy_decryptor.cc:184: possible_session_id, promise.Pass());
On 2015/07/08 21:50:13, ddorwin wrote:
> On 2015/07/08 20:58:17, jrummell wrote:
> > On 2015/07/08 20:32:13, ddorwin wrote:
> > > This appears to be a change in behavior.
> > 
> > It shouldn't be. 2nd parameter was a std::string of stripped_init_data, and
> > possible_session_id is set to that on line 168 above. It's only set when
> > session_creation_type is set to LoadSession, and so it should be the same.
> > 
> > Passing it in the promise above could be considered a change in behavior,
> since
> > the session_id will get included in some errors from LoadSession() where it
> was
> > "" before, but I don't think that is an issue.
> > 
> > I could change line 168 to use the char* + size parameters if that would be
> > clearer. WDYT?
> 
> Oops, I commented on the wrong line. See my comment on the promise above (as
you
> suspected).

Acknowledged.

https://codereview.chromium.org/1224053003/diff/1/media/cdm/proxy_decryptor.c...
media/cdm/proxy_decryptor.cc:398: key_error_cb_.Run(possible_session_id,
MediaKeys::kUnknownError, 0);
On 2015/07/08 21:50:13, ddorwin wrote:
> On 2015/07/08 20:58:17, jrummell wrote:
> > On 2015/07/08 20:32:13, ddorwin wrote:
> > > If the above is a change in behavior, perhaps we can avoid this mess by
also
> > > using string().
> > 
> > Looking at this part again this is a change in behavior. Previously a failed
> > promise would have called OnLegacySessionError with session_id empty. This
> > "improves" things by including the actual session_id. I can revert it if
you'd
> > prefer that it doesn't change.
> 
> Since we're going to disable this very soon, I don't think we should change
> behavior, even if it might be an "improvement."

Done.

https://codereview.chromium.org/1224053003/diff/1/media/test/data/eme_player_...
File media/test/data/eme_player_js/player_utils.js (right):

https://codereview.chromium.org/1224053003/diff/1/media/test/data/eme_player_...
media/test/data/eme_player_js/player_utils.js:73: Utils.failTest('Session not
found.', EME_SESSION_NOT_FOUND);
On 2015/07/08 21:50:13, ddorwin wrote:
> On 2015/07/08 20:58:17, jrummell wrote:
> > On 2015/07/08 20:32:13, ddorwin wrote:
> > > failTest() is actually used to report expected "errors" in some cases. :(
> > 
> > I guess all that is required is a title change to end the test. I could
simply
> > call Utils.setResultInTitle() here, but I don't think that makes things any
> > clearer.
> 
> Hmm, that might be more consistent with the fact that this is not an "error"
in
> the spec.
> 
> More generally, "fail" depends on what the test that calls this expected.
> There's nothing to do in this CL - it was just an observation.
> Utils.ReportResult or something might be a better name. These "utils"
functions
> probably shouldn't ever "fail" unless it's something that would be considered
a
> DCHECK.

Done.

[ddorwin, 2015-07-09 00:29:36]

In the description:
 * This is (External) Clear Key CDM, not Clear Key.
 * I don't think "empty string" in "Clear Key should resolve the promise with an
empty string" is correct. The change is a nullptr.

https://codereview.chromium.org/1224053003/diff/20001/media/cdm/proxy_decrypt...
File media/cdm/proxy_decryptor.cc (right):

https://codereview.chromium.org/1224053003/diff/20001/media/cdm/proxy_decrypt...
media/cdm/proxy_decryptor.cc:395: // Load() returns empty |session_id| if the
session is not found, so
LoadSession

https://codereview.chromium.org/1224053003/diff/20001/media/cdm/proxy_decrypt...
media/cdm/proxy_decryptor.cc:398: key_error_cb_.Run(session_id,
MediaKeys::kUnknownError, 0);
Should we call OnLegacySessionError() for consistency and readability? That
seems to be the pattern in this file.

https://codereview.chromium.org/1224053003/diff/20001/media/test/data/eme_pla...
File media/test/data/eme_player_js/player_utils.js (right):

https://codereview.chromium.org/1224053003/diff/20001/media/test/data/eme_pla...
media/test/data/eme_player_js/player_utils.js:73: Utils.reportResult('Session
not found.',
Thanks. However, I don't think we should make this change in an isolated way. We
should probably stick with failTest() for now and consider a more comprehensive
change.

https://codereview.chromium.org/1224053003/diff/20001/media/test/data/eme_pla...
File media/test/data/eme_player_js/utils.js (right):

https://codereview.chromium.org/1224053003/diff/20001/media/test/data/eme_pla...
media/test/data/eme_player_js/utils.js:127: Utils.failTest = function(msg,
newTitle) {
See below. We should comment that this should only be used for unexpected
failures.

However, I'm not sure the distinction is meaningful. A better solution might be
to rename this reportResult.

I think we should probably revisit this in a separate CL.

https://codereview.chromium.org/1224053003/diff/20001/media/test/data/eme_pla...
media/test/data/eme_player_js/utils.js:149: Utils.reportResult = function(msg,
newTitle) {
Please add some comments. For example, reports a result. The result may or may
not be a failure depending on the test expectations.

https://codereview.chromium.org/1224053003/diff/20001/media/test/data/eme_pla...
media/test/data/eme_player_js/utils.js:150: Utils.timeLog(msg);
Continuing from the first comment: If this really is a failure, we get a timeLog
instead of a documentLog. Should we in that case? What is the difference? (We're
missing comments for both of those methods. :( )

[jrummell, 2015-07-10 00:16:50]

Updated.

https://codereview.chromium.org/1224053003/diff/20001/media/cdm/proxy_decrypt...
File media/cdm/proxy_decryptor.cc (right):

https://codereview.chromium.org/1224053003/diff/20001/media/cdm/proxy_decrypt...
media/cdm/proxy_decryptor.cc:395: // Load() returns empty |session_id| if the
session is not found, so
On 2015/07/09 00:29:35, ddorwin wrote:
> LoadSession

Done.

https://codereview.chromium.org/1224053003/diff/20001/media/cdm/proxy_decrypt...
media/cdm/proxy_decryptor.cc:398: key_error_cb_.Run(session_id,
MediaKeys::kUnknownError, 0);
On 2015/07/09 00:29:35, ddorwin wrote:
> Should we call OnLegacySessionError() for consistency and readability? That
> seems to be the pattern in this file.

Sure. then it matches what was previously returned.

https://codereview.chromium.org/1224053003/diff/20001/media/test/data/eme_pla...
File media/test/data/eme_player_js/player_utils.js (right):

https://codereview.chromium.org/1224053003/diff/20001/media/test/data/eme_pla...
media/test/data/eme_player_js/player_utils.js:73: Utils.reportResult('Session
not found.',
On 2015/07/09 00:29:35, ddorwin wrote:
> Thanks. However, I don't think we should make this change in an isolated way.
We
> should probably stick with failTest() for now and consider a more
comprehensive
> change.

Reverted.

https://codereview.chromium.org/1224053003/diff/20001/media/test/data/eme_pla...
File media/test/data/eme_player_js/utils.js (right):

https://codereview.chromium.org/1224053003/diff/20001/media/test/data/eme_pla...
media/test/data/eme_player_js/utils.js:127: Utils.failTest = function(msg,
newTitle) {
On 2015/07/09 00:29:36, ddorwin wrote:
> See below. We should comment that this should only be used for unexpected
> failures.
> 
> However, I'm not sure the distinction is meaningful. A better solution might
be
> to rename this reportResult.
> 
> I think we should probably revisit this in a separate CL.

Acknowledged.

https://codereview.chromium.org/1224053003/diff/20001/media/test/data/eme_pla...
media/test/data/eme_player_js/utils.js:149: Utils.reportResult = function(msg,
newTitle) {
On 2015/07/09 00:29:36, ddorwin wrote:
> Please add some comments. For example, reports a result. The result may or may
> not be a failure depending on the test expectations.

Removed function.

https://codereview.chromium.org/1224053003/diff/20001/media/test/data/eme_pla...
media/test/data/eme_player_js/utils.js:150: Utils.timeLog(msg);
On 2015/07/09 00:29:35, ddorwin wrote:
> Continuing from the first comment: If this really is a failure, we get a
timeLog
> instead of a documentLog. Should we in that case? What is the difference?
(We're
> missing comments for both of those methods. :( )

Removed function.

[ddorwin, 2015-07-10 00:35:12]

lgtm % comment

https://codereview.chromium.org/1224053003/diff/40001/media/cdm/proxy_decrypt...
File media/cdm/proxy_decryptor.cc (right):

https://codereview.chromium.org/1224053003/diff/40001/media/cdm/proxy_decrypt...
media/cdm/proxy_decryptor.cc:399: "Incorrect session id specified for
LoadSession().");
* s/Incorrect/Unknown/ or something like that.
* LoadSession() is not something the developer knows about.
 - It turns out the message isn't used, so I guess it doesn't matter.

[jrummell, 2015-07-10 19:20:16]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2015-07-10 19:21:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1224053003/40001

[commit-bot: I haz the power, 2015-07-10 20:40:25]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-07-10 20:41:32]

Patchset 3 (id:??) landed as
https://crrev.com/14fae6f63de53924e602035d8344dca6b19adf66
Cr-Commit-Position: refs/heads/master@{#338347}