Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1637083002:
Add CdmService to keep track of CDMs (Closed)
|
Created:
4 years, 11 months ago by jrummell Modified:
4 years, 8 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, feature-media-reviews_chromium.org, waffles Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd CdmService to keep track of CDMs
As media moves to a mojo world, CDMs will be loaded directly into
the appropriate process, rather than loaded as pepper plugins.
This change adds a class to keep track of the available CDMs.
On Win/Mac, the Widevine CDM is updated by the component updater,
so it registers (or re-registers) the CDM as it becomes available.
On Linux it will be registered if the library is available.
Future work:
- preload the libraries into whatever process will be used to host
the CDM.
- keep track of the CDMs that are loaded, so that they are only
initialized once.
- allow use of External Clear Key CDM for testing. It will be
registered if requested using a command line option.
BUG=550664
TEST=new content_unittests pass
Committed: https://crrev.com/c82f868e10e87ec7394ea7c0663bfbf679677d8d
Cr-Commit-Position: refs/heads/master@{#389036}
Patch Set 1 #
Total comments: 32
Patch Set 2 : partial changes #Patch Set 3 : rebase #Patch Set 4 : ContentDecryptionModule #
Total comments: 1
Patch Set 5 : rename + simplification #
Total comments: 80
Patch Set 6 : changes + file move #
Total comments: 23
Patch Set 7 : comments #Patch Set 8 : changes #
Total comments: 27
Patch Set 9 : more changes #
Total comments: 6
Patch Set 10 : test changes #Patch Set 11 : rebase #
Total comments: 24
Patch Set 12 : changes #
Total comments: 7
Patch Set 13 : remove const #
Total comments: 8
Patch Set 14 : changes #Patch Set 15 : rebase + add default copy constructor #Messages
Total messages: 54 (14 generated)
jrummell@chromium.org changed reviewers: + xhwang@chromium.org
PTAL. This is just the registration of key systems, independent of the pepper plugins.
Can we transition entirely from one registration API to another rather than maintaining two (and registering twice)? It seems the platform-specific implementation details (of Chrome) should be abstracted by Mojo. https://codereview.chromium.org/1637083002/diff/1/chrome/common/chrome_conten... File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/1637083002/diff/1/chrome/common/chrome_conten... chrome/common/chrome_content_client.cc:560: #if defined(WIDEVINE_CDM_AVAILABLE) && !defined(WIDEVINE_CDM_IS_COMPONENT) This code will run on Android. Yes, we want to register all at once, but the how is different. Ideally, this would be handled by the platform-specific Mojo service code. Should Chrome need to know how the Mojo CDM service is implemented? https://codereview.chromium.org/1637083002/diff/1/chrome/common/chrome_switch... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/1637083002/diff/1/chrome/common/chrome_switch... chrome/common/chrome_switches.cc:417: const char kEnableExternalClearKey[] = "enable-external-clear-key"; We might want to add add "-cdm" or "-key-system" since this is not self-explanatory. WDYT? https://codereview.chromium.org/1637083002/diff/1/chrome/common/chrome_switch... chrome/common/chrome_switches.cc:417: const char kEnableExternalClearKey[] = "enable-external-clear-key"; Should we use a more general name for this capability since it could also be implemented on Android and other platforms? Also to clearly indicate that this is for testing. Perhaps --enable-test-key-systems https://codereview.chromium.org/1637083002/diff/1/content/public/common/conte... File content/public/common/content_client.h (right): https://codereview.chromium.org/1637083002/diff/1/content/public/common/conte... content/public/common/content_client.h:91: virtual void AddKeySystems( Can we remove the ContentRendererClient method? What is the plan there?
Thanks! I only review the new interfaces/classes and didn't review the test and registration calls. Here are my first round of comments. https://chromiumcodereview.appspot.com/1637083002/diff/1/chrome/common/chrome... File chrome/common/chrome_content_client.cc (right): https://chromiumcodereview.appspot.com/1637083002/diff/1/chrome/common/chrome... chrome/common/chrome_content_client.cc:125: static bool IsWidevineAvailable(base::FilePath* cdm_adapter_path, Probably we should move this function and the ECK registration code into a different file. This file just have too many unrelated stuff... https://chromiumcodereview.appspot.com/1637083002/diff/1/chrome/common/chrome... chrome/common/chrome_content_client.cc:575: // External Clear Key is used for testing the interface to key systems. This CL is getting large. It seems we can split the registration of External Clear Key (with command line) into a separate CL. WDYT? https://chromiumcodereview.appspot.com/1637083002/diff/1/content/common/media... File content/common/media/key_system_service.cc (right): https://chromiumcodereview.appspot.com/1637083002/diff/1/content/common/media... content/common/media/key_system_service.cc:20: GetContentClient()->AddKeySystems(®istered_key_systems_); See comment below. https://chromiumcodereview.appspot.com/1637083002/diff/1/content/public/commo... File content/public/common/content_client.h (right): https://chromiumcodereview.appspot.com/1637083002/diff/1/content/public/commo... content/public/common/content_client.h:91: virtual void AddKeySystems( On 2016/01/26 20:21:53, ddorwin wrote: > Can we remove the ContentRendererClient method? What is the plan there? There's no plan to change how the key system info is populated in the render process (chrome -> ContentRendererClient -> ContentMediaClient -> media::KeySystems), other than we'll remove the pepper_type after we switch to mojo. https://chromiumcodereview.appspot.com/1637083002/diff/1/content/public/commo... content/public/common/content_client.h:92: std::vector<content::KeySystemRegistration>* key_systems) {} As chatted offline, on Linux we'll actually need to preload the CDM in zygote_main_linux.cc, the same as how we preload pepper plugins. When we do that, we'll have to check whether the CDM library exists. Since we already need to do this, probably we should just go ahead and call KeySystemService::Register() before preloading. Then PreloadCdm() can just load all registered CDMs. With that, we probably don't need this AddKeySystems() call anymore. https://chromiumcodereview.appspot.com/1637083002/diff/1/content/public/commo... File content/public/common/key_system_registration.h (right): https://chromiumcodereview.appspot.com/1637083002/diff/1/content/public/commo... content/public/common/key_system_registration.h:19: std::string key_system_; No need to use trailing "_" for struct members. https://chromiumcodereview.appspot.com/1637083002/diff/1/content/public/commo... content/public/common/key_system_registration.h:23: base::FilePath path_; Can we use a more specific name? Key System doesn't really have a path. It's the underlying cdm::CDM that has a path. How about something like cdm_path, or cdm_library_path? https://chromiumcodereview.appspot.com/1637083002/diff/1/content/public/commo... content/public/common/key_system_registration.h:26: base::string16 codecs_; Can we use std::vector<base::string16> for the "list" of codecs? https://chromiumcodereview.appspot.com/1637083002/diff/1/content/public/commo... File content/public/common/key_system_service.h (right): https://chromiumcodereview.appspot.com/1637083002/diff/1/content/public/commo... content/public/common/key_system_service.h:5: #ifndef CONTENT_PUBLIC_COMMON_KEY_SYSTEM_SERVICE_H_ Is there any reason we do't put this in content/public/browser/? From our offline chat, it seems we won't use this class in the utility process, so this will only be used by browser/ code. https://chromiumcodereview.appspot.com/1637083002/diff/1/content/public/commo... content/public/common/key_system_service.h:26: // Register |key_system|, replacing any entry that may exist for it. |path| Register_s_; same below. https://chromiumcodereview.appspot.com/1637083002/diff/1/content/public/commo... content/public/common/key_system_service.h:31: const base::string16& codecs); How should the list of codecs be formatted in this string? https://chromiumcodereview.appspot.com/1637083002/diff/1/content/public/commo... content/public/common/key_system_service.h:36: bool Path(const std::string& key_system, base::FilePath* path); Path() is a bit vague. How about GetCdmLibraryPath()? https://chromiumcodereview.appspot.com/1637083002/diff/1/content/public/commo... content/public/common/key_system_service.h:39: // registered, true is returned and |codecs| is updated. Otherwise false Is it possible "true" is returned and "codecs" is still empty? https://chromiumcodereview.appspot.com/1637083002/diff/1/content/public/commo... content/public/common/key_system_service.h:41: bool Codecs(const std::string& key_system, base::string16* codecs); ditto about name too general. How about GetSupportedCodecs? https://chromiumcodereview.appspot.com/1637083002/diff/1/content/public/commo... content/public/common/key_system_service.h:47: std::vector<base::FilePath>* paths); What's the purpose of this API? It's a bit odd that we return |key_systems| and |paths| separately.
Description was changed from ========== Add key_system_service to keep track of CDMs As media moves to a mojo world, CDMs will be loaded directly into the appropriate process, rather than loaded as pepper plugins. This change adds a class to keep track of the available CDMs. On Win/Mac, the Widevine CDM is updated by the component updater, so it registers (or re-registers) the CDM as it becomes available. On Linux it will be registered if the library is available. For testing, there is the External Clear Key CDM. It will be registered if requested using a command line option. Future work will preload the libraries into whatever process will be used to host the CDM before that process is sandboxed. It will also need to keep track of the CDMs that are loaded, so that they are only initialized once. BUG=550664 TEST=new content_unittests pass ========== to ========== Add key_system_service to keep track of CDMs As media moves to a mojo world, CDMs will be loaded directly into the appropriate process, rather than loaded as pepper plugins. This change adds a class to keep track of the available CDMs. On Win/Mac, the Widevine CDM is updated by the component updater, so it registers (or re-registers) the CDM as it becomes available. On Linux it will be registered if the library is available. On Linux any registered CDMs will be preloaded before the process is sandboxed. Future work: - preload the libraries on Win/Mac into whatever process will be used to host the CDM. - keep track of the CDMs that are loaded, so that they are only initialized once. - allow use of External Clear Key CDM for testing. It will be registered if requested using a command line option. BUG=550664 TEST=new content_unittests pass ==========
Description was changed from ========== Add key_system_service to keep track of CDMs As media moves to a mojo world, CDMs will be loaded directly into the appropriate process, rather than loaded as pepper plugins. This change adds a class to keep track of the available CDMs. On Win/Mac, the Widevine CDM is updated by the component updater, so it registers (or re-registers) the CDM as it becomes available. On Linux it will be registered if the library is available. On Linux any registered CDMs will be preloaded before the process is sandboxed. Future work: - preload the libraries on Win/Mac into whatever process will be used to host the CDM. - keep track of the CDMs that are loaded, so that they are only initialized once. - allow use of External Clear Key CDM for testing. It will be registered if requested using a command line option. BUG=550664 TEST=new content_unittests pass ========== to ========== Add content_decryption_module_service to keep track of CDMs As media moves to a mojo world, CDMs will be loaded directly into the appropriate process, rather than loaded as pepper plugins. This change adds a class to keep track of the available CDMs. On Win/Mac, the Widevine CDM is updated by the component updater, so it registers (or re-registers) the CDM as it becomes available. On Linux it will be registered if the library is available. On Linux any registered CDMs will be preloaded before the process is sandboxed. Future work: - preload the libraries on Win/Mac into whatever process will be used to host the CDM. - keep track of the CDMs that are loaded, so that they are only initialized once. - allow use of External Clear Key CDM for testing. It will be registered if requested using a command line option. BUG=550664 TEST=new content_unittests pass ==========
Updated. Renamed to ContentDecryptionModuleRegistration, etc. so the previous PS aren't that useful. ECK removed, for a later CL. https://codereview.chromium.org/1637083002/diff/1/chrome/common/chrome_conten... File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/1637083002/diff/1/chrome/common/chrome_conten... chrome/common/chrome_content_client.cc:575: // External Clear Key is used for testing the interface to key systems. On 2016/01/29 07:43:06, xhwang wrote: > This CL is getting large. It seems we can split the registration of External > Clear Key (with command line) into a separate CL. WDYT? Done. https://codereview.chromium.org/1637083002/diff/1/chrome/common/chrome_switch... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/1637083002/diff/1/chrome/common/chrome_switch... chrome/common/chrome_switches.cc:417: const char kEnableExternalClearKey[] = "enable-external-clear-key"; On 2016/01/26 20:21:53, ddorwin wrote: > Should we use a more general name for this capability since it could also be > implemented on Android and other platforms? Also to clearly indicate that this > is for testing. Perhaps --enable-test-key-systems I like the generic term. Done. https://codereview.chromium.org/1637083002/diff/1/content/common/media/key_sy... File content/common/media/key_system_service.cc (right): https://codereview.chromium.org/1637083002/diff/1/content/common/media/key_sy... content/common/media/key_system_service.cc:20: GetContentClient()->AddKeySystems(®istered_key_systems_); On 2016/01/29 07:43:06, xhwang wrote: > See comment below. Acknowledged. https://codereview.chromium.org/1637083002/diff/1/content/public/common/conte... File content/public/common/content_client.h (right): https://codereview.chromium.org/1637083002/diff/1/content/public/common/conte... content/public/common/content_client.h:92: std::vector<content::KeySystemRegistration>* key_systems) {} On 2016/01/29 07:43:06, xhwang wrote: > As chatted offline, on Linux we'll actually need to preload the CDM in > zygote_main_linux.cc, the same as how we preload pepper plugins. When we do > that, we'll have to check whether the CDM library exists. Since we already need > to do this, probably we should just go ahead and call > KeySystemService::Register() before preloading. Then PreloadCdm() can just load > all registered CDMs. > > With that, we probably don't need this AddKeySystems() call anymore. I looked at zygote_main_linux.cc, and actually updated it to preload the CDMs. https://codereview.chromium.org/1637083002/diff/1/content/public/common/key_s... File content/public/common/key_system_registration.h (right): https://codereview.chromium.org/1637083002/diff/1/content/public/common/key_s... content/public/common/key_system_registration.h:19: std::string key_system_; On 2016/01/29 07:43:06, xhwang wrote: > No need to use trailing "_" for struct members. Done. https://codereview.chromium.org/1637083002/diff/1/content/public/common/key_s... content/public/common/key_system_registration.h:23: base::FilePath path_; On 2016/01/29 07:43:06, xhwang wrote: > Can we use a more specific name? Key System doesn't really have a path. It's the > underlying cdm::CDM that has a path. How about something like cdm_path, or > cdm_library_path? Done. https://codereview.chromium.org/1637083002/diff/1/content/public/common/key_s... content/public/common/key_system_registration.h:26: base::string16 codecs_; On 2016/01/29 07:43:06, xhwang wrote: > Can we use std::vector<base::string16> for the "list" of codecs? Done. https://codereview.chromium.org/1637083002/diff/1/content/public/common/key_s... File content/public/common/key_system_service.h (right): https://codereview.chromium.org/1637083002/diff/1/content/public/common/key_s... content/public/common/key_system_service.h:26: // Register |key_system|, replacing any entry that may exist for it. |path| On 2016/01/29 07:43:07, xhwang wrote: > Register_s_; same below. Done. https://codereview.chromium.org/1637083002/diff/1/content/public/common/key_s... content/public/common/key_system_service.h:31: const base::string16& codecs); On 2016/01/29 07:43:07, xhwang wrote: > How should the list of codecs be formatted in this string? Converted to a std::vector<>. https://codereview.chromium.org/1637083002/diff/1/content/public/common/key_s... content/public/common/key_system_service.h:36: bool Path(const std::string& key_system, base::FilePath* path); On 2016/01/29 07:43:07, xhwang wrote: > Path() is a bit vague. How about GetCdmLibraryPath()? Done. https://codereview.chromium.org/1637083002/diff/1/content/public/common/key_s... content/public/common/key_system_service.h:39: // registered, true is returned and |codecs| is updated. Otherwise false On 2016/01/29 07:43:07, xhwang wrote: > Is it possible "true" is returned and "codecs" is still empty? Updated. https://codereview.chromium.org/1637083002/diff/1/content/public/common/key_s... content/public/common/key_system_service.h:41: bool Codecs(const std::string& key_system, base::string16* codecs); On 2016/01/29 07:43:07, xhwang wrote: > ditto about name too general. How about GetSupportedCodecs? Done. https://codereview.chromium.org/1637083002/diff/1/content/public/common/key_s... content/public/common/key_system_service.h:47: std::vector<base::FilePath>* paths); On 2016/01/29 07:43:07, xhwang wrote: > What's the purpose of this API? It's a bit odd that we return |key_systems| and > |paths| separately. Combined using std::pair. https://codereview.chromium.org/1637083002/diff/60001/content/common/media/co... File content/common/media/content_decryption_module_service.cc (right): https://codereview.chromium.org/1637083002/diff/60001/content/common/media/co... content/common/media/content_decryption_module_service.cc:54: for (size_t i = 0; i < registered_modules_.size(); i++) { I added FindEntry() to avoid duplicating code and then I forgot to refactor these 2 methods. Will do in next CL.
xhwang@chromium.org changed reviewers: + ddorwin@chromium.org
+ddorwin for high level review I only skimmed the CL. Here are some high level comments: - We have been using the short form "Cdm" in many places. I feel it's okay to use the short form here as well to avoid super long names. How about CdmService and CdmRegistration? CdmService is unfortunately close to MojoCdmService, but that seems fine. - We have PepperPluginInfo and KeySystemInfo, so CdmRegistration should probably be renamed to CdmInfo. - The CdmService and CdmInfo will only be used in the browser process. Maybe move content/common to content/browser? - The content public interface should be as small as possible, it seems we only need the CdmInfo and CdmService::Register(). The latter can be wrapped into a function like RegisterCdmService() so that we don't expose the whole CdmService interface as a public content API. - This CL is getting big, with 686 lines of new code... Can we add CdmInfo and CdmService + unittest first, then do registration and pepper refactoring in a different CL?
On 2016/03/05 00:10:02, xhwang wrote: > +ddorwin for high level review > > I only skimmed the CL. Here are some high level comments: > > - We have been using the short form "Cdm" in many places. I feel it's okay to > use the short form here as well to avoid super long names. How about CdmService > and CdmRegistration? CdmService is unfortunately close to MojoCdmService, but > that seems fine. Agreed. Spelling it out was requested by a Blink public OWNER. In non-media code, especially in something like as content/public/, we might want to spell it out. > - We have PepperPluginInfo and KeySystemInfo, so CdmRegistration should probably > be renamed to CdmInfo. > > - The CdmService and CdmInfo will only be used in the browser process. Maybe > move content/common to content/browser? > > - The content public interface should be as small as possible, it seems we only > need the CdmInfo and CdmService::Register(). The latter can be wrapped into a > function like RegisterCdmService() so that we don't expose the whole CdmService > interface as a public content API. > > - This CL is getting big, with 686 lines of new code... Can we add CdmInfo and > CdmService + unittest first, then do registration and pepper refactoring in a > different CL? I like small CLs, but it's also important to understand how new interfaces will be used. If that's clear, great. Also, you may want to have multiple open Cls at once so you can demonstrate the context. One other thought / proposed guideline: * CDM info/registration is the actual CDM and details about loading it. * Key System info/registration is registering the Key System for use. Some CDM details should be abstracted from the Key System info if possible. * CDM info exists in the browser and Key System info exists in the renderer. WDYT?
On 2016/03/05 01:20:10, ddorwin wrote: > On 2016/03/05 00:10:02, xhwang wrote: > > +ddorwin for high level review > > > > I only skimmed the CL. Here are some high level comments: > > > > - We have been using the short form "Cdm" in many places. I feel it's okay to > > use the short form here as well to avoid super long names. How about > CdmService > > and CdmRegistration? CdmService is unfortunately close to MojoCdmService, but > > that seems fine. > > Agreed. Spelling it out was requested by a Blink public OWNER. In non-media > code, especially in something like as content/public/, we might want to spell it > out. We already use "Cdm" in content/public/browser today: https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... > > - We have PepperPluginInfo and KeySystemInfo, so CdmRegistration should > probably > > be renamed to CdmInfo. > > > > - The CdmService and CdmInfo will only be used in the browser process. Maybe > > move content/common to content/browser? > > > > - The content public interface should be as small as possible, it seems we > only > > need the CdmInfo and CdmService::Register(). The latter can be wrapped into a > > function like RegisterCdmService() so that we don't expose the whole > CdmService > > interface as a public content API. > > > > - This CL is getting big, with 686 lines of new code... Can we add CdmInfo and > > CdmService + unittest first, then do registration and pepper refactoring in a > > different CL? > > I like small CLs, but it's also important to understand how new interfaces will > be used. If that's clear, great. Also, you may want to have multiple open Cls at > once so you can demonstrate the context. > > > One other thought / proposed guideline: > * CDM info/registration is the actual CDM and details about loading it. > * Key System info/registration is registering the Key System for use. Some CDM > details should be abstracted from the Key System info if possible. > * CDM info exists in the browser and Key System info exists in the renderer. Mostly agreed. Just one clarification, the CDM info is really for CDMs that needs to be loaded OOP using Mojo. > WDYT?
Description was changed from ========== Add content_decryption_module_service to keep track of CDMs As media moves to a mojo world, CDMs will be loaded directly into the appropriate process, rather than loaded as pepper plugins. This change adds a class to keep track of the available CDMs. On Win/Mac, the Widevine CDM is updated by the component updater, so it registers (or re-registers) the CDM as it becomes available. On Linux it will be registered if the library is available. On Linux any registered CDMs will be preloaded before the process is sandboxed. Future work: - preload the libraries on Win/Mac into whatever process will be used to host the CDM. - keep track of the CDMs that are loaded, so that they are only initialized once. - allow use of External Clear Key CDM for testing. It will be registered if requested using a command line option. BUG=550664 TEST=new content_unittests pass ========== to ========== Add content_decryption_module_service to keep track of CDMs As media moves to a mojo world, CDMs will be loaded directly into the appropriate process, rather than loaded as pepper plugins. This change adds a class to keep track of the available CDMs. On Win/Mac, the Widevine CDM is updated by the component updater, so it registers (or re-registers) the CDM as it becomes available. On Linux it will be registered if the library is available. Future work: - preload the libraries into whatever process will be used to host the CDM. - keep track of the CDMs that are loaded, so that they are only initialized once. - allow use of External Clear Key CDM for testing. It will be registered if requested using a command line option. BUG=550664 TEST=new content_unittests pass ==========
Updated. These changes simply handle the registration of Widevine, nothing else. Also updated the description to reflect this. I looked at moving the files under chrome/browser, but I don't think DEPS lets me do that. CdmService will eventually be needed by content/zygote/zygote_main_linux.cc and content/utility/utility_thread_impl.cc to get the list of CDMs for loading (matching what is done for plugins).
Description was changed from ========== Add content_decryption_module_service to keep track of CDMs As media moves to a mojo world, CDMs will be loaded directly into the appropriate process, rather than loaded as pepper plugins. This change adds a class to keep track of the available CDMs. On Win/Mac, the Widevine CDM is updated by the component updater, so it registers (or re-registers) the CDM as it becomes available. On Linux it will be registered if the library is available. Future work: - preload the libraries into whatever process will be used to host the CDM. - keep track of the CDMs that are loaded, so that they are only initialized once. - allow use of External Clear Key CDM for testing. It will be registered if requested using a command line option. BUG=550664 TEST=new content_unittests pass ========== to ========== Add CdmService to keep track of CDMs As media moves to a mojo world, CDMs will be loaded directly into the appropriate process, rather than loaded as pepper plugins. This change adds a class to keep track of the available CDMs. On Win/Mac, the Widevine CDM is updated by the component updater, so it registers (or re-registers) the CDM as it becomes available. On Linux it will be registered if the library is available. Future work: - preload the libraries into whatever process will be used to host the CDM. - keep track of the CDMs that are loaded, so that they are only initialized once. - allow use of External Clear Key CDM for testing. It will be registered if requested using a command line option. BUG=550664 TEST=new content_unittests pass ==========
This is a good start. Mostly minor comments. Let's discuss the commit message too. I did not review the component installer or unit test. https://codereview.chromium.org/1637083002/diff/80001/chrome/common/chrome_co... File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/1637083002/diff/80001/chrome/common/chrome_co... chrome/common/chrome_content_client.cc:103: static bool IsWidevineAvailable( GetWidevine<Something> would be more accurate. https://codereview.chromium.org/1637083002/diff/80001/chrome/common/chrome_co... chrome/common/chrome_content_client.cc:105: std::vector<base::string16>* codecs_supported, Should we really be working in string16 here? I think that is specific to the plugin code, so we should handle it there. https://codereview.chromium.org/1637083002/diff/80001/chrome/common/chrome_co... chrome/common/chrome_content_client.cc:106: std::vector<base::string16>* supported_init_data_types) { supported_init_data_types is never used. Even without the TODO below, these could be populated. There might be a separate TODO to pass them from the browser process rather than deriving them in the renderer. Regardless, it's weird to have an unused parameter now. https://codereview.chromium.org/1637083002/diff/80001/chrome/common/chrome_co... chrome/common/chrome_content_client.cc:554: widevine_adapter_path.DirName().AppendASCII(kWidevineCdmFileName), This line should be a separate statement under the comment above. The comment does not describe the entire statement, and this is fairly complex to inline. https://codereview.chromium.org/1637083002/diff/80001/chrome/common/chrome_co... chrome/common/chrome_content_client.cc:559: } TODO: Add ECK? https://codereview.chromium.org/1637083002/diff/80001/content/common/media/cd... File content/common/media/cdm_service.cc (right): https://codereview.chromium.org/1637083002/diff/80001/content/common/media/cd... content/common/media/cdm_service.cc:34: DCHECK(!version.empty()); Really? What about Android, etc.? https://codereview.chromium.org/1637083002/diff/80001/content/common/media/cd... content/common/media/cdm_service.cc:36: if (registered_modules_[i].cdm_id == cdm_id && As mentioned later, why not just always insert? https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... File content/public/common/cdm_info.h (right): https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_info.h:12: #include "base/strings/string16.h" Don't use string16. https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_info.h:17: // Collects information about Content Decryption Modules. An object does not perform actions. Contains? Represents? Also, it only represents a single CDM. For example, "Represents a Content Decryption Module implementation and its capabilities." Note that it is important that this represents details about a concrete CDM that is actually present - not an abstract one like Key System info. https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_info.h:20: std::string cdm_id; "CDM ID" is already overloaded. For example, in CdmContext. We should try to come up with another term. https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_info.h:22: // Version of the CDM. May be empty. (Pretty much everywhere except desktop.) https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_info.h:25: // Path to the library implementing the CDM. May be empty() if the Remove "()". This should probably be ifdef'd to the equivalent of ENABLE_PEPPER_CDMS. I would also move it below the more common and required values below. https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_info.h:26: // CDM is already included as part of the system. If we don't ifdef this, we should clean up this line. https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_info.h:29: // List of codecs supported by the CDM. How we use this information varies depending on the platform. We should fix that sometime. https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_info.h:32: // List of init_data_types supported by the CDM. s/init_data_types/Initialization Data Types/ https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_info.h:37: const base::FilePath& module_library_path, The reordering suggestion above would complicate the suggested ifdef here, though. https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... File content/public/common/cdm_service.h (right): https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_service.h:14: #include "base/strings/string16.h" no https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_service.h:27: // same |version|). |module_library_path| is the path to the actual CDM Do we expect this to be called multiple times with the same version? I would hope not. It seems simpler to just always insert. https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_service.h:28: // (may be empty if the CDM is already included as part of the system), ditto on the ifdef. https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_service.h:32: void Register(const std::string& cdm_id, nit: RegisterCdm seems better. WDYT? https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_service.h:39: // Paths in |key_systems| may be empty if the CDM is already included as Seems unnecessary. This is already covered in the CdmInfo definition. https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_service.h:49: std::vector<CdmInfo> registered_modules_; "modules" is first used alone here. "cdms" seems better. or just |cdms_|. https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... File content/public/common/content_client.h (right): https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/content_client.h:95: std::vector<content::CdmInfo>* modules) {} |cdms|
I only reviewed content/public API and part of component updater. https://chromiumcodereview.appspot.com/1637083002/diff/80001/chrome/browser/c... File chrome/browser/component_updater/widevine_cdm_component_installer.cc (right): https://chromiumcodereview.appspot.com/1637083002/diff/80001/chrome/browser/c... chrome/browser/component_updater/widevine_cdm_component_installer.cc:200: return base::string16(); move line 196 here? https://chromiumcodereview.appspot.com/1637083002/diff/80001/chrome/browser/c... chrome/browser/component_updater/widevine_cdm_component_installer.cc:204: // TODO(http://crbug.com/469202): Add this to the manifest and extract here. nit: TODO(jrummell) or TODO(xhwang) https://chromiumcodereview.appspot.com/1637083002/diff/80001/chrome/browser/c... chrome/browser/component_updater/widevine_cdm_component_installer.cc:230: // TODO(crbug.com/469202). Add support for initDataTypes in the manifest, ditto about TODO https://chromiumcodereview.appspot.com/1637083002/diff/80001/chrome/common/ch... File chrome/common/chrome_content_client.cc (right): https://chromiumcodereview.appspot.com/1637083002/diff/80001/chrome/common/ch... chrome/common/chrome_content_client.cc:187: !defined(WIDEVINE_CDM_IS_COMPONENT) Why remove skip_widevine_cdm_file_check? https://chromiumcodereview.appspot.com/1637083002/diff/80001/content/public/c... File content/public/common/cdm_info.h (right): https://chromiumcodereview.appspot.com/1637083002/diff/80001/content/public/c... content/public/common/cdm_info.h:20: std::string cdm_id; What is this for? If we need this, we are already using cdm_id in some other context (see cdm_context). Maybe we should find another name. Also, since we are already in Cdm* class, "cdm" prefix seems redundant. https://chromiumcodereview.appspot.com/1637083002/diff/80001/content/public/c... content/public/common/cdm_info.h:23: std::string version; use base::Version? https://chromiumcodereview.appspot.com/1637083002/diff/80001/content/public/c... content/public/common/cdm_info.h:27: base::FilePath module_library_path; nit: s/module_library_path/path? See: https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com... https://chromiumcodereview.appspot.com/1637083002/diff/80001/content/public/c... content/public/common/cdm_info.h:30: std::vector<base::string16> supported_codecs; why these need to be string16 instead of string? https://chromiumcodereview.appspot.com/1637083002/diff/80001/content/public/c... content/public/common/cdm_info.h:33: std::vector<base::string16> supported_init_data_types; Are we using these init_data_types in this CL? If not shall we leave it in a future CL, where we may also need session types? https://chromiumcodereview.appspot.com/1637083002/diff/80001/content/public/c... content/public/common/cdm_info.h:40: ~CdmInfo(); Declare ctor/dtor before member variables https://chromiumcodereview.appspot.com/1637083002/diff/80001/content/public/c... File content/public/common/cdm_service.h (right): https://chromiumcodereview.appspot.com/1637083002/diff/80001/content/public/c... content/public/common/cdm_service.h:26: // Registers |cdm_id|, replacing any entry that may exist for it (with ditto about cdm_id https://chromiumcodereview.appspot.com/1637083002/diff/80001/content/public/c... content/public/common/cdm_service.h:36: const std::vector<base::string16>& supported_init_data_types); It seems content APIs should be pure abstract: https://www.chromium.org/developers/content-module/content-api Maybe this file should be a pure interface and we should put the implementations in another file. https://chromiumcodereview.appspot.com/1637083002/diff/80001/content/public/c... content/public/common/cdm_service.h:41: const std::vector<CdmInfo>& GetAllRegisteredCdms() const; For interfaces we don't use "const": https://www.chromium.org/developers/content-module/content-api
https://codereview.chromium.org/1637083002/diff/80001/chrome/common/chrome_co... File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/1637083002/diff/80001/chrome/common/chrome_co... chrome/common/chrome_content_client.cc:108: if (PathService::Get(chrome::FILE_WIDEVINE_CDM_ADAPTER, cdm_adapter_path)) { This code will compile on all platforms except Windows and Mac. You need the equivalent of ENABLE_PEPPER_CDMS so that this only runs on desktop. Related to my comment below, you'll also need to decide whether this entire function is desktop-specific or whether you'll have the desktop path extract information from the results. https://codereview.chromium.org/1637083002/diff/80001/chrome/common/chrome_co... chrome/common/chrome_content_client.cc:541: void ChromeContentClient::AddContentDecryptionModules( I think you need to put all this code in a separate function that Gets all CDM Modules. You'll eventually need it for preloading CDMs before sandboxing once Pepper CDMs are disabled (unless we do per-CDM loading, in which case there will need to be more specific functions). This method would call that function on desktop. (There might be some abstraction that complicates things a bit. I'll explain offline.) This is similar to how AddPepperPlugins() calls ComputeBuiltInPlugins(), which is used in sandboxing and perhaps elsewhere.
https://codereview.chromium.org/1637083002/diff/80001/chrome/common/chrome_co... File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/1637083002/diff/80001/chrome/common/chrome_co... chrome/common/chrome_content_client.cc:108: if (PathService::Get(chrome::FILE_WIDEVINE_CDM_ADAPTER, cdm_adapter_path)) { On 2016/03/18 20:38:53, ddorwin wrote: > This code will compile on all platforms except Windows and Mac. You need the > equivalent of ENABLE_PEPPER_CDMS so that this only runs on desktop. > > Related to my comment below, you'll also need to decide whether this entire > function is desktop-specific or whether you'll have the desktop path extract > information from the results. Note that the previous location of this was protected by if defined(ENABLE_PEPPER_CDMS). This needs to be protected by the equivalent of "is desktop cdm". But this function - or similar - needs to run and be accurate on all platforms. IOW, Android should support AddContentDecryptionModules(), right?
https://codereview.chromium.org/1637083002/diff/80001/chrome/common/chrome_co... File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/1637083002/diff/80001/chrome/common/chrome_co... chrome/common/chrome_content_client.cc:108: if (PathService::Get(chrome::FILE_WIDEVINE_CDM_ADAPTER, cdm_adapter_path)) { On 2016/03/18 20:48:56, ddorwin wrote: > On 2016/03/18 20:38:53, ddorwin wrote: > > This code will compile on all platforms except Windows and Mac. You need the > > equivalent of ENABLE_PEPPER_CDMS so that this only runs on desktop. > > > > Related to my comment below, you'll also need to decide whether this entire > > function is desktop-specific or whether you'll have the desktop path extract > > information from the results. > > Note that the previous location of this was protected by if > defined(ENABLE_PEPPER_CDMS). This needs to be protected by the equivalent of "is > desktop cdm". But this function - or similar - needs to run and be accurate on > all platforms. IOW, Android should support AddContentDecryptionModules(), right? Note sure if I understand the question completely :) Android doesn't need this CL. The reason we have these logic in the browser is (1) To populate key system info* (2) CDM is component updated (3) CDM is is a separate binary so we need preloading. None of these apply to Android. On Android, the CDM is the MediaDrmBridge. * Note that CdmMessageFilterAndroid::OnQueryKeySystemSupport() doesn't use anything here.
https://codereview.chromium.org/1637083002/diff/80001/chrome/common/chrome_co... File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/1637083002/diff/80001/chrome/common/chrome_co... chrome/common/chrome_content_client.cc:108: if (PathService::Get(chrome::FILE_WIDEVINE_CDM_ADAPTER, cdm_adapter_path)) { On 2016/03/18 20:57:58, xhwang wrote: > On 2016/03/18 20:48:56, ddorwin wrote: > > On 2016/03/18 20:38:53, ddorwin wrote: > > > This code will compile on all platforms except Windows and Mac. You need the > > > equivalent of ENABLE_PEPPER_CDMS so that this only runs on desktop. > > > > > > Related to my comment below, you'll also need to decide whether this entire > > > function is desktop-specific or whether you'll have the desktop path extract > > > information from the results. > > > > Note that the previous location of this was protected by if > > defined(ENABLE_PEPPER_CDMS). This needs to be protected by the equivalent of > "is > > desktop cdm". But this function - or similar - needs to run and be accurate on > > all platforms. IOW, Android should support AddContentDecryptionModules(), > right? > > Note sure if I understand the question completely :) > > Android doesn't need this CL. The reason we have these logic in the browser is > (1) To populate key system info* (2) CDM is component updated (3) CDM is is a > separate binary so we need preloading. None of these apply to Android. > > On Android, the CDM is the MediaDrmBridge. > > * Note that CdmMessageFilterAndroid::OnQueryKeySystemSupport() doesn't use > anything here. I thought the CDM Registry and related IPCs were going to unify all these paths. That was my intent, at least. https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... File content/public/common/cdm_service.h (right): https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_service.h:21: class CONTENT_EXPORT CdmService { Is this really a service or just a registry?
https://codereview.chromium.org/1637083002/diff/80001/chrome/common/chrome_co... File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/1637083002/diff/80001/chrome/common/chrome_co... chrome/common/chrome_content_client.cc:108: if (PathService::Get(chrome::FILE_WIDEVINE_CDM_ADAPTER, cdm_adapter_path)) { On 2016/03/18 22:22:11, ddorwin wrote: > On 2016/03/18 20:57:58, xhwang wrote: > > On 2016/03/18 20:48:56, ddorwin wrote: > > > On 2016/03/18 20:38:53, ddorwin wrote: > > > > This code will compile on all platforms except Windows and Mac. You need > the > > > > equivalent of ENABLE_PEPPER_CDMS so that this only runs on desktop. > > > > > > > > Related to my comment below, you'll also need to decide whether this > entire > > > > function is desktop-specific or whether you'll have the desktop path > extract > > > > information from the results. > > > > > > Note that the previous location of this was protected by if > > > defined(ENABLE_PEPPER_CDMS). This needs to be protected by the equivalent of > > "is > > > desktop cdm". But this function - or similar - needs to run and be accurate > on > > > all platforms. IOW, Android should support AddContentDecryptionModules(), > > right? > > > > Note sure if I understand the question completely :) > > > > Android doesn't need this CL. The reason we have these logic in the browser is > > (1) To populate key system info* (2) CDM is component updated (3) CDM is is a > > separate binary so we need preloading. None of these apply to Android. > > > > On Android, the CDM is the MediaDrmBridge. > > > > * Note that CdmMessageFilterAndroid::OnQueryKeySystemSupport() doesn't use > > anything here. > > I thought the CDM Registry and related IPCs were going to unify all these paths. > That was my intent, at least. Agreed that we should unify the two IPC paths. I imagine it'll be a mojo call where the mojo service is hosted by CdmService directly. https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... File content/public/common/cdm_service.h (right): https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_service.h:21: class CONTENT_EXPORT CdmService { On 2016/03/18 22:22:11, ddorwin wrote: > Is this really a service or just a registry? Either works for me :) I guess we were following the model of PluginService.
Updated. I moved the cdmservice files to match what the pluginservice does. initdatatypes is gone. Now using std::string rather than string16. Use separate name for Widevine (instead of keysystemname). Plus a bunch of other things. https://codereview.chromium.org/1637083002/diff/80001/chrome/browser/componen... File chrome/browser/component_updater/widevine_cdm_component_installer.cc (right): https://codereview.chromium.org/1637083002/diff/80001/chrome/browser/componen... chrome/browser/component_updater/widevine_cdm_component_installer.cc:200: return base::string16(); On 2016/03/18 19:35:38, xhwang wrote: > move line 196 here? Done. https://codereview.chromium.org/1637083002/diff/80001/chrome/browser/componen... chrome/browser/component_updater/widevine_cdm_component_installer.cc:204: // TODO(http://crbug.com/469202): Add this to the manifest and extract here. On 2016/03/18 19:35:38, xhwang wrote: > nit: TODO(jrummell) or TODO(xhwang) Removed this. https://codereview.chromium.org/1637083002/diff/80001/chrome/browser/componen... chrome/browser/component_updater/widevine_cdm_component_installer.cc:230: // TODO(crbug.com/469202). Add support for initDataTypes in the manifest, On 2016/03/18 19:35:38, xhwang wrote: > ditto about TODO Also removed. https://codereview.chromium.org/1637083002/diff/80001/chrome/common/chrome_co... File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/1637083002/diff/80001/chrome/common/chrome_co... chrome/common/chrome_content_client.cc:103: static bool IsWidevineAvailable( On 2016/03/18 19:24:18, ddorwin wrote: > GetWidevine<Something> would be more accurate. However, the function returns bool to indicate if WV available or not. Not sure of a different name that isn't really long. https://codereview.chromium.org/1637083002/diff/80001/chrome/common/chrome_co... chrome/common/chrome_content_client.cc:105: std::vector<base::string16>* codecs_supported, On 2016/03/18 19:24:18, ddorwin wrote: > Should we really be working in string16 here? I think that is specific to the > plugin code, so we should handle it there. Done. https://codereview.chromium.org/1637083002/diff/80001/chrome/common/chrome_co... chrome/common/chrome_content_client.cc:106: std::vector<base::string16>* supported_init_data_types) { On 2016/03/18 19:24:17, ddorwin wrote: > supported_init_data_types is never used. Even without the TODO below, these > could be populated. There might be a separate TODO to pass them from the browser > process rather than deriving them in the renderer. > > Regardless, it's weird to have an unused parameter now. Removed. https://codereview.chromium.org/1637083002/diff/80001/chrome/common/chrome_co... chrome/common/chrome_content_client.cc:187: !defined(WIDEVINE_CDM_IS_COMPONENT) On 2016/03/18 19:35:38, xhwang wrote: > Why remove skip_widevine_cdm_file_check? It has simply moved to the IsWidevineAvailable() method. https://codereview.chromium.org/1637083002/diff/80001/chrome/common/chrome_co... chrome/common/chrome_content_client.cc:541: void ChromeContentClient::AddContentDecryptionModules( On 2016/03/18 20:38:53, ddorwin wrote: > I think you need to put all this code in a separate function that Gets all CDM > Modules. You'll eventually need it for preloading CDMs before sandboxing once > Pepper CDMs are disabled (unless we do per-CDM loading, in which case there will > need to be more specific functions). This method would call that function on > desktop. (There might be some abstraction that complicates things a bit. I'll > explain offline.) > > This is similar to how AddPepperPlugins() calls ComputeBuiltInPlugins(), which > is used in sandboxing and perhaps elsewhere. Added RegisterAllAvailableCdms() that will call AddContentDecryptionModules() to make it explicit. https://codereview.chromium.org/1637083002/diff/80001/chrome/common/chrome_co... chrome/common/chrome_content_client.cc:554: widevine_adapter_path.DirName().AppendASCII(kWidevineCdmFileName), On 2016/03/18 19:24:17, ddorwin wrote: > This line should be a separate statement under the comment above. The comment > does not describe the entire statement, and this is fairly complex to inline. Done. https://codereview.chromium.org/1637083002/diff/80001/chrome/common/chrome_co... chrome/common/chrome_content_client.cc:559: } On 2016/03/18 19:24:18, ddorwin wrote: > TODO: Add ECK? Done. https://codereview.chromium.org/1637083002/diff/80001/content/common/media/cd... File content/common/media/cdm_service.cc (right): https://codereview.chromium.org/1637083002/diff/80001/content/common/media/cd... content/common/media/cdm_service.cc:34: DCHECK(!version.empty()); On 2016/03/18 19:24:18, ddorwin wrote: > Really? What about Android, etc.? Removed. https://codereview.chromium.org/1637083002/diff/80001/content/common/media/cd... content/common/media/cdm_service.cc:36: if (registered_modules_[i].cdm_id == cdm_id && On 2016/03/18 19:24:18, ddorwin wrote: > As mentioned later, why not just always insert? Done. https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... File content/public/common/cdm_info.h (right): https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_info.h:12: #include "base/strings/string16.h" On 2016/03/18 19:24:18, ddorwin wrote: > Don't use string16. Done. https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_info.h:17: // Collects information about Content Decryption Modules. On 2016/03/18 19:24:18, ddorwin wrote: > An object does not perform actions. Contains? Represents? > Also, it only represents a single CDM. > > For example, "Represents a Content Decryption Module implementation and its > capabilities." > > Note that it is important that this represents details about a concrete CDM that > is actually present - not an abstract one like Key System info. Done. https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_info.h:20: std::string cdm_id; On 2016/03/18 19:35:38, xhwang wrote: > What is this for? > > If we need this, we are already using cdm_id in some other context (see > cdm_context). Maybe we should find another name. > > Also, since we are already in Cdm* class, "cdm" prefix seems redundant. Renamed. https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_info.h:22: // Version of the CDM. On 2016/03/18 19:24:18, ddorwin wrote: > May be empty. > > (Pretty much everywhere except desktop.) Done. https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_info.h:23: std::string version; On 2016/03/18 19:35:38, xhwang wrote: > use base::Version? Done. https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_info.h:25: // Path to the library implementing the CDM. May be empty() if the On 2016/03/18 19:24:18, ddorwin wrote: > Remove "()". > > This should probably be ifdef'd to the equivalent of ENABLE_PEPPER_CDMS. > > I would also move it below the more common and required values below. I'd like to avoid #ifdefs. https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_info.h:26: // CDM is already included as part of the system. On 2016/03/18 19:24:18, ddorwin wrote: > If we don't ifdef this, we should clean up this line. Done. https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_info.h:27: base::FilePath module_library_path; On 2016/03/18 19:35:38, xhwang wrote: > nit: s/module_library_path/path? > > See: > > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com... Done. https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_info.h:29: // List of codecs supported by the CDM. On 2016/03/18 19:24:18, ddorwin wrote: > How we use this information varies depending on the platform. We should fix that > sometime. Acknowledged. https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_info.h:30: std::vector<base::string16> supported_codecs; On 2016/03/18 19:35:38, xhwang wrote: > why these need to be string16 instead of string? Copied from what the Pepper code for WV did. Now switched to std::string. https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_info.h:32: // List of init_data_types supported by the CDM. On 2016/03/18 19:24:18, ddorwin wrote: > s/init_data_types/Initialization Data Types/ Removed since we aren't using this yet. https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_info.h:33: std::vector<base::string16> supported_init_data_types; On 2016/03/18 19:35:38, xhwang wrote: > Are we using these init_data_types in this CL? If not shall we leave it in a > future CL, where we may also need session types? Done. https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_info.h:37: const base::FilePath& module_library_path, On 2016/03/18 19:24:18, ddorwin wrote: > The reordering suggestion above would complicate the suggested ifdef here, > though. Acknowledged. https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_info.h:40: ~CdmInfo(); On 2016/03/18 19:35:38, xhwang wrote: > Declare ctor/dtor before member variables Done. https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... File content/public/common/cdm_service.h (right): https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_service.h:14: #include "base/strings/string16.h" On 2016/03/18 19:24:19, ddorwin wrote: > no Done. https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_service.h:26: // Registers |cdm_id|, replacing any entry that may exist for it (with On 2016/03/18 19:35:38, xhwang wrote: > ditto about cdm_id Done. https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_service.h:27: // same |version|). |module_library_path| is the path to the actual CDM On 2016/03/18 19:24:19, ddorwin wrote: > Do we expect this to be called multiple times with the same version? I would > hope not. It seems simpler to just always insert. Done. https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_service.h:28: // (may be empty if the CDM is already included as part of the system), On 2016/03/18 19:24:19, ddorwin wrote: > ditto on the ifdef. No longer an issue here. https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_service.h:32: void Register(const std::string& cdm_id, On 2016/03/18 19:24:19, ddorwin wrote: > nit: RegisterCdm seems better. WDYT? Done. https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_service.h:36: const std::vector<base::string16>& supported_init_data_types); On 2016/03/18 19:35:38, xhwang wrote: > It seems content APIs should be pure abstract: > > https://www.chromium.org/developers/content-module/content-api > > Maybe this file should be a pure interface and we should put the implementations > in another file. Done (by moving this file and matching what is done for plugins). https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_service.h:39: // Paths in |key_systems| may be empty if the CDM is already included as On 2016/03/18 19:24:19, ddorwin wrote: > Seems unnecessary. This is already covered in the CdmInfo definition. Done. https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_service.h:41: const std::vector<CdmInfo>& GetAllRegisteredCdms() const; On 2016/03/18 19:35:38, xhwang wrote: > For interfaces we don't use "const": > > https://www.chromium.org/developers/content-module/content-api Done. https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/cdm_service.h:49: std::vector<CdmInfo> registered_modules_; On 2016/03/18 19:24:18, ddorwin wrote: > "modules" is first used alone here. "cdms" seems better. or just |cdms_|. Done. https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... File content/public/common/content_client.h (right): https://codereview.chromium.org/1637083002/diff/80001/content/public/common/c... content/public/common/content_client.h:95: std::vector<content::CdmInfo>* modules) {} On 2016/03/18 19:24:19, ddorwin wrote: > |cdms| Done.
Thanks! I only reviewed the content public interface. https://chromiumcodereview.appspot.com/1637083002/diff/100001/content/public/... File content/public/browser/cdm_service.h (right): https://chromiumcodereview.appspot.com/1637083002/diff/100001/content/public/... content/public/browser/cdm_service.h:15: class CONTENT_EXPORT CdmService { Previously you mentioned that we can't put this under content/public/browser because "CdmService will eventually be needed by content/zygote/zygote_main_linux.cc ... to get the list of CDMs for loading". Is this not a concern any more? https://chromiumcodereview.appspot.com/1637083002/diff/100001/content/public/... content/public/browser/cdm_service.h:23: virtual void RegisterAllAvailableCdms() = 0; Why this is on the public API? Who should call it? Shouldn't this be called internally when CdmServiceImpl constructs itself? https://chromiumcodereview.appspot.com/1637083002/diff/100001/content/public/... content/public/browser/cdm_service.h:28: virtual void RegisterCdm(const CdmInfo& info, bool add_at_beginning) = 0; Today in pepper we always append at end. This is important so that we don't end up using the information in a older CDM for rMKSA, but load the newer CDM in createMediaKeys(). Only after we restart the browser will the new CDM take effect if there was an old CDM. So for this API, when will we need |add_at_beginning| to be true? https://chromiumcodereview.appspot.com/1637083002/diff/100001/content/public/... File content/public/common/cdm_info.h (right): https://chromiumcodereview.appspot.com/1637083002/diff/100001/content/public/... content/public/common/cdm_info.h:25: // Identifier for the CDM (e.g. Widevine). nit: s/Identifier/Name? https://chromiumcodereview.appspot.com/1637083002/diff/100001/content/public/... content/public/common/cdm_info.h:26: std::string name; Not for this CL. I wonder whether we should use enum for the CDM name/type. BTW, in the FileIO CL we are using "type". We'd better be consistent. https://chromiumcodereview.appspot.com/1637083002/diff/100001/content/public/... content/public/common/cdm_info.h:32: // CDM is always available (e.g. Widevine on Android). On Linux our WV CDM is also always available... It seems the fundamental difference is the CDM is a separate library. https://chromiumcodereview.appspot.com/1637083002/diff/100001/content/public/... content/public/common/cdm_info.h:35: // List of codecs supported by the CDM (e.g. webm). nit: "webm" is not a codec. You can use "vp8" as an example. https://chromiumcodereview.appspot.com/1637083002/diff/100001/content/public/... content/public/common/cdm_info.h:36: std::vector<std::string> supported_codecs; We should probably use VideoCodec and AudioCodec here. Since this will affect existing code (e.g. component installer, IPC), we can do it later. Could you please add a TODO? https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_c...
Looks like I just updated comments, and replied to the other questions. https://codereview.chromium.org/1637083002/diff/100001/content/public/browser... File content/public/browser/cdm_service.h (right): https://codereview.chromium.org/1637083002/diff/100001/content/public/browser... content/public/browser/cdm_service.h:15: class CONTENT_EXPORT CdmService { On 2016/03/26 07:46:34, xhwang wrote: > Previously you mentioned that we can't put this under content/public/browser > because "CdmService will eventually be needed by > content/zygote/zygote_main_linux.cc ... to get the list of CDMs for loading". Is > this not a concern any more? As we discussed offline, the zygote doesn't use the PluginService. It simply calls the equivalent of AddContentDecryptionModules() to get the list of plugins. So we'll do the same. https://codereview.chromium.org/1637083002/diff/100001/content/public/browser... content/public/browser/cdm_service.h:23: virtual void RegisterAllAvailableCdms() = 0; On 2016/03/26 07:46:34, xhwang wrote: > Why this is on the public API? Who should call it? Shouldn't this be called > internally when CdmServiceImpl constructs itself? ddorwin@ asked in a previous comment to make it explicit. This also matches up with PluginServiceImpl::RegisterPepperPlugins(). https://codereview.chromium.org/1637083002/diff/100001/content/public/browser... content/public/browser/cdm_service.h:28: virtual void RegisterCdm(const CdmInfo& info, bool add_at_beginning) = 0; On 2016/03/26 07:46:34, xhwang wrote: > Today in pepper we always append at end. This is important so that we don't end > up using the information in a older CDM for rMKSA, but load the newer CDM in > createMediaKeys(). Only after we restart the browser will the new CDM take > effect if there was an old CDM. > > So for this API, when will we need |add_at_beginning| to be true? The component updater adds new versions to the front of the list. However, either way we have possible problems between the timing of the RMSKA() call and the timing of CreateMediaKeys() if we don't pass versions around (e.g. the former could see v1, while v2 is available by the time of the latter). Currently the view is that v2 will still be a better fit, as capabilities should normally be increasing. Obviously if they decrease, there will be possible problems later on. https://codereview.chromium.org/1637083002/diff/100001/content/public/common/... File content/public/common/cdm_info.h (right): https://codereview.chromium.org/1637083002/diff/100001/content/public/common/... content/public/common/cdm_info.h:25: // Identifier for the CDM (e.g. Widevine). On 2016/03/26 07:46:34, xhwang wrote: > nit: s/Identifier/Name? Done. https://codereview.chromium.org/1637083002/diff/100001/content/public/common/... content/public/common/cdm_info.h:26: std::string name; On 2016/03/26 07:46:34, xhwang wrote: > Not for this CL. I wonder whether we should use enum for the CDM name/type. BTW, > in the FileIO CL we are using "type". We'd better be consistent. Could be done, but then Cast would need to change it to add any additional CDMs. This way it's just a string. https://codereview.chromium.org/1637083002/diff/100001/content/public/common/... content/public/common/cdm_info.h:32: // CDM is always available (e.g. Widevine on Android). On 2016/03/26 07:46:34, xhwang wrote: > On Linux our WV CDM is also always available... It seems the fundamental > difference is the CDM is a separate library. Done. https://codereview.chromium.org/1637083002/diff/100001/content/public/common/... content/public/common/cdm_info.h:35: // List of codecs supported by the CDM (e.g. webm). On 2016/03/26 07:46:34, xhwang wrote: > nit: "webm" is not a codec. You can use "vp8" as an example. Done.
More discussion on the interface. Thanks for the patience! https://chromiumcodereview.appspot.com/1637083002/diff/100001/content/public/... File content/public/browser/cdm_service.h (right): https://chromiumcodereview.appspot.com/1637083002/diff/100001/content/public/... content/public/browser/cdm_service.h:15: class CONTENT_EXPORT CdmService { On 2016/03/29 23:48:45, jrummell wrote: > On 2016/03/26 07:46:34, xhwang wrote: > > Previously you mentioned that we can't put this under content/public/browser > > because "CdmService will eventually be needed by > > content/zygote/zygote_main_linux.cc ... to get the list of CDMs for loading". > Is > > this not a concern any more? > > As we discussed offline, the zygote doesn't use the PluginService. It simply > calls the equivalent of AddContentDecryptionModules() to get the list of > plugins. So we'll do the same. Great. It's even better now we can keep this in content/public/browser! https://chromiumcodereview.appspot.com/1637083002/diff/100001/content/public/... content/public/browser/cdm_service.h:23: virtual void RegisterAllAvailableCdms() = 0; On 2016/03/29 23:48:45, jrummell wrote: > On 2016/03/26 07:46:34, xhwang wrote: > > Why this is on the public API? Who should call it? Shouldn't this be called > > internally when CdmServiceImpl constructs itself? > > ddorwin@ asked in a previous comment to make it explicit. This also matches up > with PluginServiceImpl::RegisterPepperPlugins(). RegisterPepperPlugins() is a private helper function in PluginServiceImpl. It's not part of content public API. Do you remember why ddorwin@ suggests to add this? https://chromiumcodereview.appspot.com/1637083002/diff/100001/content/public/... content/public/browser/cdm_service.h:28: virtual void RegisterCdm(const CdmInfo& info, bool add_at_beginning) = 0; On 2016/03/29 23:48:45, jrummell wrote: > On 2016/03/26 07:46:34, xhwang wrote: > > Today in pepper we always append at end. This is important so that we don't > end > > up using the information in a older CDM for rMKSA, but load the newer CDM in > > createMediaKeys(). Only after we restart the browser will the new CDM take > > effect if there was an old CDM. > > > > So for this API, when will we need |add_at_beginning| to be true? > > The component updater adds new versions to the front of the list. > > However, either way we have possible problems between the timing of the RMSKA() > call and the timing of CreateMediaKeys() if we don't pass versions around (e.g. > the former could see v1, while v2 is available by the time of the latter). > Currently the view is that v2 will still be a better fit, as capabilities should > normally be increasing. Obviously if they decrease, there will be possible > problems later on. This model is seems a bit complicated. How would we decide wether |add_at_beginning| should be true or false? I am a big fan of simple solutions that work. Can we just keep the current behavior such that the second registration will not take effect? In the component CDM's case, after we restart the browser, the old CDM will be removed from disk and we'll only register the new CDM. https://chromiumcodereview.appspot.com/1637083002/diff/100001/content/public/... File content/public/common/cdm_info.h (right): https://chromiumcodereview.appspot.com/1637083002/diff/100001/content/public/... content/public/common/cdm_info.h:26: std::string name; On 2016/03/29 23:48:45, jrummell wrote: > On 2016/03/26 07:46:34, xhwang wrote: > > Not for this CL. I wonder whether we should use enum for the CDM name/type. > BTW, > > in the FileIO CL we are using "type". We'd better be consistent. > > Could be done, but then Cast would need to change it to add any additional CDMs. > This way it's just a string. Can consolidate on "name" vs "type" (as in the file io cl)?
Updated. https://codereview.chromium.org/1637083002/diff/100001/content/public/browser... File content/public/browser/cdm_service.h (right): https://codereview.chromium.org/1637083002/diff/100001/content/public/browser... content/public/browser/cdm_service.h:15: class CONTENT_EXPORT CdmService { On 2016/03/30 05:11:18, xhwang wrote: > On 2016/03/29 23:48:45, jrummell wrote: > > On 2016/03/26 07:46:34, xhwang wrote: > > > Previously you mentioned that we can't put this under content/public/browser > > > because "CdmService will eventually be needed by > > > content/zygote/zygote_main_linux.cc ... to get the list of CDMs for > loading". > > Is > > > this not a concern any more? > > > > As we discussed offline, the zygote doesn't use the PluginService. It simply > > calls the equivalent of AddContentDecryptionModules() to get the list of > > plugins. So we'll do the same. > > Great. It's even better now we can keep this in content/public/browser! Agreed. https://codereview.chromium.org/1637083002/diff/100001/content/public/browser... content/public/browser/cdm_service.h:23: virtual void RegisterAllAvailableCdms() = 0; On 2016/03/30 05:11:17, xhwang wrote: > On 2016/03/29 23:48:45, jrummell wrote: > > On 2016/03/26 07:46:34, xhwang wrote: > > > Why this is on the public API? Who should call it? Shouldn't this be called > > > internally when CdmServiceImpl constructs itself? > > > > ddorwin@ asked in a previous comment to make it explicit. This also matches up > > with PluginServiceImpl::RegisterPepperPlugins(). > > RegisterPepperPlugins() is a private helper function in PluginServiceImpl. It's > not part of content public API. Do you remember why ddorwin@ suggests to add > this? As discussed offline, this is now renamed Init() (which is the public call on PluginService). https://codereview.chromium.org/1637083002/diff/100001/content/public/browser... content/public/browser/cdm_service.h:28: virtual void RegisterCdm(const CdmInfo& info, bool add_at_beginning) = 0; On 2016/03/30 05:11:18, xhwang wrote: > On 2016/03/29 23:48:45, jrummell wrote: > > On 2016/03/26 07:46:34, xhwang wrote: > > > Today in pepper we always append at end. This is important so that we don't > > end > > > up using the information in a older CDM for rMKSA, but load the newer CDM in > > > createMediaKeys(). Only after we restart the browser will the new CDM take > > > effect if there was an old CDM. > > > > > > So for this API, when will we need |add_at_beginning| to be true? > > > > The component updater adds new versions to the front of the list. > > > > However, either way we have possible problems between the timing of the > RMSKA() > > call and the timing of CreateMediaKeys() if we don't pass versions around > (e.g. > > the former could see v1, while v2 is available by the time of the latter). > > Currently the view is that v2 will still be a better fit, as capabilities > should > > normally be increasing. Obviously if they decrease, there will be possible > > problems later on. > > This model is seems a bit complicated. How would we decide wether > |add_at_beginning| should be true or false? > > I am a big fan of simple solutions that work. Can we just keep the current > behavior such that the second registration will not take effect? In the > component CDM's case, after we restart the browser, the old CDM will be removed > from disk and we'll only register the new CDM. Done. https://codereview.chromium.org/1637083002/diff/100001/content/public/common/... File content/public/common/cdm_info.h (right): https://codereview.chromium.org/1637083002/diff/100001/content/public/common/... content/public/common/cdm_info.h:26: std::string name; On 2016/03/30 05:11:18, xhwang wrote: > On 2016/03/29 23:48:45, jrummell wrote: > > On 2016/03/26 07:46:34, xhwang wrote: > > > Not for this CL. I wonder whether we should use enum for the CDM name/type. > > BTW, > > > in the FileIO CL we are using "type". We'd better be consistent. > > > > Could be done, but then Cast would need to change it to add any additional > CDMs. > > This way it's just a string. > > Can consolidate on "name" vs "type" (as in the file io cl)? Switched to "type".
Sorry for the delay. Mostly looking good with a few comments. https://chromiumcodereview.appspot.com/1637083002/diff/140001/chrome/browser/... File chrome/browser/component_updater/widevine_cdm_component_installer.cc (right): https://chromiumcodereview.appspot.com/1637083002/diff/140001/chrome/browser/... chrome/browser/component_updater/widevine_cdm_component_installer.cc:226: adapter_install_path.DirName().AppendASCII(kWidevineCdmFileName); This is a bit hacky. To be future proof, can we pass cdm_install_path in RegisterWidevineCdmWithChrome() and MakeWidevineCdmPluginInfo(). Then in MakeWidevineCdmPluginInfo(), it's trivial to get the adapter_install_path. https://chromiumcodereview.appspot.com/1637083002/diff/140001/chrome/browser/... chrome/browser/component_updater/widevine_cdm_component_installer.cc:230: base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL); nit: We probably don't want whitespace and don't like empty entries. I wish there's an easy way to check this. https://chromiumcodereview.appspot.com/1637083002/diff/140001/chrome/browser/... chrome/browser/component_updater/widevine_cdm_component_installer.cc:234: true); Now we don't take a boolean any more. https://chromiumcodereview.appspot.com/1637083002/diff/140001/chrome/common/c... File chrome/common/chrome_content_client.cc (right): https://chromiumcodereview.appspot.com/1637083002/diff/140001/chrome/common/c... chrome/common/chrome_content_client.cc:547: // same directory as the installed adapter. We probably should have a new path for DIR_WIDEVINE_CDM which will contain both the CDM and the CDM adapter. Feel free to do this in a next CL (with a TODO). https://chromiumcodereview.appspot.com/1637083002/diff/140001/chrome/common/c... File chrome/common/chrome_content_client.h (right): https://chromiumcodereview.appspot.com/1637083002/diff/140001/chrome/common/c... chrome/common/chrome_content_client.h:64: std::vector<content::CdmInfo>* modules) override; nit: How about s/module/cdms/ https://chromiumcodereview.appspot.com/1637083002/diff/140001/content/browser... File content/browser/browser_main_loop.cc (right): https://chromiumcodereview.appspot.com/1637083002/diff/140001/content/browser... content/browser/browser_main_loop.cc:174: #if defined(ENABLE_MOJO_MEDIA) #if defined(ENABLE_MOJO_CDM) && defined(ENABLE_PEPPER_CDMS) https://chromiumcodereview.appspot.com/1637083002/diff/140001/content/browser... content/browser/browser_main_loop.cc:750: #if defined(ENABLE_MOJO_MEDIA) ditto https://chromiumcodereview.appspot.com/1637083002/diff/140001/content/browser... content/browser/browser_main_loop.cc:752: // CDM service as it is predominantly used from the IO thread. Also mention that this must be called on the main thread since it involves file path check. https://chromiumcodereview.appspot.com/1637083002/diff/140001/content/browser... File content/browser/media/cdm_service_impl_unittest.cc (right): https://chromiumcodereview.appspot.com/1637083002/diff/140001/content/browser... content/browser/media/cdm_service_impl_unittest.cc:22: const char kTestCodecs[] = "webm,mp4"; nit: webm is not a codec https://chromiumcodereview.appspot.com/1637083002/diff/140001/content/browser... content/browser/media/cdm_service_impl_unittest.cc:59: const char kTestKeySystem[] = "test1"; kTestKeySystemType? Also it seems not interesting to have different types for each test. Shall we just use a const one for all tests? https://chromiumcodereview.appspot.com/1637083002/diff/140001/content/browser... content/browser/media/cdm_service_impl_unittest.cc:80: } Can you add a test to make sure the new CDM is inserted from the beginning? https://chromiumcodereview.appspot.com/1637083002/diff/140001/content/public/... File content/public/browser/cdm_service.h (right): https://chromiumcodereview.appspot.com/1637083002/diff/140001/content/public/... content/public/browser/cdm_service.h:26: // insertted at the head of the list so that it can override any older inserted https://chromiumcodereview.appspot.com/1637083002/diff/140001/content/public/... content/public/browser/cdm_service.h:27: // registrations. Please add a note to potential issues described in https://bugs.chromium.org/p/chromium/issues/detail?id=599588 Thanks!
Updated. https://codereview.chromium.org/1637083002/diff/140001/chrome/browser/compone... File chrome/browser/component_updater/widevine_cdm_component_installer.cc (right): https://codereview.chromium.org/1637083002/diff/140001/chrome/browser/compone... chrome/browser/component_updater/widevine_cdm_component_installer.cc:226: adapter_install_path.DirName().AppendASCII(kWidevineCdmFileName); On 2016/04/13 00:22:12, xhwang wrote: > This is a bit hacky. To be future proof, can we pass cdm_install_path in > RegisterWidevineCdmWithChrome() and MakeWidevineCdmPluginInfo(). Then in > MakeWidevineCdmPluginInfo(), it's trivial to get the adapter_install_path. Done. https://codereview.chromium.org/1637083002/diff/140001/chrome/browser/compone... chrome/browser/component_updater/widevine_cdm_component_installer.cc:230: base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL); On 2016/04/13 00:22:12, xhwang wrote: > nit: We probably don't want whitespace and don't like empty entries. I wish > there's an easy way to check this. Done. https://codereview.chromium.org/1637083002/diff/140001/chrome/browser/compone... chrome/browser/component_updater/widevine_cdm_component_installer.cc:234: true); On 2016/04/13 00:22:12, xhwang wrote: > Now we don't take a boolean any more. Done. https://codereview.chromium.org/1637083002/diff/140001/chrome/common/chrome_c... File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/1637083002/diff/140001/chrome/common/chrome_c... chrome/common/chrome_content_client.cc:547: // same directory as the installed adapter. On 2016/04/13 00:22:12, xhwang wrote: > We probably should have a new path for DIR_WIDEVINE_CDM which will contain both > the CDM and the CDM adapter. > > Feel free to do this in a next CL (with a TODO). Done. I also moved it into IsWidevineAvailable() so we check for the existence on both the CDM and the adapter. https://codereview.chromium.org/1637083002/diff/140001/chrome/common/chrome_c... File chrome/common/chrome_content_client.h (right): https://codereview.chromium.org/1637083002/diff/140001/chrome/common/chrome_c... chrome/common/chrome_content_client.h:64: std::vector<content::CdmInfo>* modules) override; On 2016/04/13 00:22:12, xhwang wrote: > nit: How about s/module/cdms/ Done. https://codereview.chromium.org/1637083002/diff/140001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/1637083002/diff/140001/content/browser/browse... content/browser/browser_main_loop.cc:174: #if defined(ENABLE_MOJO_MEDIA) On 2016/04/13 00:22:12, xhwang wrote: > #if defined(ENABLE_MOJO_CDM) && defined(ENABLE_PEPPER_CDMS) Done. https://codereview.chromium.org/1637083002/diff/140001/content/browser/browse... content/browser/browser_main_loop.cc:750: #if defined(ENABLE_MOJO_MEDIA) On 2016/04/13 00:22:12, xhwang wrote: > ditto Done. https://codereview.chromium.org/1637083002/diff/140001/content/browser/browse... content/browser/browser_main_loop.cc:752: // CDM service as it is predominantly used from the IO thread. On 2016/04/13 00:22:12, xhwang wrote: > Also mention that this must be called on the main thread since it involves file > path check. Done. https://codereview.chromium.org/1637083002/diff/140001/content/browser/media/... File content/browser/media/cdm_service_impl_unittest.cc (right): https://codereview.chromium.org/1637083002/diff/140001/content/browser/media/... content/browser/media/cdm_service_impl_unittest.cc:22: const char kTestCodecs[] = "webm,mp4"; On 2016/04/13 00:22:12, xhwang wrote: > nit: webm is not a codec Done. https://codereview.chromium.org/1637083002/diff/140001/content/browser/media/... content/browser/media/cdm_service_impl_unittest.cc:59: const char kTestKeySystem[] = "test1"; On 2016/04/13 00:22:12, xhwang wrote: > kTestKeySystemType? Renamed. > Also it seems not interesting to have different types for each test. Shall we > just use a const one for all tests? CdmService is a singleton, so the tests all add to the list. Using different names avoids this. https://codereview.chromium.org/1637083002/diff/140001/content/browser/media/... content/browser/media/cdm_service_impl_unittest.cc:80: } On 2016/04/13 00:22:12, xhwang wrote: > Can you add a test to make sure the new CDM is inserted from the beginning? Done. https://codereview.chromium.org/1637083002/diff/140001/content/public/browser... File content/public/browser/cdm_service.h (right): https://codereview.chromium.org/1637083002/diff/140001/content/public/browser... content/public/browser/cdm_service.h:26: // insertted at the head of the list so that it can override any older On 2016/04/13 00:22:13, xhwang wrote: > inserted Done. https://codereview.chromium.org/1637083002/diff/140001/content/public/browser... content/public/browser/cdm_service.h:27: // registrations. On 2016/04/13 00:22:13, xhwang wrote: > Please add a note to potential issues described in > https://bugs.chromium.org/p/chromium/issues/detail?id=599588 > > Thanks! Done (although I think this is more an issue for the service providing data to RMKSA than this registration).
looking good. I only have a few comments on the test. https://chromiumcodereview.appspot.com/1637083002/diff/140001/content/browser... File content/browser/media/cdm_service_impl_unittest.cc (right): https://chromiumcodereview.appspot.com/1637083002/diff/140001/content/browser... content/browser/media/cdm_service_impl_unittest.cc:59: const char kTestKeySystem[] = "test1"; On 2016/04/14 01:32:16, jrummell wrote: > On 2016/04/13 00:22:12, xhwang wrote: > > kTestKeySystemType? > > Renamed. > > > Also it seems not interesting to have different types for each test. Shall we > > just use a const one for all tests? > > CdmService is a singleton, so the tests all add to the list. Using different > names avoids this. See the new comment about singleton. https://chromiumcodereview.appspot.com/1637083002/diff/160001/chrome/browser/... File chrome/browser/component_updater/widevine_cdm_component_installer.cc (right): https://chromiumcodereview.appspot.com/1637083002/diff/160001/chrome/browser/... chrome/browser/component_updater/widevine_cdm_component_installer.cc:224: // actual Widevine library, not the adapter. This comment seems redundant. https://chromiumcodereview.appspot.com/1637083002/diff/160001/content/browser... File content/browser/media/cdm_service_impl_unittest.cc (right): https://chromiumcodereview.appspot.com/1637083002/diff/160001/content/browser... content/browser/media/cdm_service_impl_unittest.cc:14: #include "content/browser/media/cdm_service_impl.h" Move this as the first include since this is what we are testing. https://chromiumcodereview.appspot.com/1637083002/diff/160001/content/browser... content/browser/media/cdm_service_impl_unittest.cc:40: CdmService::GetInstance()->RegisterCdm( Since this is CdmServiceImplTest, we should call into CdmServiceImpl directly. That would also avoid the issue associated with singleton.
Updated. https://codereview.chromium.org/1637083002/diff/160001/chrome/browser/compone... File chrome/browser/component_updater/widevine_cdm_component_installer.cc (right): https://codereview.chromium.org/1637083002/diff/160001/chrome/browser/compone... chrome/browser/component_updater/widevine_cdm_component_installer.cc:224: // actual Widevine library, not the adapter. On 2016/04/14 06:20:26, xhwang wrote: > This comment seems redundant. Done. https://codereview.chromium.org/1637083002/diff/160001/content/browser/media/... File content/browser/media/cdm_service_impl_unittest.cc (right): https://codereview.chromium.org/1637083002/diff/160001/content/browser/media/... content/browser/media/cdm_service_impl_unittest.cc:14: #include "content/browser/media/cdm_service_impl.h" On 2016/04/14 06:20:26, xhwang wrote: > Move this as the first include since this is what we are testing. Done. https://codereview.chromium.org/1637083002/diff/160001/content/browser/media/... content/browser/media/cdm_service_impl_unittest.cc:40: CdmService::GetInstance()->RegisterCdm( On 2016/04/14 06:20:26, xhwang wrote: > Since this is CdmServiceImplTest, we should call into CdmServiceImpl directly. > That would also avoid the issue associated with singleton. Done.
LGTM, thanks!
jrummell@chromium.org changed reviewers: + sievers@chromium.org, sorin@chromium.org, thestig@chromium.org
Adding OWNERS. +sorin@ for chrome/browser/component_updater/widevine_cdm_component_installer.cc +thestig@ for chrome/common/* +sievers@ for content/
lgtm Thank you! Minor style issues below, please consider fixing as you wish. https://codereview.chromium.org/1637083002/diff/200001/chrome/browser/compone... File chrome/browser/component_updater/widevine_cdm_component_installer.cc (right): https://codereview.chromium.org/1637083002/diff/200001/chrome/browser/compone... chrome/browser/component_updater/widevine_cdm_component_installer.cc:116: base::string16 codecs, Consider passing codecs as a ref to const type. https://codereview.chromium.org/1637083002/diff/200001/chrome/browser/compone... chrome/browser/component_updater/widevine_cdm_component_installer.cc:207: base::string16 codecs = GetCodecs(*manifest); const? https://codereview.chromium.org/1637083002/diff/200001/chrome/browser/compone... chrome/browser/component_updater/widevine_cdm_component_installer.cc:226: std::vector<std::string> supported_codecs = const? https://codereview.chromium.org/1637083002/diff/200001/chrome/common/chrome_c... File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/1637083002/diff/200001/chrome/common/chrome_c... chrome/common/chrome_content_client.cc:197: widevine_cdm.description = kWidevineCdmDescription + Could use stringprintf.h for more efficient formatting of these literals. It will eliminate several temporaries created when the rhs of the assignment is evaluated. https://codereview.chromium.org/1637083002/diff/200001/chrome/common/chrome_c... chrome/common/chrome_content_client.cc:556: base::Version version(WIDEVINE_CDM_VERSION_STRING); const https://codereview.chromium.org/1637083002/diff/200001/content/browser/media/... File content/browser/media/cdm_service_impl.h (right): https://codereview.chromium.org/1637083002/diff/200001/content/browser/media/... content/browser/media/cdm_service_impl.h:26: void RegisterCdm(const CdmInfo& info) override; Can we just forward declare CdmInfo instead of including the entire header file for its definition? https://codereview.chromium.org/1637083002/diff/200001/content/browser/media/... content/browser/media/cdm_service_impl.h:27: const std::vector<CdmInfo>& GetAllRegisteredCdms() override; const? https://codereview.chromium.org/1637083002/diff/200001/content/browser/media/... File content/browser/media/cdm_service_impl_unittest.cc (right): https://codereview.chromium.org/1637083002/diff/200001/content/browser/media/... content/browser/media/cdm_service_impl_unittest.cc:39: std::vector<std::string> codecs = const? https://codereview.chromium.org/1637083002/diff/200001/content/browser/media/... content/browser/media/cdm_service_impl_unittest.cc:97: std::vector<std::string> versions = GetVersions(kTestKeySystemType); const https://codereview.chromium.org/1637083002/diff/200001/content/public/browser... File content/public/browser/cdm_service.h (right): https://codereview.chromium.org/1637083002/diff/200001/content/public/browser... content/public/browser/cdm_service.h:35: virtual const std::vector<CdmInfo>& GetAllRegisteredCdms() = 0; const? https://codereview.chromium.org/1637083002/diff/200001/content/public/browser... content/public/browser/cdm_service.h:35: virtual const std::vector<CdmInfo>& GetAllRegisteredCdms() = 0; forward declare CdmInfo? https://codereview.chromium.org/1637083002/diff/200001/content/public/common/... File content/public/common/content_client.h (right): https://codereview.chromium.org/1637083002/diff/200001/content/public/common/... content/public/common/content_client.h:91: // module(s) it supports. Maybe we just say modules.
Updated. https://codereview.chromium.org/1637083002/diff/200001/chrome/browser/compone... File chrome/browser/component_updater/widevine_cdm_component_installer.cc (right): https://codereview.chromium.org/1637083002/diff/200001/chrome/browser/compone... chrome/browser/component_updater/widevine_cdm_component_installer.cc:116: base::string16 codecs, On 2016/04/15 01:53:26, Sorin Jianu wrote: > Consider passing codecs as a ref to const type. Done. https://codereview.chromium.org/1637083002/diff/200001/chrome/browser/compone... chrome/browser/component_updater/widevine_cdm_component_installer.cc:207: base::string16 codecs = GetCodecs(*manifest); On 2016/04/15 01:53:26, Sorin Jianu wrote: > const? Done. https://codereview.chromium.org/1637083002/diff/200001/chrome/browser/compone... chrome/browser/component_updater/widevine_cdm_component_installer.cc:226: std::vector<std::string> supported_codecs = On 2016/04/15 01:53:26, Sorin Jianu wrote: > const? Done. https://codereview.chromium.org/1637083002/diff/200001/chrome/common/chrome_c... File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/1637083002/diff/200001/chrome/common/chrome_c... chrome/common/chrome_content_client.cc:197: widevine_cdm.description = kWidevineCdmDescription + On 2016/04/15 01:53:26, Sorin Jianu wrote: > Could use stringprintf.h for more efficient formatting of these literals. It > will eliminate several temporaries created when the rhs of the assignment is > evaluated. Done. https://codereview.chromium.org/1637083002/diff/200001/chrome/common/chrome_c... chrome/common/chrome_content_client.cc:556: base::Version version(WIDEVINE_CDM_VERSION_STRING); On 2016/04/15 01:53:26, Sorin Jianu wrote: > const Done. https://codereview.chromium.org/1637083002/diff/200001/content/browser/media/... File content/browser/media/cdm_service_impl.h (right): https://codereview.chromium.org/1637083002/diff/200001/content/browser/media/... content/browser/media/cdm_service_impl.h:26: void RegisterCdm(const CdmInfo& info) override; On 2016/04/15 01:53:26, Sorin Jianu wrote: > Can we just forward declare CdmInfo instead of including the entire header file > for its definition? Done. https://codereview.chromium.org/1637083002/diff/200001/content/browser/media/... content/browser/media/cdm_service_impl.h:27: const std::vector<CdmInfo>& GetAllRegisteredCdms() override; On 2016/04/15 01:53:26, Sorin Jianu wrote: > const? Done. https://codereview.chromium.org/1637083002/diff/200001/content/browser/media/... File content/browser/media/cdm_service_impl_unittest.cc (right): https://codereview.chromium.org/1637083002/diff/200001/content/browser/media/... content/browser/media/cdm_service_impl_unittest.cc:39: std::vector<std::string> codecs = On 2016/04/15 01:53:26, Sorin Jianu wrote: > const? Done. https://codereview.chromium.org/1637083002/diff/200001/content/browser/media/... content/browser/media/cdm_service_impl_unittest.cc:97: std::vector<std::string> versions = GetVersions(kTestKeySystemType); On 2016/04/15 01:53:26, Sorin Jianu wrote: > const Done. https://codereview.chromium.org/1637083002/diff/200001/content/public/browser... File content/public/browser/cdm_service.h (right): https://codereview.chromium.org/1637083002/diff/200001/content/public/browser... content/public/browser/cdm_service.h:35: virtual const std::vector<CdmInfo>& GetAllRegisteredCdms() = 0; On 2016/04/15 01:53:26, Sorin Jianu wrote: > forward declare CdmInfo? Done. https://codereview.chromium.org/1637083002/diff/200001/content/public/browser... content/public/browser/cdm_service.h:35: virtual const std::vector<CdmInfo>& GetAllRegisteredCdms() = 0; On 2016/04/15 01:53:26, Sorin Jianu wrote: > const? Done. https://codereview.chromium.org/1637083002/diff/200001/content/public/common/... File content/public/common/content_client.h (right): https://codereview.chromium.org/1637083002/diff/200001/content/public/common/... content/public/common/content_client.h:91: // module(s) it supports. On 2016/04/15 01:53:26, Sorin Jianu wrote: > Maybe we just say modules. Done.
https://codereview.chromium.org/1637083002/diff/220001/content/public/browser... File content/public/browser/cdm_service.h (right): https://codereview.chromium.org/1637083002/diff/220001/content/public/browser... content/public/browser/cdm_service.h:15: Can you also put a small comment, at least saying what 'CDM' stands for, and what a CDM service is? https://codereview.chromium.org/1637083002/diff/220001/content/public/browser... content/public/browser/cdm_service.h:34: Hmm it's a bit confusing that there are two APIs to add CDMs (this one and the ContentClient callback). Why do we need both? https://codereview.chromium.org/1637083002/diff/220001/content/public/browser... content/public/browser/cdm_service.h:36: virtual const std::vector<CdmInfo>& GetAllRegisteredCdms() const = 0; Make this method non-const, see reasoning here: https://www.chromium.org/developers/content-module/content-api
Updated. https://codereview.chromium.org/1637083002/diff/220001/content/public/browser... File content/public/browser/cdm_service.h (right): https://codereview.chromium.org/1637083002/diff/220001/content/public/browser... content/public/browser/cdm_service.h:15: On 2016/04/18 21:24:21, sievers wrote: > Can you also put a small comment, at least saying what 'CDM' stands for, and > what a CDM service is? Done. https://codereview.chromium.org/1637083002/diff/220001/content/public/browser... content/public/browser/cdm_service.h:34: On 2016/04/18 21:24:21, sievers wrote: > Hmm it's a bit confusing that there are two APIs to add CDMs (this one and the > ContentClient callback). Why do we need both? Widevine (the only CDM supported with Chrome) is updated via the component updater on Win/Mac, bundled on Linux, and included as part of Android and Cast. So we need a way for embedders to register CDMs as well as the component updater. https://codereview.chromium.org/1637083002/diff/220001/content/public/browser... content/public/browser/cdm_service.h:36: virtual const std::vector<CdmInfo>& GetAllRegisteredCdms() const = 0; On 2016/04/18 21:24:21, sievers wrote: > Make this method non-const, see reasoning here: > https://www.chromium.org/developers/content-module/content-api Done.
https://codereview.chromium.org/1637083002/diff/240001/chrome/browser/compone... File chrome/browser/component_updater/widevine_cdm_component_installer.cc (right): https://codereview.chromium.org/1637083002/diff/240001/chrome/browser/compone... chrome/browser/component_updater/widevine_cdm_component_installer.cc:194: if (manifest.GetString(kCdmCodecsListName, &codecs)) { GetStringASCII(), then in RegisterWidevineCdmWithChrome() omit the UTF16ToASCII(codecs), and add ASCIIToUTF16() to MakeWidevineCdmPluginInfo(). This skips one round of conversations because GetString() internally has to convert to string16. https://codereview.chromium.org/1637083002/diff/240001/chrome/common/chrome_c... File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/1637083002/diff/240001/chrome/common/chrome_c... chrome/common/chrome_content_client.cc:76: #if defined(WIDEVINE_CDM_AVAILABLE) && defined(ENABLE_PEPPER_CDMS) && \ Do you want to #define something here and use that below, instead of writing out this long #if statement over and over? https://codereview.chromium.org/1637083002/diff/240001/chrome/common/chrome_c... chrome/common/chrome_content_client.cc:105: static bool IsWidevineAvailable(base::FilePath* adapter_path, You don't need to decl functions static inside an anonymous namespace. https://codereview.chromium.org/1637083002/diff/240001/chrome/common/chrome_c... chrome/common/chrome_content_client.cc:108: static bool skip_widevine_cdm_file_check = false; Questioning the original code - is the intention to do the check only once, and then use the cached result in all future calls? As is, if Widevine is not available on the first call, this code keeps checking on subsequent calls.
https://codereview.chromium.org/1637083002/diff/220001/content/public/browser... File content/public/browser/cdm_service.h (right): https://codereview.chromium.org/1637083002/diff/220001/content/public/browser... content/public/browser/cdm_service.h:34: On 2016/04/19 23:20:39, jrummell wrote: > On 2016/04/18 21:24:21, sievers wrote: > > Hmm it's a bit confusing that there are two APIs to add CDMs (this one and the > > ContentClient callback). Why do we need both? > > Widevine (the only CDM supported with Chrome) is updated via the component > updater on Win/Mac, bundled on Linux, and included as part of Android and Cast. > So we need a way for embedders to register CDMs as well as the component > updater. But I'm wondering why you need two public APIs. It's just a bit confusing,
Updated. > But I'm wondering why you need two public APIs. It's just a bit confusing. I'm just following the existing pattern used by pepper plugins (e.g. ContentClient::AddPepperPlugins() and PluginService::RegisterInternalPlugin()). Is there a better/different way I should be doing this? https://codereview.chromium.org/1637083002/diff/240001/chrome/browser/compone... File chrome/browser/component_updater/widevine_cdm_component_installer.cc (right): https://codereview.chromium.org/1637083002/diff/240001/chrome/browser/compone... chrome/browser/component_updater/widevine_cdm_component_installer.cc:194: if (manifest.GetString(kCdmCodecsListName, &codecs)) { On 2016/04/19 23:46:17, Lei Zhang wrote: > GetStringASCII(), then in RegisterWidevineCdmWithChrome() omit the > UTF16ToASCII(codecs), and add ASCIIToUTF16() to MakeWidevineCdmPluginInfo(). > > This skips one round of conversations because GetString() internally has to > convert to string16. Done. https://codereview.chromium.org/1637083002/diff/240001/chrome/common/chrome_c... File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/1637083002/diff/240001/chrome/common/chrome_c... chrome/common/chrome_content_client.cc:76: #if defined(WIDEVINE_CDM_AVAILABLE) && defined(ENABLE_PEPPER_CDMS) && \ On 2016/04/19 23:46:17, Lei Zhang wrote: > Do you want to #define something here and use that below, instead of writing out > this long #if statement over and over? Done. https://codereview.chromium.org/1637083002/diff/240001/chrome/common/chrome_c... chrome/common/chrome_content_client.cc:105: static bool IsWidevineAvailable(base::FilePath* adapter_path, On 2016/04/19 23:46:17, Lei Zhang wrote: > You don't need to decl functions static inside an anonymous namespace. Done. https://codereview.chromium.org/1637083002/diff/240001/chrome/common/chrome_c... chrome/common/chrome_content_client.cc:108: static bool skip_widevine_cdm_file_check = false; On 2016/04/19 23:46:17, Lei Zhang wrote: > Questioning the original code - is the intention to do the check only once, and > then use the cached result in all future calls? > > As is, if Widevine is not available on the first call, this code keeps checking > on subsequent calls. Back when this was added (https://chromiumcodereview.appspot.com/11099059) using a static bool was the pattern that all the plugin checks used. Reading the comments for PDF ("Once we're sandboxed, we can't know if the PDF plugin is available or not; but (on Linux) this function is always called once before we're sandboxed.") it seems that it shouldn't do the PathExists() check at all the second time. I'll fix this in a subsequent CL.
chrome/ lgtm
lgtm
The CQ bit was checked by jrummell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org, sorin@chromium.org Link to the patchset: https://codereview.chromium.org/1637083002/#ps260001 (title: "changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1637083002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1637083002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jrummell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, sorin@chromium.org, xhwang@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/1637083002/#ps280001 (title: "rebase + add default copy constructor")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1637083002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1637083002/280001
Message was sent while issue was closed.
Description was changed from ========== Add CdmService to keep track of CDMs As media moves to a mojo world, CDMs will be loaded directly into the appropriate process, rather than loaded as pepper plugins. This change adds a class to keep track of the available CDMs. On Win/Mac, the Widevine CDM is updated by the component updater, so it registers (or re-registers) the CDM as it becomes available. On Linux it will be registered if the library is available. Future work: - preload the libraries into whatever process will be used to host the CDM. - keep track of the CDMs that are loaded, so that they are only initialized once. - allow use of External Clear Key CDM for testing. It will be registered if requested using a command line option. BUG=550664 TEST=new content_unittests pass ========== to ========== Add CdmService to keep track of CDMs As media moves to a mojo world, CDMs will be loaded directly into the appropriate process, rather than loaded as pepper plugins. This change adds a class to keep track of the available CDMs. On Win/Mac, the Widevine CDM is updated by the component updater, so it registers (or re-registers) the CDM as it becomes available. On Linux it will be registered if the library is available. Future work: - preload the libraries into whatever process will be used to host the CDM. - keep track of the CDMs that are loaded, so that they are only initialized once. - allow use of External Clear Key CDM for testing. It will be registered if requested using a command line option. BUG=550664 TEST=new content_unittests pass ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Add CdmService to keep track of CDMs As media moves to a mojo world, CDMs will be loaded directly into the appropriate process, rather than loaded as pepper plugins. This change adds a class to keep track of the available CDMs. On Win/Mac, the Widevine CDM is updated by the component updater, so it registers (or re-registers) the CDM as it becomes available. On Linux it will be registered if the library is available. Future work: - preload the libraries into whatever process will be used to host the CDM. - keep track of the CDMs that are loaded, so that they are only initialized once. - allow use of External Clear Key CDM for testing. It will be registered if requested using a command line option. BUG=550664 TEST=new content_unittests pass ========== to ========== Add CdmService to keep track of CDMs As media moves to a mojo world, CDMs will be loaded directly into the appropriate process, rather than loaded as pepper plugins. This change adds a class to keep track of the available CDMs. On Win/Mac, the Widevine CDM is updated by the component updater, so it registers (or re-registers) the CDM as it becomes available. On Linux it will be registered if the library is available. Future work: - preload the libraries into whatever process will be used to host the CDM. - keep track of the CDMs that are loaded, so that they are only initialized once. - allow use of External Clear Key CDM for testing. It will be registered if requested using a command line option. BUG=550664 TEST=new content_unittests pass Committed: https://crrev.com/c82f868e10e87ec7394ea7c0663bfbf679677d8d Cr-Commit-Position: refs/heads/master@{#389036} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/c82f868e10e87ec7394ea7c0663bfbf679677d8d Cr-Commit-Position: refs/heads/master@{#389036} |
