Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2791903004:
media: Implement MediaDrmStorageImpl with tests (Closed)
Descriptionmedia: Implement MediaDrmStorageImpl with tests
Implement MediaDrmStorageImpl using PrefService.
TBR=sdefresne@chromium.org
BUG=493521
TEST=New unit tests added.
Review-Url: https://codereview.chromium.org/2791903004
Cr-Commit-Position: refs/heads/master@{#462929}
Committed: https://chromium.googlesource.com/chromium/src/+/d0b7b3b7e9f7d4e4c1f8276c5b416eaa05ac9bb0
Patch Set 1 #Patch Set 2 : fix rebase errors #
Total comments: 10
Patch Set 3 : comments #
Total comments: 2
Patch Set 4 : comments #
Messages
Total messages: 37 (24 generated)
The CQ bit was checked by xhwang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by xhwang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by xhwang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
xhwang@chromium.org changed reviewers: + yucliu@chromium.org
PTAL
Some comments on the failure branches. I think we should treat some failure cases as successful cases. What do you think? https://codereview.chromium.org/2791903004/diff/20001/components/cdm/browser/... File components/cdm/browser/media_drm_storage_impl.cc (right): https://codereview.chromium.org/2791903004/diff/20001/components/cdm/browser/... components/cdm/browser/media_drm_storage_impl.cc:139: callback.Run(false); If this happens, all information associated with the old storage will become invalid, because new device provision already happens in MediaDrm level. Should we allow this case? If there're mis-sync between our storage and MediaDrm storage, this will happen, e.g. deleting license on rooted device but don't touch our storage file. https://codereview.chromium.org/2791903004/diff/20001/components/cdm/browser/... components/cdm/browser/media_drm_storage_impl.cc:185: callback.Run(false); I think offline licenses can be updated, in which case we will hit this. https://codereview.chromium.org/2791903004/diff/20001/components/cdm/browser/... components/cdm/browser/media_drm_storage_impl.cc:241: callback.Run(nullptr); MediaDrmBridge level has logic to clear storage when failure happens. But is it to also add the logic here? https://codereview.chromium.org/2791903004/diff/20001/components/cdm/browser/... components/cdm/browser/media_drm_storage_impl.cc:281: callback.Run(sessions_dict->RemoveWithoutPathExpansion(session_id, nullptr)); If session_id doesn't exist, this should be treated as removed. Probably we want to return true here. https://codereview.chromium.org/2791903004/diff/20001/media/base/android/medi... File media/base/android/media_drm_storage.h (right): https://codereview.chromium.org/2791903004/diff/20001/media/base/android/medi... media/base/android/media_drm_storage.h:72: // the result will be true. Otherwise, the result will be false. If session_id doesn't exist, this should be true, because the result is the same as 'removed'
The CQ bit was checked by xhwang@chromium.org to run a CQ dry run
yucliu1: PTAL again! https://codereview.chromium.org/2791903004/diff/20001/components/cdm/browser/... File components/cdm/browser/media_drm_storage_impl.cc (right): https://codereview.chromium.org/2791903004/diff/20001/components/cdm/browser/... components/cdm/browser/media_drm_storage_impl.cc:139: callback.Run(false); On 2017/04/04 06:50:00, yucliu1 wrote: > If this happens, all information associated with the old storage will become > invalid, because new device provision already happens in MediaDrm level. Should > we allow this case? > > If there're mis-sync between our storage and MediaDrm storage, this will happen, > e.g. deleting license on rooted device but don't touch our storage file. Updated the implementation to clear the old dictionary. https://codereview.chromium.org/2791903004/diff/20001/components/cdm/browser/... components/cdm/browser/media_drm_storage_impl.cc:185: callback.Run(false); On 2017/04/04 06:50:00, yucliu1 wrote: > I think offline licenses can be updated, in which case we will hit this. Good point. Done. https://codereview.chromium.org/2791903004/diff/20001/components/cdm/browser/... components/cdm/browser/media_drm_storage_impl.cc:241: callback.Run(nullptr); On 2017/04/04 06:50:00, yucliu1 wrote: > MediaDrmBridge level has logic to clear storage when failure happens. But is it > to also add the logic here? That should never happen unless user manually modifies the file. I don't feel we should complicate our logic for those cases. After all, use will have a way to clear all the data ("clear media licenses") to start from scratch. https://codereview.chromium.org/2791903004/diff/20001/components/cdm/browser/... components/cdm/browser/media_drm_storage_impl.cc:281: callback.Run(sessions_dict->RemoveWithoutPathExpansion(session_id, nullptr)); On 2017/04/04 06:50:00, yucliu1 wrote: > If session_id doesn't exist, this should be treated as removed. Probably we want > to return true here. Done. https://codereview.chromium.org/2791903004/diff/20001/media/base/android/medi... File media/base/android/media_drm_storage.h (right): https://codereview.chromium.org/2791903004/diff/20001/media/base/android/medi... media/base/android/media_drm_storage.h:72: // the result will be true. Otherwise, the result will be false. On 2017/04/04 06:50:00, yucliu1 wrote: > If session_id doesn't exist, this should be true, because the result is the same > as 'removed' Done. No matter what the result is, it seems there's nothing the caller can do.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2791903004/diff/40001/components/cdm/browser/... File components/cdm/browser/media_drm_storage_impl.cc (right): https://codereview.chromium.org/2791903004/diff/40001/components/cdm/browser/... components/cdm/browser/media_drm_storage_impl.cc:78: bool HasEntry(const base::DictionaryValue& dict, const std::string& key) { The function is only called in DVLOG_IF, will this cause lint error for production build?
The CQ bit was checked by xhwang@chromium.org to run a CQ dry run
yucliu1: PTAL again https://codereview.chromium.org/2791903004/diff/40001/components/cdm/browser/... File components/cdm/browser/media_drm_storage_impl.cc (right): https://codereview.chromium.org/2791903004/diff/40001/components/cdm/browser/... components/cdm/browser/media_drm_storage_impl.cc:78: bool HasEntry(const base::DictionaryValue& dict, const std::string& key) { On 2017/04/06 22:13:03, yucliu1 wrote: > The function is only called in DVLOG_IF, will this cause lint error for > production build? Good point. Done.
yucliu1: PTAL again https://codereview.chromium.org/2791903004/diff/40001/components/cdm/browser/... File components/cdm/browser/media_drm_storage_impl.cc (right): https://codereview.chromium.org/2791903004/diff/40001/components/cdm/browser/... components/cdm/browser/media_drm_storage_impl.cc:78: bool HasEntry(const base::DictionaryValue& dict, const std::string& key) { On 2017/04/06 22:13:03, yucliu1 wrote: > The function is only called in DVLOG_IF, will this cause lint error for > production build? Good point. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
xhwang@chromium.org changed reviewers: + sdefresne@chromium.org
sdefresne: Please OWNERS review the change in components/BUILD.gn for adding a test file.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== media: Implement MediaDrmStorageImpl with tests Implement MediaDrmStorageImpl using PrefService. BUG=493521 TEST=New unit tests added. ========== to ========== media: Implement MediaDrmStorageImpl with tests Implement MediaDrmStorageImpl using PrefService. TBR=sdefresne@chromium.org BUG=493521 TEST=New unit tests added. ==========
I didn't realize sdefresne@ is OOO. TBRing sdefresne@ for the trivial change of adding a test file in components/BUILD.gn.
The CQ bit was checked by xhwang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491588054784900,
"parent_rev": "cf4aa356378e931692ed10abbe78d2fddbca60b2", "commit_rev":
"d0b7b3b7e9f7d4e4c1f8276c5b416eaa05ac9bb0"}
Message was sent while issue was closed.
Description was changed from ========== media: Implement MediaDrmStorageImpl with tests Implement MediaDrmStorageImpl using PrefService. TBR=sdefresne@chromium.org BUG=493521 TEST=New unit tests added. ========== to ========== media: Implement MediaDrmStorageImpl with tests Implement MediaDrmStorageImpl using PrefService. TBR=sdefresne@chromium.org BUG=493521 TEST=New unit tests added. Review-Url: https://codereview.chromium.org/2791903004 Cr-Commit-Position: refs/heads/master@{#462929} Committed: https://chromium.googlesource.com/chromium/src/+/d0b7b3b7e9f7d4e4c1f8276c5b41... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/d0b7b3b7e9f7d4e4c1f8276c5b41...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2808563002/ by hanxi@chromium.org. The reason for reverting is: This CL causes MediaDrmStorageImplTest failing on chromium.linux/Android Tests: https://uberchromegw.corp.google.com/i/chromium.linux/builders/Android%20Test... BUG=709601.
Message was sent while issue was closed.
components/BUILD.gn lgtm |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
