Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1050823009:
Handle all possible cdm::Status values before passing through Pepper (Closed)
|
Created:
5 years, 8 months ago by jrummell Modified:
5 years, 7 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, yusukes+watch_chromium.org, feature-media-reviews_chromium.org, binji+watch_chromium.org, tzik, jam, raymes+watch_chromium.org, eme-reviews_chromium.org, teravest+watch_chromium.org, darin-cc_chromium.org, bradnelson+warch_chromium.org, mkwst+moarreviews-renderer_chromium.org, piman+watch_chromium.org, ihf+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHandle all possible cdm::Status values before passing through Pepper
BUG=471322
TEST=EME tests pass
Committed: https://crrev.com/95e07abe0d0073060d169d4b3e6de8f63b68653c
Cr-Commit-Position: refs/heads/master@{#327181}
Patch Set 1 #
Total comments: 5
Patch Set 2 : don't pass through Pepper #
Total comments: 4
Patch Set 3 : Changes #Messages
Total messages: 18 (6 generated)
jrummell@chromium.org changed reviewers: + bbudge@chromium.org, sandersd@chromium.org
PTAL. Simple change to pass all possible values through Pepper.
lgtm
lgtm
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/1050823009/1
The CQ bit was unchecked by ddorwin@chromium.org
ddorwin@chromium.org changed reviewers: + ddorwin@chromium.org
https://codereview.chromium.org/1050823009/diff/1/media/cdm/ppapi/cdm_adapter.cc File media/cdm/ppapi/cdm_adapter.cc (right): https://codereview.chromium.org/1050823009/diff/1/media/cdm/ppapi/cdm_adapter... media/cdm/ppapi/cdm_adapter.cc:104: case cdm::kSessionError: This also appears to only be used by the Initialize* methods. If so, we should rename it. You'll need to consult with the CDM team to confirm this, but clear_key_cdm.cc also uses it there. https://codereview.chromium.org/1050823009/diff/1/media/cdm/ppapi/cdm_adapter... media/cdm/ppapi/cdm_adapter.cc:110: case cdm::kDeferredInitialization: This is only expected from the Initialize* methods where it is explicitly handled. No other function should return it, and there is no reason to define Pepper values for it. If we want to explicitly handle all values, that's fine, but we should just fall into the default case. https://codereview.chromium.org/1050823009/diff/1/ppapi/api/private/pp_conten... File ppapi/api/private/pp_content_decryptor.idl (right): https://codereview.chromium.org/1050823009/diff/1/ppapi/api/private/pp_conten... ppapi/api/private/pp_content_decryptor.idl:161: /** Session management error. */ I don't think we need these.
Removed the additional values from passing through Pepper and instead added a comment noting this. https://codereview.chromium.org/1050823009/diff/1/media/cdm/ppapi/cdm_adapter.cc File media/cdm/ppapi/cdm_adapter.cc (right): https://codereview.chromium.org/1050823009/diff/1/media/cdm/ppapi/cdm_adapter... media/cdm/ppapi/cdm_adapter.cc:104: case cdm::kSessionError: On 2015/04/23 00:33:42, ddorwin wrote: > This also appears to only be used by the Initialize* methods. If so, we should > rename it. You'll need to consult with the CDM team to confirm this, but > clear_key_cdm.cc also uses it there. Fixed. https://codereview.chromium.org/1050823009/diff/1/media/cdm/ppapi/cdm_adapter... media/cdm/ppapi/cdm_adapter.cc:110: case cdm::kDeferredInitialization: On 2015/04/23 00:33:42, ddorwin wrote: > This is only expected from the Initialize* methods where it is explicitly > handled. No other function should return it, and there is no reason to define > Pepper values for it. > > If we want to explicitly handle all values, that's fine, but we should just fall > into the default case. Fixed.
Update the description to reflect the current change. https://codereview.chromium.org/1050823009/diff/20001/media/cdm/ppapi/cdm_ada... File media/cdm/ppapi/cdm_adapter.cc (right): https://codereview.chromium.org/1050823009/diff/20001/media/cdm/ppapi/cdm_ada... media/cdm/ppapi/cdm_adapter.cc:111: // Initialize* methods internally and never returned. DecodeAndDecrypt* Deliver* should never use.... https://codereview.chromium.org/1050823009/diff/20001/media/cdm/ppapi/cdm_ada... media/cdm/ppapi/cdm_adapter.cc:117: return PP_DECRYPTRESULT_DECODE_ERROR; DECRYPT? This call can result from just Decrypt() too.
Updated description (and code). https://codereview.chromium.org/1050823009/diff/20001/media/cdm/ppapi/cdm_ada... File media/cdm/ppapi/cdm_adapter.cc (right): https://codereview.chromium.org/1050823009/diff/20001/media/cdm/ppapi/cdm_ada... media/cdm/ppapi/cdm_adapter.cc:111: // Initialize* methods internally and never returned. DecodeAndDecrypt* On 2015/04/27 22:43:24, ddorwin wrote: > Deliver* should never use.... Done. https://codereview.chromium.org/1050823009/diff/20001/media/cdm/ppapi/cdm_ada... media/cdm/ppapi/cdm_adapter.cc:117: return PP_DECRYPTRESULT_DECODE_ERROR; On 2015/04/27 22:43:24, ddorwin wrote: > DECRYPT? This call can result from just Decrypt() too. This is what was in the original, and content_decryptor_delegate (receiving end) converts them into the same error. Changed.
lgtm
The CQ bit was checked by jrummell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org, sandersd@chromium.org Link to the patchset: https://codereview.chromium.org/1050823009/#ps40001 (title: "Changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1050823009/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/95e07abe0d0073060d169d4b3e6de8f63b68653c Cr-Commit-Position: refs/heads/master@{#327181} |
