Clean up the CDM exception codes [chromium/src : master]
John Rummell (Gerrit)
John Rummell would like Xiaohan Wang to review this change.
Clean up the CDM exception codes
Currently there are only 4 EME exceptions reported to users (TypeError,
NotSupportedError, InvalidStateError, and QuotaExceededError). Internally the
code was using InvalidAccessError as TypeError, so rename it to match what
users see. The other 3 errors (UnknownError, ClientError, and OutputError)
are not used, and are removed.
This change also makes CdmPromise::Exception an enum class.
BUG=570216
TEST=EME browser_tests still pass
Change-Id: Ib0a3970651332866b10b233d36bd880d996660af
---
M content/renderer/media/cdm/ppapi_decryptor.cc
M content/renderer/pepper/content_decryptor_delegate.cc
M media/base/cdm_promise.h
M media/base/cdm_promise_adapter.cc
M media/base/ipc/media_param_traits_macros.h
M media/blink/cdm_result_promise_helper.cc
M media/blink/cdm_result_promise_helper.h
M media/blink/new_session_cdm_result_promise.cc
M media/cdm/aes_decryptor.cc
M media/cdm/cdm_adapter.cc
M media/cdm/ppapi/external_clear_key/clear_key_cdm.cc
M media/cdm/ppapi/ppapi_cdm_adapter.cc
M media/mojo/clients/mojo_cdm.cc
M media/mojo/clients/mojo_cdm_unittest.cc
M media/remoting/proto_enum_utils.cc
M media/remoting/proto_utils.cc
M media/remoting/rpc.proto
M ppapi/api/private/pp_content_decryptor.idl
M ppapi/c/private/pp_content_decryptor.h
M tools/metrics/histograms/enums.xml
20 files changed, 98 insertions(+), 133 deletions(-)
diff --git a/content/renderer/media/cdm/ppapi_decryptor.cc b/content/renderer/media/cdm/ppapi_decryptor.cc
index cdc7f3f1..c21b0f0 100644
--- a/content/renderer/media/cdm/ppapi_decryptor.cc
+++ b/content/renderer/media/cdm/ppapi_decryptor.cc
@@ -114,7 +114,7 @@
DCHECK(render_task_runner_->BelongsToCurrentThread());
if (!CdmDelegate()) {
- promise->reject(media::CdmPromise::INVALID_STATE_ERROR, 0,
+ promise->reject(media::CdmPromise::Exception::INVALID_STATE_ERROR, 0,
"CDM has failed.");
return;
}
@@ -131,7 +131,7 @@
DCHECK(render_task_runner_->BelongsToCurrentThread());
if (!CdmDelegate()) {
- promise->reject(media::CdmPromise::INVALID_STATE_ERROR, 0,
+ promise->reject(media::CdmPromise::Exception::INVALID_STATE_ERROR, 0,
"CDM has failed.");
return;
}
@@ -148,7 +148,7 @@
DCHECK(render_task_runner_->BelongsToCurrentThread());
if (!CdmDelegate()) {
- promise->reject(media::CdmPromise::INVALID_STATE_ERROR, 0,
+ promise->reject(media::CdmPromise::Exception::INVALID_STATE_ERROR, 0,
"CDM has failed.");
return;
}
@@ -163,7 +163,7 @@
DCHECK(render_task_runner_->BelongsToCurrentThread());
if (!CdmDelegate()) {
- promise->reject(media::CdmPromise::INVALID_STATE_ERROR, 0,
+ promise->reject(media::CdmPromise::Exception::INVALID_STATE_ERROR, 0,
"CDM has failed.");
return;
}
@@ -177,7 +177,7 @@
DCHECK(render_task_runner_->BelongsToCurrentThread());
if (!CdmDelegate()) {
- promise->reject(media::CdmPromise::INVALID_STATE_ERROR, 0,
+ promise->reject(media::CdmPromise::Exception::INVALID_STATE_ERROR, 0,
"CDM has failed.");
return;
}
@@ -192,7 +192,7 @@
DCHECK(render_task_runner_->BelongsToCurrentThread());
if (!CdmDelegate()) {
- promise->reject(media::CdmPromise::INVALID_STATE_ERROR, 0,
+ promise->reject(media::CdmPromise::Exception::INVALID_STATE_ERROR, 0,
"CDM has failed.");
return;
}
diff --git a/content/renderer/pepper/content_decryptor_delegate.cc b/content/renderer/pepper/content_decryptor_delegate.cc
index bd18fc8..a6a09a4 100644
--- a/content/renderer/pepper/content_decryptor_delegate.cc
+++ b/content/renderer/pepper/content_decryptor_delegate.cc
@@ -307,22 +307,16 @@
PP_CdmExceptionCode exception_code) {
switch (exception_code) {
case PP_CDMEXCEPTIONCODE_NOTSUPPORTEDERROR:
- return CdmPromise::NOT_SUPPORTED_ERROR;
+ return CdmPromise::Exception::NOT_SUPPORTED_ERROR;
case PP_CDMEXCEPTIONCODE_INVALIDSTATEERROR:
- return CdmPromise::INVALID_STATE_ERROR;
- case PP_CDMEXCEPTIONCODE_INVALIDACCESSERROR:
- return CdmPromise::INVALID_ACCESS_ERROR;
+ return CdmPromise::Exception::INVALID_STATE_ERROR;
+ case PP_CDMEXCEPTIONCODE_TYPEERROR:
+ return CdmPromise::Exception::TYPE_ERROR;
case PP_CDMEXCEPTIONCODE_QUOTAEXCEEDEDERROR:
- return CdmPromise::QUOTA_EXCEEDED_ERROR;
- case PP_CDMEXCEPTIONCODE_UNKNOWNERROR:
- return CdmPromise::UNKNOWN_ERROR;
- case PP_CDMEXCEPTIONCODE_CLIENTERROR:
- return CdmPromise::CLIENT_ERROR;
- case PP_CDMEXCEPTIONCODE_OUTPUTERROR:
- return CdmPromise::OUTPUT_ERROR;
+ return CdmPromise::Exception::QUOTA_EXCEEDED_ERROR;
default:
NOTREACHED();
- return CdmPromise::UNKNOWN_ERROR;
+ return CdmPromise::Exception::NOT_SUPPORTED_ERROR;
}
}
@@ -431,7 +425,7 @@
std::unique_ptr<media::SimpleCdmPromise> promise) {
if (certificate.size() < media::limits::kMinCertificateLength ||
certificate.size() > media::limits::kMaxCertificateLength) {
- promise->reject(CdmPromise::INVALID_ACCESS_ERROR, 0,
+ promise->reject(CdmPromise::Exception::TYPE_ERROR, 0,
"Incorrect certificate.");
return;
}
@@ -485,7 +479,7 @@
const std::string& session_id,
std::unique_ptr<SimpleCdmPromise> promise) {
if (session_id.length() > media::limits::kMaxSessionIdLength) {
- promise->reject(CdmPromise::INVALID_ACCESS_ERROR, 0, "Incorrect session.");
+ promise->reject(CdmPromise::Exception::TYPE_ERROR, 0, "Incorrect session.");
return;
}
@@ -498,7 +492,7 @@
const std::string& session_id,
std::unique_ptr<SimpleCdmPromise> promise) {
if (session_id.length() > media::limits::kMaxSessionIdLength) {
- promise->reject(CdmPromise::INVALID_ACCESS_ERROR, 0, "Incorrect session.");
+ promise->reject(CdmPromise::Exception::TYPE_ERROR, 0, "Incorrect session.");
return;
}
diff --git a/media/base/cdm_promise.h b/media/base/cdm_promise.h
index d48a114..be36bd6 100644
--- a/media/base/cdm_promise.h
+++ b/media/base/cdm_promise.h
@@ -33,17 +33,12 @@
// the pepper interface.
class MEDIA_EXPORT CdmPromise {
public:
- // TODO(jrummell): Remove deprecated errors. See
- // http://crbug.com/570216
- enum Exception {
+ enum class Exception {
NOT_SUPPORTED_ERROR,
INVALID_STATE_ERROR,
- INVALID_ACCESS_ERROR,
QUOTA_EXCEEDED_ERROR,
- UNKNOWN_ERROR,
- CLIENT_ERROR,
- OUTPUT_ERROR,
- EXCEPTION_MAX = OUTPUT_ERROR
+ TYPE_ERROR,
+ EXCEPTION_MAX = TYPE_ERROR
};
enum ResolveParameterType {
@@ -130,7 +125,7 @@
std::string message =
"Unfulfilled promise rejected automatically during destruction.";
DVLOG(1) << message;
- reject(INVALID_STATE_ERROR, 0, message);
+ reject(Exception::INVALID_STATE_ERROR, 0, message);
DCHECK(is_settled_);
}
diff --git a/media/base/cdm_promise_adapter.cc b/media/base/cdm_promise_adapter.cc
index a74c95a..584bf95 100644
--- a/media/base/cdm_promise_adapter.cc
+++ b/media/base/cdm_promise_adapter.cc
@@ -61,7 +61,8 @@
// Reject all outstanding promises.
DCHECK(thread_checker_.CalledOnValidThread());
for (auto& promise : promises_)
- promise.second->reject(CdmPromise::UNKNOWN_ERROR, 0, "Operation aborted.");
+ promise.second->reject(CdmPromise::Exception::INVALID_STATE_ERROR, 0,
+ "Operation aborted.");
promises_.clear();
}
diff --git a/media/base/ipc/media_param_traits_macros.h b/media/base/ipc/media_param_traits_macros.h
index 79083bc..3326101 100644
--- a/media/base/ipc/media_param_traits_macros.h
+++ b/media/base/ipc/media_param_traits_macros.h
@@ -67,7 +67,7 @@
media::EncryptionScheme::CipherMode::CIPHER_MODE_MAX)
IPC_ENUM_TRAITS_MAX_VALUE(media::CdmPromise::Exception,
- media::CdmPromise::EXCEPTION_MAX)
+ media::CdmPromise::Exception::EXCEPTION_MAX)
IPC_ENUM_TRAITS_MAX_VALUE(media::CdmMessageType,
media::CdmMessageType::MESSAGE_TYPE_MAX)
diff --git a/media/blink/cdm_result_promise_helper.cc b/media/blink/cdm_result_promise_helper.cc
index 16ede5d..b7811f2 100644
--- a/media/blink/cdm_result_promise_helper.cc
+++ b/media/blink/cdm_result_promise_helper.cc
@@ -12,20 +12,14 @@
CdmResultForUMA ConvertCdmExceptionToResultForUMA(
CdmPromise::Exception exception_code) {
switch (exception_code) {
- case CdmPromise::NOT_SUPPORTED_ERROR:
+ case CdmPromise::Exception::NOT_SUPPORTED_ERROR:
return NOT_SUPPORTED_ERROR;
- case CdmPromise::INVALID_STATE_ERROR:
+ case CdmPromise::Exception::INVALID_STATE_ERROR:
return INVALID_STATE_ERROR;
- case CdmPromise::INVALID_ACCESS_ERROR:
- return INVALID_ACCESS_ERROR;
- case CdmPromise::QUOTA_EXCEEDED_ERROR:
+ case CdmPromise::Exception::QUOTA_EXCEEDED_ERROR:
return QUOTA_EXCEEDED_ERROR;
- case CdmPromise::UNKNOWN_ERROR:
- return UNKNOWN_ERROR;
- case CdmPromise::CLIENT_ERROR:
- return CLIENT_ERROR;
- case CdmPromise::OUTPUT_ERROR:
- return OUTPUT_ERROR;
+ case CdmPromise::Exception::TYPE_ERROR:
+ return TYPE_ERROR;
}
NOTREACHED();
return UNKNOWN_ERROR;
@@ -34,25 +28,14 @@
blink::WebContentDecryptionModuleException ConvertCdmException(
CdmPromise::Exception exception_code) {
switch (exception_code) {
- case CdmPromise::NOT_SUPPORTED_ERROR:
+ case CdmPromise::Exception::NOT_SUPPORTED_ERROR:
return blink::kWebContentDecryptionModuleExceptionNotSupportedError;
- case CdmPromise::INVALID_STATE_ERROR:
+ case CdmPromise::Exception::INVALID_STATE_ERROR:
return blink::kWebContentDecryptionModuleExceptionInvalidStateError;
-
- // TODO(jrummell): Since InvalidAccess is not returned, thus should be
- // renamed to TYPE_ERROR. http://crbug.com/570216#c11.
- case CdmPromise::INVALID_ACCESS_ERROR:
- return blink::kWebContentDecryptionModuleExceptionTypeError;
- case CdmPromise::QUOTA_EXCEEDED_ERROR:
+ case CdmPromise::Exception::QUOTA_EXCEEDED_ERROR:
return blink::kWebContentDecryptionModuleExceptionQuotaExceededError;
- case CdmPromise::UNKNOWN_ERROR:
- return blink::kWebContentDecryptionModuleExceptionUnknownError;
-
- // These are deprecated, and should be removed.
- // http://crbug.com/570216#c11.
- case CdmPromise::CLIENT_ERROR:
- case CdmPromise::OUTPUT_ERROR:
- break;
+ case CdmPromise::Exception::TYPE_ERROR:
+ return blink::kWebContentDecryptionModuleExceptionTypeError;
}
NOTREACHED();
return blink::kWebContentDecryptionModuleExceptionUnknownError;
diff --git a/media/blink/cdm_result_promise_helper.h b/media/blink/cdm_result_promise_helper.h
index badeb5d..5d8426e 100644
--- a/media/blink/cdm_result_promise_helper.h
+++ b/media/blink/cdm_result_promise_helper.h
@@ -22,7 +22,7 @@
SUCCESS = 0,
NOT_SUPPORTED_ERROR = 1,
INVALID_STATE_ERROR = 2,
- INVALID_ACCESS_ERROR = 3,
+ TYPE_ERROR = 3,
QUOTA_EXCEEDED_ERROR = 4,
UNKNOWN_ERROR = 5,
CLIENT_ERROR = 6,
diff --git a/media/blink/new_session_cdm_result_promise.cc b/media/blink/new_session_cdm_result_promise.cc
index f2757d0..adca254 100644
--- a/media/blink/new_session_cdm_result_promise.cc
+++ b/media/blink/new_session_cdm_result_promise.cc
@@ -52,7 +52,8 @@
new_session_created_cb_.Run(session_id, &status);
if (status == SessionInitStatus::UNKNOWN_STATUS) {
- reject(INVALID_STATE_ERROR, 0, "Cannot finish session initialization");
+ reject(Exception::INVALID_STATE_ERROR, 0,
+ "Cannot finish session initialization");
return;
}
diff --git a/media/cdm/aes_decryptor.cc b/media/cdm/aes_decryptor.cc
index e149677..44698e5 100644
--- a/media/cdm/aes_decryptor.cc
+++ b/media/cdm/aes_decryptor.cc
@@ -291,7 +291,7 @@
void AesDecryptor::SetServerCertificate(
const std::vector<uint8_t>& certificate,
std::unique_ptr<SimpleCdmPromise> promise) {
- promise->reject(CdmPromise::NOT_SUPPORTED_ERROR, 0,
+ promise->reject(CdmPromise::Exception::NOT_SUPPORTED_ERROR, 0,
"SetServerCertificate() is not supported.");
}
@@ -313,7 +313,8 @@
// |init_data| is simply the key needed.
if (init_data.size() < limits::kMinKeyIdLength ||
init_data.size() > limits::kMaxKeyIdLength) {
- promise->reject(CdmPromise::NOT_SUPPORTED_ERROR, 0, "Incorrect length");
+ promise->reject(CdmPromise::Exception::NOT_SUPPORTED_ERROR, 0,
+ "Incorrect length");
return;
}
keys.push_back(init_data);
@@ -322,13 +323,13 @@
#if BUILDFLAG(USE_PROPRIETARY_CODECS)
// |init_data| is a set of 0 or more concatenated 'pssh' boxes.
if (!GetKeyIdsForCommonSystemId(init_data, &keys)) {
- promise->reject(CdmPromise::NOT_SUPPORTED_ERROR, 0,
+ promise->reject(CdmPromise::Exception::NOT_SUPPORTED_ERROR, 0,
"No supported PSSH box found.");
return;
}
break;
#else
- promise->reject(CdmPromise::NOT_SUPPORTED_ERROR, 0,
+ promise->reject(CdmPromise::Exception::NOT_SUPPORTED_ERROR, 0,
"Initialization data type CENC is not supported.");
return;
#endif
@@ -337,14 +338,15 @@
std::string error_message;
if (!ExtractKeyIdsFromKeyIdsInitData(init_data_string, &keys,
&error_message)) {
- promise->reject(CdmPromise::NOT_SUPPORTED_ERROR, 0, error_message);
+ promise->reject(CdmPromise::Exception::NOT_SUPPORTED_ERROR, 0,
+ error_message);
return;
}
break;
}
default:
NOTREACHED();
- promise->reject(CdmPromise::NOT_SUPPORTED_ERROR, 0,
+ promise->reject(CdmPromise::Exception::NOT_SUPPORTED_ERROR, 0,
"init_data_type not supported.");
return;
}
@@ -360,7 +362,7 @@
std::unique_ptr<NewSessionCdmPromise> promise) {
// TODO(xhwang): Change this to NOTREACHED() when blink checks for key systems
// that do not support loadSession. See http://crbug.com/342481
- promise->reject(CdmPromise::NOT_SUPPORTED_ERROR, 0,
+ promise->reject(CdmPromise::Exception::NOT_SUPPORTED_ERROR, 0,
"LoadSession() is not supported.");
}
@@ -374,7 +376,7 @@
// could get called on a closed session.
// https://github.com/w3c/encrypted-media/issues/365
if (open_sessions_.find(session_id) == open_sessions_.end()) {
- promise->reject(CdmPromise::INVALID_ACCESS_ERROR, 0,
+ promise->reject(CdmPromise::Exception::INVALID_STATE_ERROR, 0,
"Session does not exist.");
return;
}
@@ -384,14 +386,14 @@
KeyIdAndKeyPairs keys;
CdmSessionType session_type = CdmSessionType::TEMPORARY_SESSION;
if (!ExtractKeysFromJWKSet(key_string, &keys, &session_type)) {
- promise->reject(CdmPromise::INVALID_ACCESS_ERROR, 0,
+ promise->reject(CdmPromise::Exception::TYPE_ERROR, 0,
"Response is not a valid JSON Web Key Set.");
return;
}
// Make sure that at least one key was extracted.
if (keys.empty()) {
- promise->reject(CdmPromise::INVALID_ACCESS_ERROR, 0,
+ promise->reject(CdmPromise::Exception::TYPE_ERROR, 0,
"Response does not contain any keys.");
return;
}
@@ -401,7 +403,7 @@
if (it->second.length() !=
static_cast<size_t>(DecryptConfig::kDecryptionKeySize)) {
DVLOG(1) << "Invalid key length: " << it->second.length();
- promise->reject(CdmPromise::INVALID_ACCESS_ERROR, 0,
+ promise->reject(CdmPromise::Exception::TYPE_ERROR, 0,
"Invalid key length.");
return;
}
@@ -412,7 +414,7 @@
key_added = true;
if (!AddDecryptionKey(session_id, it->first, it->second)) {
- promise->reject(CdmPromise::INVALID_ACCESS_ERROR, 0,
+ promise->reject(CdmPromise::Exception::INVALID_STATE_ERROR, 0,
"Unable to add key.");
return;
}
@@ -473,7 +475,7 @@
if (it == open_sessions_.end()) {
// Session doesn't exist. Since this should only be called if the session
// existed at one time, this must mean the session has been closed.
- promise->reject(CdmPromise::INVALID_STATE_ERROR, 0,
+ promise->reject(CdmPromise::Exception::INVALID_STATE_ERROR, 0,
"The session is already closed.");
return;
}
diff --git a/media/cdm/cdm_adapter.cc b/media/cdm/cdm_adapter.cc
index 3003d92..e5b5ea2 100644
--- a/media/cdm/cdm_adapter.cc
+++ b/media/cdm/cdm_adapter.cc
@@ -69,23 +69,21 @@
CdmPromise::Exception ToMediaExceptionType(cdm::Error error) {
switch (error) {
case cdm::kNotSupportedError:
- return CdmPromise::NOT_SUPPORTED_ERROR;
+ return CdmPromise::Exception::NOT_SUPPORTED_ERROR;
case cdm::kInvalidStateError:
- return CdmPromise::INVALID_STATE_ERROR;
+ return CdmPromise::Exception::INVALID_STATE_ERROR;
case cdm::kInvalidAccessError:
- return CdmPromise::INVALID_ACCESS_ERROR;
+ return CdmPromise::Exception::TYPE_ERROR;
case cdm::kQuotaExceededError:
- return CdmPromise::QUOTA_EXCEEDED_ERROR;
+ return CdmPromise::Exception::QUOTA_EXCEEDED_ERROR;
case cdm::kUnknownError:
- return CdmPromise::UNKNOWN_ERROR;
case cdm::kClientError:
- return CdmPromise::CLIENT_ERROR;
case cdm::kOutputError:
- return CdmPromise::OUTPUT_ERROR;
+ break;
}
NOTREACHED() << "Unexpected cdm::Error " << error;
- return CdmPromise::UNKNOWN_ERROR;
+ return CdmPromise::Exception::NOT_SUPPORTED_ERROR;
}
CdmMessageType ToMediaMessageType(cdm::MessageType message_type) {
@@ -422,7 +420,7 @@
std::unique_ptr<media::SimpleCdmPromise> promise) {
cdm_.reset(CreateCdmInstance(key_system_, cdm_path));
if (!cdm_) {
- promise->reject(CdmPromise::INVALID_ACCESS_ERROR, 0,
+ promise->reject(CdmPromise::Exception::NOT_SUPPORTED_ERROR, 0,
"Unable to create CDM.");
return;
}
@@ -439,7 +437,7 @@
if (certificate.size() < limits::kMinCertificateLength ||
certificate.size() > limits::kMaxCertificateLength) {
- promise->reject(CdmPromise::INVALID_ACCESS_ERROR, 0,
+ promise->reject(CdmPromise::Exception::TYPE_ERROR, 0,
"Incorrect certificate.");
return;
}
diff --git a/media/cdm/ppapi/external_clear_key/clear_key_cdm.cc b/media/cdm/ppapi/external_clear_key/clear_key_cdm.cc
index 9ceaf14..59a5d46 100644
--- a/media/cdm/ppapi/external_clear_key/clear_key_cdm.cc
+++ b/media/cdm/ppapi/external_clear_key/clear_key_cdm.cc
@@ -136,20 +136,14 @@
static cdm::Error ConvertException(
media::CdmPromise::Exception exception_code) {
switch (exception_code) {
- case media::CdmPromise::NOT_SUPPORTED_ERROR:
+ case media::CdmPromise::Exception::NOT_SUPPORTED_ERROR:
return cdm::kNotSupportedError;
- case media::CdmPromise::INVALID_STATE_ERROR:
+ case media::CdmPromise::Exception::INVALID_STATE_ERROR:
return cdm::kInvalidStateError;
- case media::CdmPromise::INVALID_ACCESS_ERROR:
+ case media::CdmPromise::Exception::TYPE_ERROR:
return cdm::kInvalidAccessError;
- case media::CdmPromise::QUOTA_EXCEEDED_ERROR:
+ case media::CdmPromise::Exception::QUOTA_EXCEEDED_ERROR:
return cdm::kQuotaExceededError;
- case media::CdmPromise::UNKNOWN_ERROR:
- return cdm::kUnknownError;
- case media::CdmPromise::CLIENT_ERROR:
- return cdm::kClientError;
- case media::CdmPromise::OUTPUT_ERROR:
- return cdm::kOutputError;
}
NOTREACHED();
return cdm::kUnknownError;
diff --git a/media/cdm/ppapi/ppapi_cdm_adapter.cc b/media/cdm/ppapi/ppapi_cdm_adapter.cc
index aaa39e2..6b29212 100644
--- a/media/cdm/ppapi/ppapi_cdm_adapter.cc
+++ b/media/cdm/ppapi/ppapi_cdm_adapter.cc
@@ -262,19 +262,20 @@
case cdm::kInvalidStateError:
return PP_CDMEXCEPTIONCODE_INVALIDSTATEERROR;
case cdm::kInvalidAccessError:
- return PP_CDMEXCEPTIONCODE_INVALIDACCESSERROR;
+ return PP_CDMEXCEPTIONCODE_TYPEERROR;
case cdm::kQuotaExceededError:
return PP_CDMEXCEPTIONCODE_QUOTAEXCEEDEDERROR;
+
+ // The following are no longer used and can be removed.
+ // http://crbug.com/570216.
case cdm::kUnknownError:
- return PP_CDMEXCEPTIONCODE_UNKNOWNERROR;
case cdm::kClientError:
- return PP_CDMEXCEPTIONCODE_CLIENTERROR;
case cdm::kOutputError:
- return PP_CDMEXCEPTIONCODE_OUTPUTERROR;
+ return PP_CDMEXCEPTIONCODE_NOTSUPPORTEDERROR;
}
PP_NOTREACHED();
- return PP_CDMEXCEPTIONCODE_UNKNOWNERROR;
+ return PP_CDMEXCEPTIONCODE_NOTSUPPORTEDERROR;
}
PP_CdmMessageType CdmMessageTypeToPpMessageType(cdm::MessageType message) {
diff --git a/media/mojo/clients/mojo_cdm.cc b/media/mojo/clients/mojo_cdm.cc
index dec37c6..886490e 100644
--- a/media/mojo/clients/mojo_cdm.cc
+++ b/media/mojo/clients/mojo_cdm.cc
@@ -105,7 +105,7 @@
// If connection error has happened, fail immediately.
if (remote_cdm_.encountered_error()) {
LOG(ERROR) << "Remote CDM encountered error.";
- promise->reject(CdmPromise::NOT_SUPPORTED_ERROR, 0,
+ promise->reject(CdmPromise::Exception::NOT_SUPPORTED_ERROR, 0,
"Mojo CDM creation failed.");
return;
}
@@ -132,7 +132,7 @@
// Handle initial connection error.
if (pending_init_promise_) {
DCHECK(!cdm_session_tracker_.HasRemainingSessions());
- pending_init_promise_->reject(CdmPromise::NOT_SUPPORTED_ERROR, 0,
+ pending_init_promise_->reject(CdmPromise::Exception::NOT_SUPPORTED_ERROR, 0,
"Mojo CDM creation failed.");
// Dropping the promise could cause |this| to be destructed.
pending_init_promise_.reset();
@@ -151,7 +151,7 @@
DCHECK(thread_checker_.CalledOnValidThread());
if (!remote_cdm_) {
- promise->reject(media::CdmPromise::INVALID_STATE_ERROR, 0,
+ promise->reject(media::CdmPromise::Exception::INVALID_STATE_ERROR, 0,
"CDM connection lost.");
return;
}
@@ -171,7 +171,7 @@
DCHECK(thread_checker_.CalledOnValidThread());
if (!remote_cdm_) {
- promise->reject(media::CdmPromise::INVALID_STATE_ERROR, 0,
+ promise->reject(media::CdmPromise::Exception::INVALID_STATE_ERROR, 0,
"CDM connection lost.");
return;
}
@@ -190,7 +190,7 @@
DCHECK(thread_checker_.CalledOnValidThread());
if (!remote_cdm_) {
- promise->reject(media::CdmPromise::INVALID_STATE_ERROR, 0,
+ promise->reject(media::CdmPromise::Exception::INVALID_STATE_ERROR, 0,
"CDM connection lost.");
return;
}
@@ -208,7 +208,7 @@
DCHECK(thread_checker_.CalledOnValidThread());
if (!remote_cdm_) {
- promise->reject(media::CdmPromise::INVALID_STATE_ERROR, 0,
+ promise->reject(media::CdmPromise::Exception::INVALID_STATE_ERROR, 0,
"CDM connection lost.");
return;
}
@@ -225,7 +225,7 @@
DCHECK(thread_checker_.CalledOnValidThread());
if (!remote_cdm_) {
- promise->reject(media::CdmPromise::INVALID_STATE_ERROR, 0,
+ promise->reject(media::CdmPromise::Exception::INVALID_STATE_ERROR, 0,
"CDM connection lost.");
return;
}
@@ -242,7 +242,7 @@
DCHECK(thread_checker_.CalledOnValidThread());
if (!remote_cdm_) {
- promise->reject(media::CdmPromise::INVALID_STATE_ERROR, 0,
+ promise->reject(media::CdmPromise::Exception::INVALID_STATE_ERROR, 0,
"CDM connection lost.");
return;
}
diff --git a/media/mojo/clients/mojo_cdm_unittest.cc b/media/mojo/clients/mojo_cdm_unittest.cc
index c5352df..d70f8a7 100644
--- a/media/mojo/clients/mojo_cdm_unittest.cc
+++ b/media/mojo/clients/mojo_cdm_unittest.cc
@@ -293,7 +293,7 @@
break;
case FAILURE:
- promise->reject(media::CdmPromise::UNKNOWN_ERROR, 0,
+ promise->reject(media::CdmPromise::Exception::NOT_SUPPORTED_ERROR, 0,
"Promise rejected");
break;
@@ -329,7 +329,7 @@
break;
case FAILURE:
- promise->reject(media::CdmPromise::UNKNOWN_ERROR, 0,
+ promise->reject(media::CdmPromise::Exception::NOT_SUPPORTED_ERROR, 0,
"Promise rejected");
break;
diff --git a/media/remoting/proto_enum_utils.cc b/media/remoting/proto_enum_utils.cc
index 45e6ff7..10cfa5e 100644
--- a/media/remoting/proto_enum_utils.cc
+++ b/media/remoting/proto_enum_utils.cc
@@ -470,31 +470,32 @@
base::Optional<CdmPromise::Exception> ToCdmPromiseException(
pb::CdmException value) {
using OriginType = pb::CdmException;
- using OtherType = CdmPromise;
+ using OtherType = CdmPromise::Exception;
switch (value) {
CASE_RETURN_OTHER(NOT_SUPPORTED_ERROR);
CASE_RETURN_OTHER(INVALID_STATE_ERROR);
- CASE_RETURN_OTHER(INVALID_ACCESS_ERROR);
CASE_RETURN_OTHER(QUOTA_EXCEEDED_ERROR);
- CASE_RETURN_OTHER(UNKNOWN_ERROR);
- CASE_RETURN_OTHER(CLIENT_ERROR);
- CASE_RETURN_OTHER(OUTPUT_ERROR);
+ CASE_RETURN_OTHER(TYPE_ERROR);
+
+ // The following are no longer used.
+ case OriginType::INVALID_ACCESS_ERROR:
+ case OriginType::UNKNOWN_ERROR:
+ case OriginType::CLIENT_ERROR:
+ case OriginType::OUTPUT_ERROR:
+ return OtherType::NOT_SUPPORTED_ERROR;
}
return base::nullopt; // Not a 'default' to ensure compile-time checks.
}
base::Optional<pb::CdmException> ToProtoCdmException(
CdmPromise::Exception value) {
- using OriginType = CdmPromise;
+ using OriginType = CdmPromise::Exception;
using OtherType = pb::CdmException;
switch (value) {
CASE_RETURN_OTHER(NOT_SUPPORTED_ERROR);
CASE_RETURN_OTHER(INVALID_STATE_ERROR);
- CASE_RETURN_OTHER(INVALID_ACCESS_ERROR);
CASE_RETURN_OTHER(QUOTA_EXCEEDED_ERROR);
- CASE_RETURN_OTHER(UNKNOWN_ERROR);
- CASE_RETURN_OTHER(CLIENT_ERROR);
- CASE_RETURN_OTHER(OUTPUT_ERROR);
+ CASE_RETURN_OTHER(TYPE_ERROR);
}
return base::nullopt; // Not a 'default' to ensure compile-time checks.
}
diff --git a/media/remoting/proto_utils.cc b/media/remoting/proto_utils.cc
index 55e1ab57..a80b27b 100644
--- a/media/remoting/proto_utils.cc
+++ b/media/remoting/proto_utils.cc
@@ -412,7 +412,7 @@
return true;
}
- CdmPromise::Exception exception = CdmPromise::UNKNOWN_ERROR;
+ CdmPromise::Exception exception = CdmPromise::Exception::NOT_SUPPORTED_ERROR;
uint32_t system_code = 0;
std::string error_message;
@@ -444,7 +444,7 @@
//==============================================================================
CdmPromiseResult::CdmPromiseResult()
- : CdmPromiseResult(CdmPromise::UNKNOWN_ERROR, 0, "") {}
+ : CdmPromiseResult(CdmPromise::Exception::NOT_SUPPORTED_ERROR, 0, "") {}
CdmPromiseResult::CdmPromiseResult(CdmPromise::Exception exception,
uint32_t system_code,
diff --git a/media/remoting/rpc.proto b/media/remoting/rpc.proto
index f8f0cd6..ad4f996 100644
--- a/media/remoting/rpc.proto
+++ b/media/remoting/rpc.proto
@@ -284,6 +284,7 @@
UNKNOWN_ERROR = 4;
CLIENT_ERROR = 5;
OUTPUT_ERROR = 6;
+ TYPE_ERROR = 7;
}
// Proto version of media::CdmMessageType.
diff --git a/ppapi/api/private/pp_content_decryptor.idl b/ppapi/api/private/pp_content_decryptor.idl
index 9ac7e2f..742305c 100644
--- a/ppapi/api/private/pp_content_decryptor.idl
+++ b/ppapi/api/private/pp_content_decryptor.idl
@@ -435,12 +435,9 @@
enum PP_CdmExceptionCode {
PP_CDMEXCEPTIONCODE_NOTSUPPORTEDERROR = 1,
PP_CDMEXCEPTIONCODE_INVALIDSTATEERROR = 2,
- PP_CDMEXCEPTIONCODE_INVALIDACCESSERROR = 3,
+ PP_CDMEXCEPTIONCODE_TYPEERROR = 3,
PP_CDMEXCEPTIONCODE_QUOTAEXCEEDEDERROR = 4,
- PP_CDMEXCEPTIONCODE_UNKNOWNERROR = 5,
- PP_CDMEXCEPTIONCODE_CLIENTERROR = 6,
- PP_CDMEXCEPTIONCODE_OUTPUTERROR = 7,
- PP_CDMEXCEPTIONCODE_MAX = PP_CDMEXCEPTIONCODE_OUTPUTERROR
+ PP_CDMEXCEPTIONCODE_MAX = PP_CDMEXCEPTIONCODE_QUOTAEXCEEDEDERROR
};
/**
diff --git a/ppapi/c/private/pp_content_decryptor.h b/ppapi/c/private/pp_content_decryptor.h
index 1264560..d026f8f 100644
--- a/ppapi/c/private/pp_content_decryptor.h
+++ b/ppapi/c/private/pp_content_decryptor.h
@@ -3,7 +3,7 @@
* found in the LICENSE file.
*/
-/* From private/pp_content_decryptor.idl modified Mon Nov 21 11:44:43 2016. */
+/* From private/pp_content_decryptor.idl modified Wed Jun 14 16:48:35 2017. */
#ifndef PPAPI_C_PRIVATE_PP_CONTENT_DECRYPTOR_H_
#define PPAPI_C_PRIVATE_PP_CONTENT_DECRYPTOR_H_
@@ -484,12 +484,9 @@
typedef enum {
PP_CDMEXCEPTIONCODE_NOTSUPPORTEDERROR = 1,
PP_CDMEXCEPTIONCODE_INVALIDSTATEERROR = 2,
- PP_CDMEXCEPTIONCODE_INVALIDACCESSERROR = 3,
+ PP_CDMEXCEPTIONCODE_TYPEERROR = 3,
PP_CDMEXCEPTIONCODE_QUOTAEXCEEDEDERROR = 4,
- PP_CDMEXCEPTIONCODE_UNKNOWNERROR = 5,
- PP_CDMEXCEPTIONCODE_CLIENTERROR = 6,
- PP_CDMEXCEPTIONCODE_OUTPUTERROR = 7,
- PP_CDMEXCEPTIONCODE_MAX = PP_CDMEXCEPTIONCODE_OUTPUTERROR
+ PP_CDMEXCEPTIONCODE_MAX = PP_CDMEXCEPTIONCODE_QUOTAEXCEEDEDERROR
} PP_CdmExceptionCode;
PP_COMPILE_ASSERT_SIZE_IN_BYTES(PP_CdmExceptionCode, 4);
diff --git a/tools/metrics/histograms/enums.xml b/tools/metrics/histograms/enums.xml
index 9613905..1c0fdd4 100644
--- a/tools/metrics/histograms/enums.xml
+++ b/tools/metrics/histograms/enums.xml
@@ -3898,7 +3898,7 @@
<int value="0" label="Success"/>
<int value="1" label="NotSupportedError"/>
<int value="2" label="InvalidStateError"/>
- <int value="3" label="InvalidAccessError"/>
+ <int value="3" label="TypeError"/>
<int value="4" label="QuotaExceededError"/>
<int value="5" label="UnknownError"/>
<int value="6" label="ClientError"/>
To view, visit change 537592. To unsubscribe, visit settings.
John Rummell (Gerrit)
John Rummell posted comments on this change.
Patch set 1:Commit-Queue +1
PTAL
Xiaohan Wang (Gerrit)
Xiaohan Wang posted comments on this change.
Patch set 1:
Thanks! Generally looks good. Just a few comments.
(14 comments)
File media/blink/cdm_result_promise_helper.h:
Patch Set #1, Line 29:
OUTPUT_ERROR = 7,Can we remove these now given we should never report them?
File media/blink/cdm_result_promise_helper.cc:
Patch Set #1, Line 25:
UNKNOWN_ERRORShall we switch to another error here so that we can remove UNKNOWN_ERROR?
File media/cdm/aes_decryptor.cc:
Patch Set #1, Line 316:
promise->reject(CdmPromise::Exception::NOT_SUPPORTED_ERROR, 0,Should this be TYPE_ERROR?
Patch Set #1, Line 341:
promise->reject(CdmPromise::Exception::NOT_SUPPORTED_ERROR, 0,Should this be TYPE_ERROR?
File media/cdm/cdm_adapter.cc:
Patch Set #1, Line 79:
case cdm::kUnknownError:Add a TODO to remove these after deprecating CDM_8.
Patch Set #1, Line 85:
NOTREACHED() << "Unexpected cdm::Error " << error;Is it possible for a CDM to use these values today?
Patch Set #1, Line 86:
return CdmPromise::Exception::NOT_SUPPORTED_ERROR;Shall we use INVALID_STATE_ERROR here?
Patch Set #1, Line 423:
promise->reject(CdmPromise::Exception::NOT_SUPPORTED_ERROR, 0,Should this be INVALID_STATE_ERROR?
File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:
Patch Set #1, Line 149:
return cdm::kUnknownError;ditto that we are moving kUnknownError so pick another one?
File media/cdm/ppapi/ppapi_cdm_adapter.cc:
Patch Set #1, Line 269:
// The following are no longer used and can be removed.It cannot be removed yet. Add a TODO?
Patch Set #1, Line 278:
return PP_CDMEXCEPTIONCODE_NOTSUPPORTEDERROR;Should this be INVALID_STATE_ERROR?
File media/mojo/clients/mojo_cdm.cc:
Patch Set #1, Line 108:
promise->reject(CdmPromise::Exception::NOT_SUPPORTED_ERROR, 0,This should probably be INVALID_STATE_ERROR...
Patch Set #1, Line 135:
pending_init_promise_->reject(CdmPromise::Exception::NOT_SUPPORTED_ERROR, 0,ditto, INVALID_STATE_ERROR?
File media/remoting/proto_enum_utils.cc:
Patch Set #1, Line 484:
case OriginType::OUTPUT_ERROR:Can we just remove these? If not, add a comment about why.
To view, visit change 537592. To unsubscribe, visit settings.
John Rummell (Gerrit)
John Rummell posted comments on this change.
Patch set 2:
Updated.
(14 comments)
Patch Set #1, Line 29:
OUTPUT_ERROR = 7,Can we remove these now given we should never report them?
Based on the comment in the code, I assume we need to keep old values to ensure the UMA reports continue to work.
File media/blink/cdm_result_promise_helper.cc:
Patch Set #1, Line 25:
UNKNOWN_ERRORShall we switch to another error here so that we can remove UNKNOWN_ERROR?
Will do if the unused values can be removed.
File media/cdm/aes_decryptor.cc:
Patch Set #1, Line 316:
promise->reject(CdmPromise::Exception::TYPE_ERROR, 0,Should this be TYPE_ERROR?
Done
Patch Set #1, Line 341:
promise->reject(CdmPromise::Exception::TYPE_ERROR, 0, error_message);Should this be TYPE_ERROR?
Done
File media/cdm/cdm_adapter.cc:
Add a TODO to remove these after deprecating CDM_8.
Done
Is it possible for a CDM to use these values today?
Yes (except ClientError). Fixed.
Shall we use INVALID_STATE_ERROR here?
Done
Patch Set #1, Line 423:
cdm_.reset(CreateCdmInstance(key_system_, cdm_path));Should this be INVALID_STATE_ERROR?
Done
File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:
Patch Set #1, Line 149:
return cdm::kInvalidStateError;
ditto that we are moving kUnknownError so pick another one?
Done
File media/cdm/ppapi/ppapi_cdm_adapter.cc:
Patch Set #1, Line 269:
// TODO(jrummell): Remove these once CDM_8 is no longer supported.
It cannot be removed yet. Add a TODO?
Done
Should this be INVALID_STATE_ERROR?
Done
File media/mojo/clients/mojo_cdm.cc:
Patch Set #1, Line 108:
promise->reject(CdmPromise::Exception::INVALID_STATE_ERROR, 0,
This should probably be INVALID_STATE_ERROR...
Done
Patch Set #1, Line 135:
pending_init_promise_->reject(CdmPromise::Exception::INVALID_STATE_ERROR, 0,ditto, INVALID_STATE_ERROR?
Done
File media/remoting/proto_enum_utils.cc:
Patch Set #1, Line 484:
case OriginType::CLIENT_ERROR:
Can we just remove these? If not, add a comment about why.
Done. If I can change the RPC definition then I will remove them.
To view, visit change 537592. To unsubscribe, visit settings.
Xiaohan Wang (Gerrit)
Xiaohan Wang posted comments on this change.
Patch set 2:
+isherman and miu: Please see the questions about deprecated UMA/Protobug enums. Thanks!
(4 comments)
File media/blink/cdm_result_promise_helper.h:
Patch Set #2, Line 29:
OUTPUT_ERROR = 7,+isherman: These enums are never used so we are removing them. Can we just remove it from here and in histograms.xml?
File media/cdm/cdm_adapter.cc:
Patch Set #2, Line 80:
// TODO(jrummell): Remove these once CDM_8 is no longer supported.Can you file a bug about removing CDM_8 support and add link here?
File media/cdm/ppapi/ppapi_cdm_adapter.cc:
Patch Set #2, Line 269:
// TODO(jrummell): Remove these once CDM_8 is no longer supported.ditto
File media/remoting/proto_enum_utils.cc:
Patch Set #2, Line 485:
case OriginType::OUTPUT_ERROR:+miu: These enums are being deprecated. Do you know what's the proper way of removing them from the protobuf?
To view, visit change 537592. To unsubscribe, visit settings.
Ilya Sherman (Gerrit)
Ilya Sherman posted comments on this change.
Patch set 2:
(1 comment)
Patch Set #2, Line 29:
OUTPUT_ERROR = 7,+isherman: These enums are never used so we are removing them. Can we just
Please just mark the histogram as <obsolete>, unless there were literally no samples emitted to it, for any version of Chrome. If the histogram literally never got any data that was uploaded to the server, then it's ok to delete the metadata from the XML files. Otherwise, it's good to preserve that metadata, so that already-uploaded data is still viewable.
It's fine to delete this enum from the C++ code, though, if it is unused.
To view, visit change 537592. To unsubscribe, visit settings.
mark a. foltz (Gerrit)
mark a. foltz removed mfoltz...@chromium.org from this change.
To view, visit change 537592. To unsubscribe, visit settings.
Yuri Wiitala (Gerrit)
Yuri Wiitala posted comments on this change.
Patch set 2:
media/remoting lgtm % noting the "deprecated" values in the proto file:
(1 comment)
Patch Set #2, Line 485:
case OriginType::OUTPUT_ERROR:+miu: These enums are being deprecated. Do you know what's the proper way o
I think it'd be sufficient to add the deprecated attribute on the enum values in the media/remoting/rpc.proto file. Meaning:
enum CdmException {
...
INVALID_ACCESS_ERROR = 2 [deprecated=true];
...
UNKNOWN_ERROR = 4 [deprecated=true];
...and so on...
}The protobuf documentation only discusses using the "deprecated" attribute on type fields, so I'm not certain that the above will work. If you get a compile error, it would be sufficient to just add EOL comments instead, like:
enum CdmException {
...
INVALID_ACCESS_ERROR = 2; // DEPRECATED
...
UNKNOWN_ERROR = 4; // DEPRECATED
...and so on...
}
To view, visit change 537592. To unsubscribe, visit settings.
Xiaohan Wang (Gerrit)
Xiaohan Wang posted comments on this change.
Patch set 2:
(2 comments)
File media/blink/cdm_result_promise_helper.h:
Patch Set #2, Line 20:
// tools/metrics/histograms/histograms.xml.Please help update this, s/histograms.xml/enums.xml/
Patch Set #2, Line 29:
OUTPUT_ERROR = 7,
Please just mark the histogram as <obsolete>, unless there were literally n
Sorry I wasn't clear in the original question. We are not removing the UMA and all the enums. Instead, we are removing 3 enums in the C++ code and AFAICT these values were pretty much never used.
In this case, can we update enums.xml and remove the unused enums? Basically removing value 5, 6 and 7 from the list: https://cs.chromium.org/chromium/src/tools/metrics/histograms/enums.xml?rcl=98deed067c4ca1bcf28f34d9f784b576e5796c38&l=3919
To view, visit change 537592. To unsubscribe, visit settings.
Ilya Sherman (Gerrit)
Ilya Sherman posted comments on this change.
Patch set 2:
(1 comment)
File media/blink/cdm_result_promise_helper.h:
Patch Set #2, Line 29:
OUTPUT_ERROR = 7,
Sorry I wasn't clear in the original question. We are not removing the UMA
I see. In that case, there is not much need to change enums.xml; you can add " (deprecated)" as a suffix for the entries if you'd like to.
Note that removing the items from the middle of the list would cause issues: in data from older versions, bucket 3 would mean one thing; in newer versions, it would mean a different things. Because histogram data is persisted, it is important never to change the semantics of a given serialized value; otherwise deserialization becomes quite hard. (We could instead version the serialized data, and deserialize according to version, but we do not do that, as it's more complex.)
To view, visit change 537592. To unsubscribe, visit settings.
John Rummell (Gerrit)
John Rummell posted comments on this change.
Patch set 4:
Set Ready For Review
John Rummell (Gerrit)
PS#3 is the changes for the comments, PS#4 is a rebase to pick up the changes for ECK.
(5 comments)
File media/blink/cdm_result_promise_helper.h:
Patch Set #2, Line 20:
// tools/metrics/histograms/enums.xml.
Please help update this, s/histograms.xml/enums.xml/
Done
Patch Set #2, Line 29:
OUTPUT_ERROR = 7,
I see. In that case, there is not much need to change enums.xml; you can a
Added (deprecated) to the 3 values no longer used.
Also want to note that although the code used INVALID_ACCESS_ERROR, it was always reported to JavaScript as TYPE_ERROR. This is just renaming it to match what users see.
File media/cdm/cdm_adapter.cc:
Patch Set #2, Line 80:
// TODO(jrummell): Remove these once CDM_8 is no longer supported.Can you file a bug about removing CDM_8 support and add link here?
Patch Set #2, Line 269:
// TODO(jrummell): Remove these once CDM_8 is no longer supported.ditto
Patch Set #2, Line 485:
case OriginType::OUTPUT_ERROR:
I think it'd be sufficient to add the deprecated attribute on the enum valu
[deprecated=true] appears to work. Done.
To view, visit change 537592. To unsubscribe, visit settings.
Xiaohan Wang (Gerrit)
Xiaohan Wang posted comments on this change.
Patch set 4:
(1 comment)
File media/blink/cdm_result_promise_helper.h:
Patch Set #3, Line 29:
OUTPUT_ERROR = 7,Does it make sense to comment out line 27-29, so that they are removed from the code, but we also make sure people will not reuse those values in the future?
To view, visit change 537592. To unsubscribe, visit settings.
Yuri Wiitala (Gerrit)
Yuri Wiitala posted comments on this change.
Patch set 4:Code-Review +1
Ilya Sherman (Gerrit)
Ilya Sherman posted comments on this change.
Patch set 4:
(1 comment)
File tools/metrics/histograms/enums.xml:
Patch Set #4, Line 3927:
TypeErrorJust to confirm: Are you changing the semantics of this bucket, or just clarifying the label?
To view, visit change 537592. To unsubscribe, visit settings.
Ilya Sherman (Gerrit)
Ilya Sherman posted comments on this change.
Patch set 4:Code-Review +1
enums.xml lgtm, thanks.
(1 comment)
Patch Set #4, Line 3927:
TypeErrorJust to confirm: Are you changing the semantics of this bucket, or just cla
Nevermind, I see that you answered this in your previous message; it's just clarifying the value. C'est bon =)
To view, visit change 537592. To unsubscribe, visit settings.
John Rummell (Gerrit)
John Rummell posted comments on this change.
Patch set 5:Commit-Queue +1
Updated.
(1 comment)
File media/blink/cdm_result_promise_helper.h:
Patch Set #3, Line 29:
// OUTPUT_ERROR = 7,
Does it make sense to comment out line 27-29, so that they are removed from
Commented out 28-29. Left UNKNOWN_ERROR so it can be used if the CdmPromiseResult is not recognized.
To view, visit change 537592. To unsubscribe, visit settings.
Xiaohan Wang (Gerrit)
Xiaohan Wang posted comments on this change.
Patch set 6:
(4 comments)
File media/blink/cdm_result_promise_helper.h:
Patch Set #5, Line 27:
UNKNOWN_ERROR = 5,The previous UNKNOWN_ERROR should now be converted to INVALID_STATE_ERROR so this should really never happen now. Given the fact we don't report "invalid values for enums" in other UMA, I feel we should just remove this as well. WDYT?
Please add a comment that these values are deprecated.
Patch Set #5, Line 30:
NUM_RESULT_CODES = 8 // Must be last.This is never reported and you don't need to hardcode the value here.
File media/blink/cdm_result_promise_helper.cc:
Patch Set #5, Line 41:
return blink::kWebContentDecryptionModuleExceptionUnknownError;According to the spec, we should never return kUnknownError now:
https://w3c.github.io/encrypted-media/#exceptions
Use INVALID_STATE_ERROR instead and remove kWebContentDecryptionModuleExceptionUnknownError altogether?
To view, visit change 537592. To unsubscribe, visit settings.
Ilya Sherman (Gerrit)
Ilya Sherman posted comments on this change.
Patch set 6:
(1 comment)
File media/blink/cdm_result_promise_helper.h:
Patch Set #5, Line 30:
NUM_RESULT_CODES = 8 // Must be last.
This is never reported and you don't need to hardcode the value here.
So, technically, that's true. However, I do think that it's a bit helpful to hardcode, as a reminder for readers that the values 5-7 are reserved, and should not be reused. It's ok if you'd prefer not to explicitly hardcode this; just please make sure that it's really clear to anyone editing this enum in the future that the deprecated values must not be reused.
To view, visit change 537592. To unsubscribe, visit settings.
Xiaohan Wang (Gerrit)
Xiaohan Wang posted comments on this change.
Patch Set #5, Line 30:
NUM_RESULT_CODES = 8 // Must be last.
So, technically, that's true. However, I do think that it's a bit helpful
Agreed that deprecated values should never be reused (e.g. 6 and 7). But hardcoding NUM_RESULT_CODES here still seems unnecessary and wrong. Now the "number of results codes" is actually 6, not 8. Also, when people adding a value, they have to update 2 lines instead of one, including changing NUM_RESULT_CODES to be 9, while in the comment above we say "These values should never be changed"...
To view, visit change 537592. To unsubscribe, visit settings.
John Rummell (Gerrit)
John Rummell posted comments on this change.
Patch set 7:
Updated.
(4 comments)
File media/blink/cdm_result_promise_helper.h:
Patch Set #5, Line 27:
// UNKNOWN_ERROR = 5, // Deprecated.
The previous UNKNOWN_ERROR should now be converted to INVALID_STATE_ERROR s
Done
Please add a comment that these values are deprecated.
Done
Patch Set #5, Line 30:
NUM_RESULT_CODES // Must be last.
Agreed that deprecated values should never be reused (e.g. 6 and 7). But ha
Done
File media/blink/cdm_result_promise_helper.cc:
Patch Set #5, Line 41:
return blink::kWebContentDecryptionModuleExceptionInvalidStateError;
According to the spec, we should never return kUnknownError now:
Done. Will fix up the WebContentDecryptionModuleExceptions in another CL.
To view, visit change 537592. To unsubscribe, visit settings.
Xiaohan Wang (Gerrit)
Xiaohan Wang posted comments on this change.
Patch set 7:Code-Review +1
LGTM with a comment
(1 comment)
File media/blink/cdm_result_promise_helper.h:
Patch Set #7, Line 29:
// OUTPUT_ERROR = 7, // Deprecated.Please add a comment that deprecated values should never be reused.
To view, visit change 537592. To unsubscribe, visit settings.
John Rummell (Gerrit)
John Rummell posted comments on this change.
Patch set 8:Commit-Queue +1
Updated.
(1 comment)
Patch Set #7, Line 29:
// OUTPUT_ERROR = 7, // Deprecated.Please add a comment that deprecated values should never be reused.
Done
To view, visit change 537592. To unsubscribe, visit settings.
John Rummell (Gerrit)
John Rummell posted comments on this change.
Patch set 9:Commit-Queue +1
John Rummell (Gerrit)
John Rummell posted comments on this change.
Patch set 10:Commit-Queue +1
+sky@ for pepper changes
+dcheng for media/base/ipc/media_param_traits_macros.h
Scott Violet (Gerrit)
Scott Violet posted comments on this change.
Patch set 10:
sky->brettw (for pepper changes)
Daniel Cheng (Gerrit)
Daniel Cheng posted comments on this change.
Patch set 10:Code-Review +1
John Rummell (Gerrit)
John Rummell posted comments on this change.
Patch set 11:
ping brettw@ for pepper changes
Brett Wilson (Gerrit)
Over email John assured me that the Pepper interface is used only by a plugin that's statically shipped with Chrome, so there shouldn't be compat issues in changing this interface.
Patch set 12:Code-Review +1
To view, visit change 537592. To unsubscribe, or for help writing mail filters, visit settings.
John Rummell (Gerrit)
Patch set 12:Commit-Queue +2
Commit Bot (Gerrit)
Commit Bot merged this change.
Clean up the CDM exception codes
Currently there are only 4 EME exceptions reported to users (TypeError,
NotSupportedError, InvalidStateError, and QuotaExceededError). Internally the
code was using InvalidAccessError as TypeError, so rename it to match what
users see. The other 3 errors (UnknownError, ClientError, and OutputError)
are not used, and are removed.
This change also makes CdmPromise::Exception an enum class.
BUG=570216
TEST=EME browser_tests still pass
Change-Id: Ib0a3970651332866b10b233d36bd880d996660af
Reviewed-on: https://chromium-review.googlesource.com/537592
Reviewed-by: Brett Wilson <bre...@chromium.org>
Reviewed-by: Daniel Cheng <dch...@chromium.org>
Reviewed-by: Xiaohan Wang <xhw...@chromium.org>
Reviewed-by: Yuri Wiitala <m...@chromium.org>
Reviewed-by: Ilya Sherman <ishe...@chromium.org>
Commit-Queue: John Rummell <jrum...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494586}
---
M content/renderer/media/cdm/ppapi_decryptor.cc
M content/renderer/pepper/content_decryptor_delegate.cc
M media/base/android/media_drm_bridge.cc
M media/base/cdm_promise.h
M media/base/cdm_promise_adapter.cc
M media/base/content_decryption_module.cc
M media/base/ipc/media_param_traits_macros.h
M media/blink/cdm_result_promise_helper.cc
M media/blink/cdm_result_promise_helper.h
M media/blink/new_session_cdm_result_promise.cc
M media/cdm/aes_decryptor.cc
M media/cdm/aes_decryptor.h
M media/cdm/cdm_adapter.cc
M media/cdm/ppapi/external_clear_key/clear_key_cdm.cc
M media/cdm/ppapi/external_clear_key/clear_key_persistent_session_cdm.cc
M media/cdm/ppapi/ppapi_cdm_adapter.cc
M media/mojo/clients/mojo_cdm.cc
M media/mojo/clients/mojo_cdm_unittest.cc
M media/remoting/proto_enum_utils.cc
M media/remoting/proto_utils.cc
M media/remoting/rpc.proto
M ppapi/api/private/pp_content_decryptor.idl
M ppapi/c/private/pp_content_decryptor.h
M tools/metrics/histograms/enums.xml
24 files changed, 146 insertions(+), 162 deletions(-)