Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2617023004:
media: Copy Widevine CDM adapter signature file (Closed)
Descriptionmedia: Copy Widevine CDM adapter signature file
In Widevine CDM component updater, when we copy the adapter, we should
also copy the adapter signature file.
BUG=660287
TEST=Manually tested
Review-Url: https://codereview.chromium.org/2617023004
Cr-Commit-Position: refs/heads/master@{#442721}
Committed: https://chromium.googlesource.com/chromium/src/+/04848c804fa0df48dcd79a7dfb60ac1d227a1eb9
Patch Set 1 #Patch Set 2 : media: Copy Widevine CDM adapter signature file #
Total comments: 4
Patch Set 3 : media: Copy Widevine CDM adapter signature file #Messages
Total messages: 24 (16 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 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...
xhwang@chromium.org changed reviewers: + ddorwin@chromium.org, jrummell@chromium.org
xhwang@chromium.org changed required reviewers: + jrummell@chromium.org
I still need to test this manually. But it's a fairly simple change so PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Comments below. https://codereview.chromium.org/2617023004/diff/20001/chrome/browser/componen... File chrome/browser/component_updater/widevine_cdm_component_installer.cc (right): https://codereview.chromium.org/2617023004/diff/20001/chrome/browser/componen... chrome/browser/component_updater/widevine_cdm_component_installer.cc:400: // Copy Widevine CDM signature file if it exists. Since failing to find and/or copy this is not fatal, I think it should be after the copy of the adapter file. That way if the output directory is not writable, disk full, etc. it won't attempt 2 copy operations. https://codereview.chromium.org/2617023004/diff/20001/chrome/browser/componen... chrome/browser/component_updater/widevine_cdm_component_installer.cc:409: PLOG(WARNING) << "Widevine CDM adapter signature file does not exist."; Since there is a log if the file doesn't exist or can't be copied, why not simply call base::CopyFile( adapter_source_path.AddExtension(kSignatureFileExtension), adapter_install_path.AddExtension(kSignatureFileExtension))? Does anybody see these logging messages other than for testing?
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...
https://codereview.chromium.org/2617023004/diff/20001/chrome/browser/componen... File chrome/browser/component_updater/widevine_cdm_component_installer.cc (right): https://codereview.chromium.org/2617023004/diff/20001/chrome/browser/componen... chrome/browser/component_updater/widevine_cdm_component_installer.cc:400: // Copy Widevine CDM signature file if it exists. On 2017/01/06 02:04:00, jrummell wrote: > Since failing to find and/or copy this is not fatal, I think it should be after > the copy of the adapter file. That way if the output directory is not writable, > disk full, etc. it won't attempt 2 copy operations. Good point. Done. https://codereview.chromium.org/2617023004/diff/20001/chrome/browser/componen... chrome/browser/component_updater/widevine_cdm_component_installer.cc:409: PLOG(WARNING) << "Widevine CDM adapter signature file does not exist."; On 2017/01/06 02:04:00, jrummell wrote: > Since there is a log if the file doesn't exist or can't be copied, why not > simply call base::CopyFile( > adapter_source_path.AddExtension(kSignatureFileExtension), > adapter_install_path.AddExtension(kSignatureFileExtension))? Does anybody see > these logging messages other than for testing? We did have cases where the CDM is missing and the user can provide us these logs to help investigation. That's why we added these PLOGs. But I guess since this isn't a fatal error (we ignore the failure case) the additional log isn't very useful. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
xhwang@chromium.org changed reviewers: + sorin@chromium.org
I manually tested this CL with and without the CDM adapter signature file and it's working as expected. sorin: Please OWNERS review.
lgtm thank you!
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": 40001, "attempt_start_ts": 1484087487080450,
"parent_rev": "fa1d37f66db6b6337589a30177cfa5915600897b", "commit_rev":
"04848c804fa0df48dcd79a7dfb60ac1d227a1eb9"}
Message was sent while issue was closed.
Description was changed from ========== media: Copy Widevine CDM adapter signature file In Widevine CDM component updater, when we copy the adapter, we should also copy the adapter signature file. BUG=660287 TEST=Manually tested ========== to ========== media: Copy Widevine CDM adapter signature file In Widevine CDM component updater, when we copy the adapter, we should also copy the adapter signature file. BUG=660287 TEST=Manually tested Review-Url: https://codereview.chromium.org/2617023004 Cr-Commit-Position: refs/heads/master@{#442721} Committed: https://chromium.googlesource.com/chromium/src/+/04848c804fa0df48dcd79a7dfb60... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/04848c804fa0df48dcd79a7dfb60... |
