Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(192)

Issue 1983563002: media: Add helper function to register pepper CDMs (Closed)

Created:
4 years, 7 months ago by xhwang
Modified:
4 years, 7 months ago
Reviewers:
Lei Zhang, Nico, ddorwin
CC:
chromium-reviews, mcasas+watch+vc_chromium.org, feature-media-reviews_chromium.org, markusheintz_, msramek+watch_chromium.org, raymes+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

media: Add helper function to register pepper CDMs This removes duplicate pepper CDM registration code. BUG=610581, 582622 TEST=No functionality change. Committed: https://crrev.com/eb61e1d9dde1d6c02919c3716a2fd6729267cbac Cr-Commit-Position: refs/heads/master@{#394332}

Patch Set 1 #

Patch Set 2 : more cleanup #

Total comments: 13

Patch Set 3 : comments #

Total comments: 6

Patch Set 4 : comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+136 lines, -148 lines) Patch
M chrome/browser/content_settings/content_settings_browsertest.cc View 1 2 5 chunks +10 lines, -54 lines 0 comments Download
M chrome/browser/media/encrypted_media_browsertest.cc View 1 2 5 chunks +8 lines, -41 lines 0 comments Download
M chrome/browser/media/encrypted_media_supported_types_browsertest.cc View 1 2 8 chunks +14 lines, -53 lines 0 comments Download
A chrome/browser/media/pepper_cdm_test_helper.h View 1 2 3 1 chunk +37 lines, -0 lines 0 comments Download
A chrome/browser/media/pepper_cdm_test_helper.cc View 1 2 3 1 chunk +58 lines, -0 lines 0 comments Download
M chrome/chrome_tests.gypi View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/test/BUILD.gn View 1 2 1 chunk +5 lines, -0 lines 0 comments Download

Messages

Total messages: 21 (7 generated)
xhwang
A cleanup CL. PTAL
4 years, 7 months ago (2016-05-14 07:35:37 UTC) #2
ddorwin
LGTM with commments https://chromiumcodereview.appspot.com/1983563002/diff/20001/chrome/browser/media/pepper_cdm_test_helper.cc File chrome/browser/media/pepper_cdm_test_helper.cc (right): https://chromiumcodereview.appspot.com/1983563002/diff/20001/chrome/browser/media/pepper_cdm_test_helper.cc#newcode15 chrome/browser/media/pepper_cdm_test_helper.cc:15: #if defined(ENABLE_PEPPER_CDMS) Do we even build ...
4 years, 7 months ago (2016-05-16 21:37:55 UTC) #3
xhwang
https://chromiumcodereview.appspot.com/1983563002/diff/20001/chrome/browser/media/pepper_cdm_test_helper.cc File chrome/browser/media/pepper_cdm_test_helper.cc (right): https://chromiumcodereview.appspot.com/1983563002/diff/20001/chrome/browser/media/pepper_cdm_test_helper.cc#newcode15 chrome/browser/media/pepper_cdm_test_helper.cc:15: #if defined(ENABLE_PEPPER_CDMS) On 2016/05/16 21:37:55, ddorwin wrote: > Do ...
4 years, 7 months ago (2016-05-18 00:38:29 UTC) #4
xhwang
thestig: Please OWNERS review chrome/. This is a small cleanup change.
4 years, 7 months ago (2016-05-18 00:39:13 UTC) #6
Lei Zhang
https://chromiumcodereview.appspot.com/1983563002/diff/40001/chrome/browser/media/pepper_cdm_test_helper.cc File chrome/browser/media/pepper_cdm_test_helper.cc (right): https://chromiumcodereview.appspot.com/1983563002/diff/40001/chrome/browser/media/pepper_cdm_test_helper.cc#newcode31 chrome/browser/media/pepper_cdm_test_helper.cc:31: DCHECK(PathService::Get(base::DIR_MODULE, &adapter_path)); Can't put this in a DCHECK. It ...
4 years, 7 months ago (2016-05-18 00:53:11 UTC) #7
xhwang
Thanks for the review. Please take a look again. https://chromiumcodereview.appspot.com/1983563002/diff/40001/chrome/browser/media/pepper_cdm_test_helper.cc File chrome/browser/media/pepper_cdm_test_helper.cc (right): https://chromiumcodereview.appspot.com/1983563002/diff/40001/chrome/browser/media/pepper_cdm_test_helper.cc#newcode31 chrome/browser/media/pepper_cdm_test_helper.cc:31: ...
4 years, 7 months ago (2016-05-18 02:08:10 UTC) #8
Lei Zhang
lgtm
4 years, 7 months ago (2016-05-18 02:22:47 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1983563002/50008 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1983563002/50008
4 years, 7 months ago (2016-05-18 02:27:04 UTC) #13
commit-bot: I haz the power
Committed patchset #4 (id:50008)
4 years, 7 months ago (2016-05-18 04:18:07 UTC) #14
commit-bot: I haz the power
Patchset 4 (id:??) landed as https://crrev.com/eb61e1d9dde1d6c02919c3716a2fd6729267cbac Cr-Commit-Position: refs/heads/master@{#394332}
4 years, 7 months ago (2016-05-18 04:19:40 UTC) #16
Nico
Looks like this make tests fail in official builds: https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%20tester/builds/4758 (that bot also uses clang, ...
4 years, 7 months ago (2016-05-18 14:27:22 UTC) #18
xhwang
On 2016/05/18 14:27:22, Nico (vacation May 19 to 22) wrote: > Looks like this make ...
4 years, 7 months ago (2016-05-18 16:02:28 UTC) #19
xhwang
On 2016/05/18 16:02:28, xhwang wrote: > On 2016/05/18 14:27:22, Nico (vacation May 19 to 22) ...
4 years, 7 months ago (2016-05-18 16:34:11 UTC) #20
xhwang
4 years, 7 months ago (2016-05-18 16:34:59 UTC) #21
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:50008) has been created in
https://chromiumcodereview.appspot.com/1993813002/ by xhwang@chromium.org.

The reason for reverting is: This make tests fail in official builds:

https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%20tester/build....

Powered by Google App Engine
This is Rietveld 408576698