Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1427183002:
Move MediaDrmBridge provision communication to native side. (Closed)
|
Created:
5 years, 1 month ago by Tima Vaisburd Modified:
5 years, 1 month ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, avayvod+watch_chromium.org, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, mlamouri+watch-media_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove MediaDrmBridge provision communication to native side.
MediaDrmBridge currently uses Android's URLConnection and
AsyncTask to communicate with provisioning server. URLConnection
bypasses Chrome HTTP stack , AsyncTask requires the UI thread.
This CL moves the communication to C++ side and uses a helper
class ProvisionFetcher that encapsulate the provisioning.
The MediaDrmBridge takes this ProvisionFetcher as a paremeter.
BUG=546092
Committed: https://crrev.com/b7a0b9a3e5f55858148fdb1e36a4b0421a22467b
Cr-Commit-Position: refs/heads/master@{#359923}
Patch Set 1 #
Total comments: 43
Patch Set 2 : For on-demand provisioning the fetcher is created in content/ and depends on the renderer. #
Total comments: 68
Patch Set 3 : Rebased #Patch Set 4 : Chromecast compilation fix, added missing license #Patch Set 5 : Rebased and lost some functionality. To be added in the next PS. #Patch Set 6 : Simplified since we have an AndroidCdmFactory per render pid and credentials manager in chrome/ #
Total comments: 1
Patch Set 7 : Addressed more comments #
Total comments: 31
Patch Set 8 : AndroidCdmFactory use a fetcher factory instead of a fetcher; addressed more comments #
Total comments: 19
Patch Set 9 : Bound network context into callback function instead of using factory #
Total comments: 20
Patch Set 10 : Rebased, addressed comments #Patch Set 11 : Fixed mojo compilation #Patch Set 12 : Android Mojo client uses dummy provision fetcher instead of no fetcher at all #Patch Set 13 : Reformatted media/ #Messages
Total messages: 30 (8 generated)
timav@chromium.org changed reviewers: + qinmin@chromium.org, xhwang@chromium.org
PTAL. I'm still testing this though.
A gentle ping?
https://codereview.chromium.org/1427183002/diff/1/media/base/android/media_dr... File media/base/android/media_drm_bridge.cc (right): https://codereview.chromium.org/1427183002/diff/1/media/base/android/media_dr... media/base/android/media_drm_bridge.cc:813: provision_fetcher_->Retrieve( Is it ok to use the UI thread for network request? You probably need to use the IO thread, https://codereview.chromium.org/1427183002/diff/1/media/base/android/provisio... File media/base/android/provision_fetcher.h (right): https://codereview.chromium.org/1427183002/diff/1/media/base/android/provisio... media/base/android/provision_fetcher.h:30: // MediaDrm.ProvisionRequest. In case of an error ResponseCallback Fix the comments, seems something is missing here. https://codereview.chromium.org/1427183002/diff/1/media/base/android/provisio... media/base/android/provision_fetcher.h:35: ResponseCallback cb) = 0; const ResponseCallback&
We've always wanted to make this change. It's so nice to see it's happening! Here are my comments and nits. https://chromiumcodereview.appspot.com/1427183002/diff/1/chrome/browser/chrom... File chrome/browser/chrome_browser_main_android.cc (right): https://chromiumcodereview.appspot.com/1427183002/diff/1/chrome/browser/chrom... chrome/browser/chrome_browser_main_android.cc:130: media::SetMediaClientAndroid(new ChromeMediaClientAndroid); Wondering whether we can merge these two by putting the ProvisionFether in the MediaClientAndroid interface. For example, add a factory function in MediaClientAndroid: scoped_ptr<ProvisionFetcher> CreateProvisionFetcher(); Or probably just add a call in MediaClientAndroid: RequestProvision(const std::string& default_url, const std::string& request_data, ResponseCallback cb); https://chromiumcodereview.appspot.com/1427183002/diff/1/chrome/browser/media... File chrome/browser/media/android/provision/url_provision_fetcher.cc (right): https://chromiumcodereview.appspot.com/1427183002/diff/1/chrome/browser/media... chrome/browser/media/android/provision/url_provision_fetcher.cc:31: DVLOG(1) << __FUNCTION__ << ": request:" << os.str(); You'll end up with "os" not used in release build. How about just: DVLOG(1) << __FUNCTION__ << ": request:" << default_url << "&signedRequest=" << request_data; Also, maybe just replace l.25 with l.31? https://chromiumcodereview.appspot.com/1427183002/diff/1/chrome/browser/media... chrome/browser/media/android/provision/url_provision_fetcher.cc:52: DVLOG(1) << __FUNCTION__ << ": response code:" << source->GetResponseCode(); Can you replace l.47 with this line? https://chromiumcodereview.appspot.com/1427183002/diff/1/chrome/browser/media... File chrome/browser/media/android/provision/url_provision_fetcher.h (right): https://chromiumcodereview.appspot.com/1427183002/diff/1/chrome/browser/media... chrome/browser/media/android/provision/url_provision_fetcher.h:26: ResponseCallback cb) override; nit, see below: s/ResponseCallback cb/const ResponseCB& response_cb/ https://chromiumcodereview.appspot.com/1427183002/diff/1/content/browser/medi... File content/browser/media/android/media_drm_credential_manager.cc (right): https://chromiumcodereview.appspot.com/1427183002/diff/1/content/browser/medi... content/browser/media/android/media_drm_credential_manager.cc:99: kWidevineKeySystem, provision_fetcher.Pass()); Can we create the ProvisionFetcher directly in MDB? Then we can create it only when we need it. https://chromiumcodereview.appspot.com/1427183002/diff/1/media/base/android/b... File media/base/android/browser_cdm_factory_android.cc (right): https://chromiumcodereview.appspot.com/1427183002/diff/1/media/base/android/b... media/base/android/browser_cdm_factory_android.cc:34: key_system, provision_fetcher.Pass(), session_message_cb, ditto, can we create ProvisionFetcher in MDB so we can do lazy creation? https://chromiumcodereview.appspot.com/1427183002/diff/1/media/base/android/b... File media/base/android/browser_cdm_factory_android.h (right): https://chromiumcodereview.appspot.com/1427183002/diff/1/media/base/android/b... media/base/android/browser_cdm_factory_android.h:13: class URLRequestContextGetter; Why do we need this here? https://chromiumcodereview.appspot.com/1427183002/diff/1/media/base/android/j... File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java (left): https://chromiumcodereview.appspot.com/1427183002/diff/1/media/base/android/j... media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:22: import java.net.URL; wow, so happy to see this change! Thanks! https://chromiumcodereview.appspot.com/1427183002/diff/1/media/base/android/j... media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:93: private boolean mResetDeviceCredentialsPending; It's probably better to keep this logic in Java. See my comment below. https://chromiumcodereview.appspot.com/1427183002/diff/1/media/base/android/j... File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java (right): https://chromiumcodereview.appspot.com/1427183002/diff/1/media/base/android/j... media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:522: private void processPendingCreateSessionData() { Why are you exposing this to the native code? Can't we call this in provideProvisionResponse()? In principle, I'd like to keep the JNI layer as simple as possible. https://chromiumcodereview.appspot.com/1427183002/diff/1/media/base/android/m... File media/base/android/media_drm_bridge.cc (right): https://chromiumcodereview.appspot.com/1427183002/diff/1/media/base/android/m... media/base/android/media_drm_bridge.cc:502: DVLOG(1) << "MediaDrmBridge::" << __FUNCTION__; nit: In other places we don't use the "MediaDrmBridge" part. https://chromiumcodereview.appspot.com/1427183002/diff/1/media/base/android/m... media/base/android/media_drm_bridge.cc:819: void MediaDrmBridge::ProcessProvisioningResult(const std::string& response, nit: s/ProvisioningResult/ProvisionResponse https://chromiumcodereview.appspot.com/1427183002/diff/1/media/base/android/m... media/base/android/media_drm_bridge.cc:839: if (!reset_credentials_cb_.is_null()) { We can also fire reset_credentials_cb_ in OnResetDeviceCredentialsCompleted(). IMHO, having multiple ways to do one thing makes the code complicated. Can we keep ProcessProvisioningResult() simple by just providing the response to the Java code, then Java code can decide whether to call OnResetDeviceCredentialsCompleted(), and/or process pending create session data (below)? https://chromiumcodereview.appspot.com/1427183002/diff/1/media/base/android/p... File media/base/android/provision_fetcher.cc (right): https://chromiumcodereview.appspot.com/1427183002/diff/1/media/base/android/p... media/base/android/provision_fetcher.cc:10: ProvisionFetcherFactory* g_factory = nullptr; If we use MediaClientAndroid, we don't need the factory and the global pointer here. https://chromiumcodereview.appspot.com/1427183002/diff/1/media/base/android/p... File media/base/android/provision_fetcher.h (right): https://chromiumcodereview.appspot.com/1427183002/diff/1/media/base/android/p... media/base/android/provision_fetcher.h:22: using ResponseCallback = nit: in media/ code, we typically use CB for Callback, so this would be ResponseCB. https://chromiumcodereview.appspot.com/1427183002/diff/1/media/base/android/p... media/base/android/provision_fetcher.h:23: base::Callback<void(const std::string& response, bool success)>; nit: usually we put the flag (success) as the first parameter. https://chromiumcodereview.appspot.com/1427183002/diff/1/media/base/android/p... media/base/android/provision_fetcher.h:31: // The implementation is suppoed to talk to a provision server that s/suppoed/supposed s/provision server/provisioning server https://chromiumcodereview.appspot.com/1427183002/diff/1/media/base/android/p... media/base/android/provision_fetcher.h:35: ResponseCallback cb) = 0; nit: s/cb/response_cb https://chromiumcodereview.appspot.com/1427183002/diff/1/media/base/android/p... media/base/android/provision_fetcher.h:35: ResponseCallback cb) = 0; Can we guarantee that the |cb| is always fired asynchronously on the same thread Retrieve() is called? If so, we can make a comment here. Then the caller (MDB) doesn't need to use BindToCurrentLoop.
Patchset #2 (id:20001) has been deleted
Please take another look. I split the ProvisionFetcher creation into two paths: one comes from BrowserCdmManager and is common for chrome, cast and AndroidWebview (I think), another comes from Chrome menu and is specific to Chrome (this is ResetCredentials). For the first path I tried to keep the ProvisionFetcher creation completely within the content/, and the URL context is now corresponds to the content. The second path goes through ChromeMediaClientAndroid. https://codereview.chromium.org/1427183002/diff/1/chrome/browser/chrome_brows... File chrome/browser/chrome_browser_main_android.cc (right): https://codereview.chromium.org/1427183002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main_android.cc:130: media::SetMediaClientAndroid(new ChromeMediaClientAndroid); On 2015/11/02 20:37:04, xhwang wrote: > Wondering whether we can merge these two by putting the ProvisionFether in the > MediaClientAndroid interface. > > For example, add a factory function in MediaClientAndroid: > > scoped_ptr<ProvisionFetcher> CreateProvisionFetcher(); > > Or probably just add a call in MediaClientAndroid: > > RequestProvision(const std::string& default_url, > const std::string& request_data, > ResponseCallback cb); MediaClientAndroid now has the method CreateDefaultFetcher() which uses the last active user context and is designed for the Chrome's ResetCredentials. The fetcher for the second path (on-demand provisioning in MediaDrmBridge) happens completely within content/. https://codereview.chromium.org/1427183002/diff/1/chrome/browser/media/androi... File chrome/browser/media/android/provision/url_provision_fetcher.cc (right): https://codereview.chromium.org/1427183002/diff/1/chrome/browser/media/androi... chrome/browser/media/android/provision/url_provision_fetcher.cc:31: DVLOG(1) << __FUNCTION__ << ": request:" << os.str(); On 2015/11/02 20:37:04, xhwang wrote: > You'll end up with "os" not used in release build. How about just: > > DVLOG(1) << __FUNCTION__ << ": request:" << default_url << "&signedRequest=" << > request_data; |os| is used on l.33 ? > Also, maybe just replace l.25 with l.31? Done https://codereview.chromium.org/1427183002/diff/1/chrome/browser/media/androi... chrome/browser/media/android/provision/url_provision_fetcher.cc:52: DVLOG(1) << __FUNCTION__ << ": response code:" << source->GetResponseCode(); On 2015/11/02 20:37:04, xhwang wrote: > Can you replace l.47 with this line? Done. https://codereview.chromium.org/1427183002/diff/1/chrome/browser/media/androi... File chrome/browser/media/android/provision/url_provision_fetcher.h (right): https://codereview.chromium.org/1427183002/diff/1/chrome/browser/media/androi... chrome/browser/media/android/provision/url_provision_fetcher.h:26: ResponseCallback cb) override; On 2015/11/02 20:37:04, xhwang wrote: > nit, see below: s/ResponseCallback cb/const ResponseCB& response_cb/ Done. https://codereview.chromium.org/1427183002/diff/1/content/browser/media/andro... File content/browser/media/android/media_drm_credential_manager.cc (right): https://codereview.chromium.org/1427183002/diff/1/content/browser/media/andro... content/browser/media/android/media_drm_credential_manager.cc:99: kWidevineKeySystem, provision_fetcher.Pass()); On 2015/11/02 20:37:04, xhwang wrote: > Can we create the ProvisionFetcher directly in MDB? Then we can create it only > when we need it. Done for this path, but not for the other. https://codereview.chromium.org/1427183002/diff/1/media/base/android/browser_... File media/base/android/browser_cdm_factory_android.cc (right): https://codereview.chromium.org/1427183002/diff/1/media/base/android/browser_... media/base/android/browser_cdm_factory_android.cc:34: key_system, provision_fetcher.Pass(), session_message_cb, On 2015/11/02 20:37:04, xhwang wrote: > ditto, can we create ProvisionFetcher in MDB so we can do lazy creation? For this path |provision_fetcher| is now created in the BrowserCdmManager and passed to this method as a parameter. https://codereview.chromium.org/1427183002/diff/1/media/base/android/browser_... File media/base/android/browser_cdm_factory_android.h (right): https://codereview.chromium.org/1427183002/diff/1/media/base/android/browser_... media/base/android/browser_cdm_factory_android.h:13: class URLRequestContextGetter; On 2015/11/02 20:37:04, xhwang wrote: > Why do we need this here? Removed. https://codereview.chromium.org/1427183002/diff/1/media/base/android/java/src... File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java (left): https://codereview.chromium.org/1427183002/diff/1/media/base/android/java/src... media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:93: private boolean mResetDeviceCredentialsPending; On 2015/11/02 20:37:05, xhwang wrote: > It's probably better to keep this logic in Java. See my comment below. Done. https://codereview.chromium.org/1427183002/diff/1/media/base/android/java/src... File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java (right): https://codereview.chromium.org/1427183002/diff/1/media/base/android/java/src... media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:522: private void processPendingCreateSessionData() { On 2015/11/02 20:37:05, xhwang wrote: > Why are you exposing this to the native code? Can't we call this in > provideProvisionResponse()? > > In principle, I'd like to keep the JNI layer as simple as possible. Done. https://codereview.chromium.org/1427183002/diff/1/media/base/android/media_dr... File media/base/android/media_drm_bridge.cc (right): https://codereview.chromium.org/1427183002/diff/1/media/base/android/media_dr... media/base/android/media_drm_bridge.cc:502: DVLOG(1) << "MediaDrmBridge::" << __FUNCTION__; On 2015/11/02 20:37:05, xhwang wrote: > nit: In other places we don't use the "MediaDrmBridge" part. Class name removed. https://codereview.chromium.org/1427183002/diff/1/media/base/android/media_dr... media/base/android/media_drm_bridge.cc:813: provision_fetcher_->Retrieve( On 2015/11/02 19:39:14, qinmin wrote: > Is it ok to use the UI thread for network request? > You probably need to use the IO thread, I think URLFetcher::Start() does it by itself: https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur... https://codereview.chromium.org/1427183002/diff/1/media/base/android/media_dr... media/base/android/media_drm_bridge.cc:819: void MediaDrmBridge::ProcessProvisioningResult(const std::string& response, On 2015/11/02 20:37:05, xhwang wrote: > nit: s/ProvisioningResult/ProvisionResponse Done. https://codereview.chromium.org/1427183002/diff/1/media/base/android/media_dr... media/base/android/media_drm_bridge.cc:839: if (!reset_credentials_cb_.is_null()) { On 2015/11/02 20:37:05, xhwang wrote: > We can also fire reset_credentials_cb_ in OnResetDeviceCredentialsCompleted(). > IMHO, having multiple ways to do one thing makes the code complicated. > > Can we keep ProcessProvisioningResult() simple by just providing the response to > the Java code, then Java code can decide whether to call > OnResetDeviceCredentialsCompleted(), and/or process pending create session data > (below)? Done. https://codereview.chromium.org/1427183002/diff/1/media/base/android/provisio... File media/base/android/provision_fetcher.cc (right): https://codereview.chromium.org/1427183002/diff/1/media/base/android/provisio... media/base/android/provision_fetcher.cc:10: ProvisionFetcherFactory* g_factory = nullptr; On 2015/11/02 20:37:05, xhwang wrote: > If we use MediaClientAndroid, we don't need the factory and the global pointer > here. They are completely removed. However, I added a new factory method content::CDMProvisionFetcher::CreateWithURLContext(). https://codereview.chromium.org/1427183002/diff/1/media/base/android/provisio... File media/base/android/provision_fetcher.h (right): https://codereview.chromium.org/1427183002/diff/1/media/base/android/provisio... media/base/android/provision_fetcher.h:22: using ResponseCallback = On 2015/11/02 20:37:05, xhwang wrote: > nit: in media/ code, we typically use CB for Callback, so this would be > ResponseCB. Done. https://codereview.chromium.org/1427183002/diff/1/media/base/android/provisio... media/base/android/provision_fetcher.h:23: base::Callback<void(const std::string& response, bool success)>; On 2015/11/02 20:37:05, xhwang wrote: > nit: usually we put the flag (success) as the first parameter. Done. https://codereview.chromium.org/1427183002/diff/1/media/base/android/provisio... media/base/android/provision_fetcher.h:30: // MediaDrm.ProvisionRequest. In case of an error ResponseCallback On 2015/11/02 19:39:14, qinmin wrote: > Fix the comments, seems something is missing here. Done. https://codereview.chromium.org/1427183002/diff/1/media/base/android/provisio... media/base/android/provision_fetcher.h:31: // The implementation is suppoed to talk to a provision server that On 2015/11/02 20:37:05, xhwang wrote: > s/suppoed/supposed > > s/provision server/provisioning server I removed talks about implementation here. https://codereview.chromium.org/1427183002/diff/1/media/base/android/provisio... media/base/android/provision_fetcher.h:35: ResponseCallback cb) = 0; On 2015/11/02 19:39:14, qinmin wrote: > const ResponseCallback& Done. https://codereview.chromium.org/1427183002/diff/1/media/base/android/provisio... media/base/android/provision_fetcher.h:35: ResponseCallback cb) = 0; On 2015/11/02 20:37:05, xhwang wrote: > nit: s/cb/response_cb Done. https://codereview.chromium.org/1427183002/diff/1/media/base/android/provisio... media/base/android/provision_fetcher.h:35: ResponseCallback cb) = 0; On 2015/11/02 20:37:05, xhwang wrote: > nit: s/cb/response_cb Done. https://codereview.chromium.org/1427183002/diff/40001/content/browser/media/c... File content/browser/media/cdm/browser_cdm_manager.cc (right): https://codereview.chromium.org/1427183002/diff/40001/content/browser/media/c... content/browser/media/cdm/browser_cdm_manager.cc:558: key_system, use_hw_secure_codecs, provision_fetcher.Pass(), I pass |provision_fetcher| here as a parameter, and so I had to move the ProvisionFetcher definition to media/base/, although it is used only for Android. I wonder whether there is a better way.
https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... File content/browser/media/cdm/browser_cdm_manager.cc (right): https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/cdm/browser_cdm_manager.cc:558: key_system, use_hw_secure_codecs, provision_fetcher.Pass(), On 2015/11/05 02:24:07, Tima Vaisburd wrote: > I pass |provision_fetcher| here as a parameter, and so I had to move the > ProvisionFetcher definition to media/base/, although it is used only for > Android. I wonder whether there is a better way. An alternative could be adding another factory method in MediaClientAndroid, e.g. CreateFetcherForRenderProcess(int id) and implementing it in every subclass of MediaClientAndroid. WDYT?
My comments are on PS2. Sorry about that. I'll look at your latest PS to see whether I have extra comments. https://chromiumcodereview.appspot.com/1427183002/diff/40001/chrome/browser/a... File chrome/browser/android/chrome_media_client_android.cc (right): https://chromiumcodereview.appspot.com/1427183002/diff/40001/chrome/browser/a... chrome/browser/android/chrome_media_client_android.cc:22: Profile* profile = ProfileManager::GetActiveUserProfile(); The call GetActiveUserProfile() is what I am concerned about. See offline bug/email. https://chromiumcodereview.appspot.com/1427183002/diff/40001/chrome/browser/a... File chrome/browser/android/chrome_media_client_android.h (right): https://chromiumcodereview.appspot.com/1427183002/diff/40001/chrome/browser/a... chrome/browser/android/chrome_media_client_android.h:18: scoped_ptr<media::ProvisionFetcher> CreateDefaultFetcher() const override; naming nit: CreateProvisionFetcher would be more explicit. But here we'll always use the active user's profile, which could be easily misused when handling requests from other profiles. Since this is only used by MediaDrmCredentialManager, and MediaDrmCredentialManager is only used by chrome/. Can we just move MediaDrmCredentialManager to chrome/ to avoid exposing at all? https://chromiumcodereview.appspot.com/1427183002/diff/40001/chromecast/brows... File chromecast/browser/media/cast_browser_cdm_factory.h (right): https://chromiumcodereview.appspot.com/1427183002/diff/40001/chromecast/brows... chromecast/browser/media/cast_browser_cdm_factory.h:26: scoped_ptr<ProvisionFetcher> provision_fetcher, I am changing this file so here it will override CdmFactory::Create(). CdmFactory::Create() is generic and we should not have ProvisionFetcher here, which is Android specific. How about passing ProvisionFetcher in the AndroidCdmFactory's constructor, and pass it to MediaDrmBridge in Create()? https://chromiumcodereview.appspot.com/1408793009/diff/40001/media/base/andro... https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... File content/browser/media/android/url_provision_fetcher.cc (right): https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/android/url_provision_fetcher.cc:25: const media::ProvisionFetcher::ResponseCB& response_cb) { I suppose we can't make multiple requests? How about DCHECK(!request_); https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/android/url_provision_fetcher.cc:26: response_cb_ = media::BindToCurrentLoop(response_cb); Will Retrieve() and OnURLFetchComplete() be called on different threads? If so, we should do thread checking instead of BindToCurrentLoop. If not, we should comment on it... https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/android/url_provision_fetcher.cc:29: os << default_url << "&signedRequest=" << request_data; Is this "signedRequest" part documented anywhere? https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/android/url_provision_fetcher.cc:32: request_ = URLFetcher::Create(GURL(os.str()), URLFetcher::POST, this); sorry for the bikeshedding... I think we use streams mostly for logging purposes: https://google.github.io/styleguide/cppguide.html#Streams Though it's not banned for other use cases like this one, it's pretty rare to use it in Chromium other than logging and tests. For example, you only have ostringstream in 11 files in base/, and most of them are for logging and tests: https://code.google.com/p/chromium/codesearch#search/&q=ostringstream%20file:... Also os.str() will return a copy of the string anyways, I don't see why we prefer it to normal string in terms of performance? Can we simply use std::string? https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/android/url_provision_fetcher.cc:56: DVLOG(1) << __FUNCTION__ << ": GetResponseAsString() failed"; nit: Use DVLOG_IF https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/android/url_provision_fetcher.cc:70: scoped_ptr<media::ProvisionFetcher> CDMProvisionFetcher::CreateWithURLContext( Why do we need CDMProvisionFetcher? https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... File content/browser/media/android/url_provision_fetcher.h (right): https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/android/url_provision_fetcher.h:5: #ifndef CONTENT_BROWSER_MEDIA_ANDROID_URL_PROVISION_FETCHER_H_ tip: when moving file around, first upload a PS that only does the move, then upload a PS with real changes. This way it's easy to see what has been updated since last PS. https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/android/url_provision_fetcher.h:17: class URLProvisionFetcher : public media::ProvisionFetcher, nit: All ProvisionFetch will use a URL, so it seems odd that this particular one is a URLProvisionFetcher... How about just something like ProvisionFetcherImpl? https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/android/url_provision_fetcher.h:18: private net::URLFetcherDelegate { style: We don't use private inheritance. https://google.github.io/styleguide/cppguide.html#Inheritance https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/android/url_provision_fetcher.h:26: const media::ProvisionFetcher::ResponseCB& cb) override; s/media::ProvisionFetcher::// since |this| is a media::ProvisionFetcher? https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/android/url_provision_fetcher.h:26: const media::ProvisionFetcher::ResponseCB& cb) override; nit: cb/response_cb/ https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/android/url_provision_fetcher.h:47: */ remove https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... File content/browser/media/cdm/browser_cdm_manager.cc (right): https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/cdm/browser_cdm_manager.cc:551: provision_fetcher = CDMProvisionFetcher::CreateWithURLContext(context); Can you just create a URLProvisionFetch directly here, without going through CDMProvisionFetcher? https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/cdm/browser_cdm_manager.cc:558: key_system, use_hw_secure_codecs, provision_fetcher.Pass(), On 2015/11/05 02:24:07, Tima Vaisburd wrote: > I pass |provision_fetcher| here as a parameter, and so I had to move the > ProvisionFetcher definition to media/base/, although it is used only for > Android. I wonder whether there is a better way. See comments above. ProvisionFetcher is only used by MDB, we probably still want to keep it in media/base/android. See my comment above, we don't want this parameter in Create() call, which is used for all CDM creations. https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/public/b... File content/public/browser/android/cdm_provision_fetcher.h (right): https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/public/b... content/public/browser/android/cdm_provision_fetcher.h:19: class CONTENT_EXPORT CDMProvisionFetcher { See comments above. Not sure why we need this class... https://chromiumcodereview.appspot.com/1427183002/diff/40001/media/base/andro... File media/base/android/browser_cdm_factory_android.h (right): https://chromiumcodereview.appspot.com/1427183002/diff/40001/media/base/andro... media/base/android/browser_cdm_factory_android.h:22: scoped_ptr<ProvisionFetcher> provision_fetcher, See comment above. ProvisionFetcher is specific to MDB, and should not be passed to all CDM creation. https://chromiumcodereview.appspot.com/1427183002/diff/40001/media/base/andro... File media/base/android/media_client_android.h (right): https://chromiumcodereview.appspot.com/1427183002/diff/40001/media/base/andro... media/base/android/media_client_android.h:45: virtual scoped_ptr<ProvisionFetcher> CreateDefaultFetcher() const; See comment above. If we move the ResetCredentialManager to chrome/ we don't need this. https://chromiumcodereview.appspot.com/1427183002/diff/40001/media/base/andro... File media/base/android/media_drm_bridge.cc (right): https://chromiumcodereview.appspot.com/1427183002/diff/40001/media/base/andro... media/base/android/media_drm_bridge.cc:833: env, (uint8_t*)response.data(), response.size()); static_cast instead of C-style cast. https://chromiumcodereview.appspot.com/1427183002/diff/40001/media/base/andro... File media/base/android/media_drm_bridge.h (right): https://chromiumcodereview.appspot.com/1427183002/diff/40001/media/base/andro... media/base/android/media_drm_bridge.h:91: static ScopedMediaDrmBridgePtr CreateWithoutSessionSupport( tip: You'll need to do a rebase. Please keep the rebase PS separate from other PS. For example, you can upload a rebase-only PS, then make your modifications and upload another PS. https://chromiumcodereview.appspot.com/1427183002/diff/40001/media/base/provi... File media/base/provision_fetcher.h (right): https://chromiumcodereview.appspot.com/1427183002/diff/40001/media/base/provi... media/base/provision_fetcher.h:8: #include <string> style nit: one empty line here https://chromiumcodereview.appspot.com/1427183002/diff/40001/media/base/provi... media/base/provision_fetcher.h:10: #include "base/memory/scoped_ptr.h" not used? https://chromiumcodereview.appspot.com/1427183002/diff/40001/media/base/provi... media/base/provision_fetcher.h:30: // The implementation must call |cb| on the same thread that this method nit: s/cb/response_cb Also, the callback should be called asynchronously. https://chromiumcodereview.appspot.com/1427183002/diff/40001/media/base/provi... media/base/provision_fetcher.h:35: }; DISALLOW_COPY_AND_ASSIGN
I did the changes we discussed off-line, but there are some questions. Please take another look. https://chromiumcodereview.appspot.com/1427183002/diff/40001/chrome/browser/a... File chrome/browser/android/chrome_media_client_android.cc (right): https://chromiumcodereview.appspot.com/1427183002/diff/40001/chrome/browser/a... chrome/browser/android/chrome_media_client_android.cc:22: Profile* profile = ProfileManager::GetActiveUserProfile(); On 2015/11/06 23:08:18, xhwang wrote: > The call GetActiveUserProfile() is what I am concerned about. See offline > bug/email. Changed to ProfileManager::GetLastusedProfile() (in MediaDrmCredentialManager) https://chromiumcodereview.appspot.com/1427183002/diff/40001/chrome/browser/a... File chrome/browser/android/chrome_media_client_android.h (right): https://chromiumcodereview.appspot.com/1427183002/diff/40001/chrome/browser/a... chrome/browser/android/chrome_media_client_android.h:18: scoped_ptr<media::ProvisionFetcher> CreateDefaultFetcher() const override; On 2015/11/06 23:08:18, xhwang wrote: > naming nit: CreateProvisionFetcher would be more explicit. > > But here we'll always use the active user's profile, which could be easily > misused when handling requests from other profiles. > > Since this is only used by MediaDrmCredentialManager, and > MediaDrmCredentialManager is only used by chrome/. Can we just move > MediaDrmCredentialManager to chrome/ to avoid exposing at all? Done move. https://chromiumcodereview.appspot.com/1427183002/diff/40001/chromecast/brows... File chromecast/browser/media/cast_browser_cdm_factory.h (right): https://chromiumcodereview.appspot.com/1427183002/diff/40001/chromecast/brows... chromecast/browser/media/cast_browser_cdm_factory.h:26: scoped_ptr<ProvisionFetcher> provision_fetcher, On 2015/11/06 23:08:18, xhwang wrote: > I am changing this file so here it will override CdmFactory::Create(). > CdmFactory::Create() is generic and we should not have ProvisionFetcher here, > which is Android specific. > > How about passing ProvisionFetcher in the AndroidCdmFactory's constructor, and > pass it to MediaDrmBridge in Create()? > > https://chromiumcodereview.appspot.com/1408793009/diff/40001/media/base/andro... I did a similar change, however, can CreateBrowserCdm be called more than once on the same factory object? I assumed that yes, and passed ProvisionFetcher to the factory in BrowserCdmManager::OnInitializeCdm() - see my change there. https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... File content/browser/media/android/url_provision_fetcher.cc (right): https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/android/url_provision_fetcher.cc:25: const media::ProvisionFetcher::ResponseCB& response_cb) { On 2015/11/06 23:08:18, xhwang wrote: > I suppose we can't make multiple requests? How about > > DCHECK(!request_); Done. https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/android/url_provision_fetcher.cc:29: os << default_url << "&signedRequest=" << request_data; On 2015/11/06 23:08:18, xhwang wrote: > Is this "signedRequest" part documented anywhere? This is what the java code did. The only doc I found is this: http://developer.android.com/reference/android/media/MediaDrm.ProvisionReques... https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/android/url_provision_fetcher.cc:32: request_ = URLFetcher::Create(GURL(os.str()), URLFetcher::POST, this); On 2015/11/06 23:08:18, xhwang wrote: > sorry for the bikeshedding... > > I think we use streams mostly for logging purposes: > > https://google.github.io/styleguide/cppguide.html#Streams > > Though it's not banned for other use cases like this one, it's pretty rare to > use it in Chromium other than logging and tests. For example, you only have > ostringstream in 11 files in base/, and most of them are for logging and tests: > > https://code.google.com/p/chromium/codesearch#search/&q=ostringstream%20file:... > > Also os.str() will return a copy of the string anyways, I don't see why we > prefer it to normal string in terms of performance? > > Can we simply use std::string? Yes, we can. I think it would be nice to know what's undesirable or awkward or else wrong in this use of streams before I would try to avoid them, because I haven't seen the argument against them so far (except that they might be slow, but comparison was with the printf and still I did not see the numbers). I think the argument of current usage, or habits, would not completely answer this question. Performance can be an argument here, but as far as I get this operation will be rare? https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/android/url_provision_fetcher.cc:56: DVLOG(1) << __FUNCTION__ << ": GetResponseAsString() failed"; On 2015/11/06 23:08:18, xhwang wrote: > nit: Use DVLOG_IF Done. https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/android/url_provision_fetcher.cc:70: scoped_ptr<media::ProvisionFetcher> CDMProvisionFetcher::CreateWithURLContext( On 2015/11/06 23:08:18, xhwang wrote: > Why do we need CDMProvisionFetcher? According to the current dependency rules chrome/ can include files only from content/public. This is an implementation of the class defined in content/public/. Shall we change the DEPS instead? https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... File content/browser/media/android/url_provision_fetcher.h (right): https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/android/url_provision_fetcher.h:5: #ifndef CONTENT_BROWSER_MEDIA_ANDROID_URL_PROVISION_FETCHER_H_ On 2015/11/06 23:08:18, xhwang wrote: > tip: when moving file around, first upload a PS that only does the move, then > upload a PS with real changes. This way it's easy to see what has been updated > since last PS. Acknowledged. https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/android/url_provision_fetcher.h:17: class URLProvisionFetcher : public media::ProvisionFetcher, On 2015/11/06 23:08:18, xhwang wrote: > nit: All ProvisionFetch will use a URL, so it seems odd that this particular one > is a URLProvisionFetcher... > > How about just something like ProvisionFetcherImpl? I thought we will have another implementation for GPU process. Also, as defined now, the ProvisionFetcher interface does not tell anything about URL or HTTP. I meant, rather, direct HTTP call (versus GPU - Browser relay). We can change it to ProvisionFetcherImpl, especially if there will be just one impl. https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/android/url_provision_fetcher.h:18: private net::URLFetcherDelegate { On 2015/11/06 23:08:18, xhwang wrote: > style: We don't use private inheritance. > > https://google.github.io/styleguide/cppguide.html#Inheritance Just changed to public, but, as the link says, I did mean impl. inheritance. Would you prefer another member? https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/android/url_provision_fetcher.h:47: */ On 2015/11/06 23:08:18, xhwang wrote: > remove Done. https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... File content/browser/media/cdm/browser_cdm_manager.cc (right): https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/cdm/browser_cdm_manager.cc:551: provision_fetcher = CDMProvisionFetcher::CreateWithURLContext(context); On 2015/11/06 23:08:18, xhwang wrote: > Can you just create a URLProvisionFetch directly here, without going through > CDMProvisionFetcher? Done. https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/cdm/browser_cdm_manager.cc:558: key_system, use_hw_secure_codecs, provision_fetcher.Pass(), On 2015/11/06 23:08:19, xhwang wrote: > On 2015/11/05 02:24:07, Tima Vaisburd wrote: > > I pass |provision_fetcher| here as a parameter, and so I had to move the > > ProvisionFetcher definition to media/base/, although it is used only for > > Android. I wonder whether there is a better way. > > See comments above. > > ProvisionFetcher is only used by MDB, we probably still want to keep it in > media/base/android. See my comment above, we don't want this parameter in > Create() call, which is used for all CDM creations. Done. https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/public/b... File content/public/browser/android/cdm_provision_fetcher.h (right): https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/public/b... content/public/browser/android/cdm_provision_fetcher.h:19: class CONTENT_EXPORT CDMProvisionFetcher { On 2015/11/06 23:08:19, xhwang wrote: > See comments above. Not sure why we need this class... See my other comment about current DEPS rules. https://chromiumcodereview.appspot.com/1427183002/diff/40001/media/base/andro... File media/base/android/browser_cdm_factory_android.h (right): https://chromiumcodereview.appspot.com/1427183002/diff/40001/media/base/andro... media/base/android/browser_cdm_factory_android.h:22: scoped_ptr<ProvisionFetcher> provision_fetcher, On 2015/11/06 23:08:19, xhwang wrote: > See comment above. ProvisionFetcher is specific to MDB, and should not be passed > to all CDM creation. Done. https://chromiumcodereview.appspot.com/1427183002/diff/40001/media/base/andro... File media/base/android/media_client_android.h (right): https://chromiumcodereview.appspot.com/1427183002/diff/40001/media/base/andro... media/base/android/media_client_android.h:45: virtual scoped_ptr<ProvisionFetcher> CreateDefaultFetcher() const; On 2015/11/06 23:08:19, xhwang wrote: > See comment above. If we move the ResetCredentialManager to chrome/ we don't > need this. Removed this stuff from MediaClientAndroid and subclasses. https://chromiumcodereview.appspot.com/1427183002/diff/40001/media/base/andro... File media/base/android/media_drm_bridge.cc (right): https://chromiumcodereview.appspot.com/1427183002/diff/40001/media/base/andro... media/base/android/media_drm_bridge.cc:833: env, (uint8_t*)response.data(), response.size()); On 2015/11/06 23:08:19, xhwang wrote: > static_cast instead of C-style cast. reinterpret_cast, signed and unsigned char seems to be considered unrelated types. https://chromiumcodereview.appspot.com/1427183002/diff/40001/media/base/andro... File media/base/android/media_drm_bridge.h (right): https://chromiumcodereview.appspot.com/1427183002/diff/40001/media/base/andro... media/base/android/media_drm_bridge.h:91: static ScopedMediaDrmBridgePtr CreateWithoutSessionSupport( On 2015/11/06 23:08:19, xhwang wrote: > tip: You'll need to do a rebase. Please keep the rebase PS separate from other > PS. For example, you can upload a rebase-only PS, then make your modifications > and upload another PS. Done. https://chromiumcodereview.appspot.com/1427183002/diff/40001/media/base/provi... File media/base/provision_fetcher.h (right): https://chromiumcodereview.appspot.com/1427183002/diff/40001/media/base/provi... media/base/provision_fetcher.h:8: #include <string> On 2015/11/06 23:08:19, xhwang wrote: > style nit: one empty line here Done. https://chromiumcodereview.appspot.com/1427183002/diff/40001/media/base/provi... media/base/provision_fetcher.h:10: #include "base/memory/scoped_ptr.h" On 2015/11/06 23:08:19, xhwang wrote: > not used? Removed https://chromiumcodereview.appspot.com/1427183002/diff/40001/media/base/provi... media/base/provision_fetcher.h:30: // The implementation must call |cb| on the same thread that this method On 2015/11/06 23:08:19, xhwang wrote: > nit: s/cb/response_cb > > Also, the callback should be called asynchronously. Done. https://chromiumcodereview.appspot.com/1427183002/diff/40001/media/base/provi... media/base/provision_fetcher.h:35: }; On 2015/11/06 23:08:19, xhwang wrote: > DISALLOW_COPY_AND_ASSIGN Why do we need to require this here? https://chromiumcodereview.appspot.com/1427183002/diff/120001/content/browser... File content/browser/media/cdm/browser_cdm_manager.cc (right): https://chromiumcodereview.appspot.com/1427183002/diff/120001/content/browser... content/browser/media/cdm/browser_cdm_manager.cc:279: #if defined(OS_ANDROID) I would like to guarantee that on Android we will have AndroidCdmFactory(). Can we make this simplification though?
Some more comments. Note that there are some new comments in PS2. https://chromiumcodereview.appspot.com/1427183002/diff/40001/chromecast/brows... File chromecast/browser/media/cast_browser_cdm_factory.h (right): https://chromiumcodereview.appspot.com/1427183002/diff/40001/chromecast/brows... chromecast/browser/media/cast_browser_cdm_factory.h:26: scoped_ptr<ProvisionFetcher> provision_fetcher, On 2015/11/11 03:03:33, Tima Vaisburd wrote: > On 2015/11/06 23:08:18, xhwang wrote: > > I am changing this file so here it will override CdmFactory::Create(). > > CdmFactory::Create() is generic and we should not have ProvisionFetcher here, > > which is Android specific. > > > > How about passing ProvisionFetcher in the AndroidCdmFactory's constructor, and > > pass it to MediaDrmBridge in Create()? > > > > > https://chromiumcodereview.appspot.com/1408793009/diff/40001/media/base/andro... > > I did a similar change, however, can CreateBrowserCdm be called more than once > on the same factory object? Yes. > I assumed that yes, and passed ProvisionFetcher to > the factory in BrowserCdmManager::OnInitializeCdm() - see my change there. Not sure whether this comment is still valid. See my comment below about passing in a callback that creates the fetcher. https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... File content/browser/media/android/url_provision_fetcher.cc (right): https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/android/url_provision_fetcher.cc:29: os << default_url << "&signedRequest=" << request_data; On 2015/11/11 03:03:33, Tima Vaisburd wrote: > On 2015/11/06 23:08:18, xhwang wrote: > > Is this "signedRequest" part documented anywhere? > > This is what the java code did. The only doc I found is this: > http://developer.android.com/reference/android/media/MediaDrm.ProvisionReques... Yeah, it's like magic... +qinmin, do you remember where get this? https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/android/url_provision_fetcher.cc:32: request_ = URLFetcher::Create(GURL(os.str()), URLFetcher::POST, this); On 2015/11/11 03:03:33, Tima Vaisburd wrote: > On 2015/11/06 23:08:18, xhwang wrote: > > sorry for the bikeshedding... > > > > I think we use streams mostly for logging purposes: > > > > https://google.github.io/styleguide/cppguide.html#Streams > > > > Though it's not banned for other use cases like this one, it's pretty rare to > > use it in Chromium other than logging and tests. For example, you only have > > ostringstream in 11 files in base/, and most of them are for logging and > tests: > > > > > https://code.google.com/p/chromium/codesearch#search/&q=ostringstream%20file:... > > > > Also os.str() will return a copy of the string anyways, I don't see why we > > prefer it to normal string in terms of performance? > > > > Can we simply use std::string? > > Yes, we can. > > I think it would be nice to know what's undesirable or awkward or else wrong in > this use of streams before I would try to avoid them, because I haven't seen the > argument against them so far (except that they might be slow, but comparison was > with the printf and still I did not see the numbers). I think the argument of > current usage, or habits, would not completely answer this question. > > Performance can be an argument here, but as far as I get this operation will be > rare? I won't worry about performance here, it's really just a style thing... Here's the text in the style guide: "Use streams only when they are the best tool for the job. This is typically the case when the I/O is ad-hoc, local, human-readable, and targeted at other developers rather than end-users. Be consistent with the code around you, and with the codebase as a whole;" My 2-cents: - |default_url| and |request_data| are already strings, I don't see why stream is a better tool than simple string in this case. - The string won't be local as we we'll send it to the server. - I don't think a URL request with binary |request_data| is human-readable. - As mentioned earlier, we rarely use streams for cases other than logging and testing in our code base. So the "consistency" rule kicks in... https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/android/url_provision_fetcher.cc:70: scoped_ptr<media::ProvisionFetcher> CDMProvisionFetcher::CreateWithURLContext( On 2015/11/11 03:03:33, Tima Vaisburd wrote: > On 2015/11/06 23:08:18, xhwang wrote: > > Why do we need CDMProvisionFetcher? > > According to the current dependency rules chrome/ can include files only from > content/public. This is an implementation of the class defined in > content/public/. Shall we change the DEPS instead? Oh I see. I missed that. No we should not change DEPS. So CDMProvisionFetcher is not a fetcher, it's more a ProvisionFetcherFactory(). Or, actually, can we just have a factory function declared in content/public? https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... File content/browser/media/android/url_provision_fetcher.h (right): https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/android/url_provision_fetcher.h:17: class URLProvisionFetcher : public media::ProvisionFetcher, On 2015/11/11 03:03:33, Tima Vaisburd wrote: > On 2015/11/06 23:08:18, xhwang wrote: > > nit: All ProvisionFetch will use a URL, so it seems odd that this particular > one > > is a URLProvisionFetcher... > > > > How about just something like ProvisionFetcherImpl? > > I thought we will have another implementation for GPU process. Also, as defined > now, the ProvisionFetcher interface does not tell anything about URL or HTTP. I > meant, rather, direct HTTP call (versus GPU - Browser relay). We can change it > to ProvisionFetcherImpl, especially if there will be just one impl. Yes, I'll need one in the GPU process. That can be named something like MojoProvisionFetcher, since it uses mojo services to do provisioning. https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/android/url_provision_fetcher.h:18: private net::URLFetcherDelegate { On 2015/11/11 03:03:34, Tima Vaisburd wrote: > On 2015/11/06 23:08:18, xhwang wrote: > > style: We don't use private inheritance. > > > > https://google.github.io/styleguide/cppguide.html#Inheritance > > Just changed to public, but, as the link says, I did mean impl. inheritance. > Would you prefer another member? There are 117 cases where net::URLFetcherDelegate is inherited publicly, so public inheritance should be fine. https://code.google.com/p/chromium/codesearch#search/&q=public%5C%20net::URLF... https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... File content/browser/media/cdm/browser_cdm_manager.cc (right): https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/cdm/browser_cdm_manager.cc:558: key_system, use_hw_secure_codecs, provision_fetcher.Pass(), On 2015/11/05 02:28:54, Tima Vaisburd wrote: > On 2015/11/05 02:24:07, Tima Vaisburd wrote: > > I pass |provision_fetcher| here as a parameter, and so I had to move the > > ProvisionFetcher definition to media/base/, although it is used only for > > Android. I wonder whether there is a better way. > > An alternative could be adding another factory method in MediaClientAndroid, > e.g. CreateFetcherForRenderProcess(int id) and implementing it in every subclass > of MediaClientAndroid. WDYT? MediaClients are consumed by media/, which has no knowledge of what a render process is. https://chromiumcodereview.appspot.com/1427183002/diff/40001/media/base/provi... File media/base/provision_fetcher.h (right): https://chromiumcodereview.appspot.com/1427183002/diff/40001/media/base/provi... media/base/provision_fetcher.h:35: }; On 2015/11/11 03:03:34, Tima Vaisburd wrote: > On 2015/11/06 23:08:19, xhwang wrote: > > DISALLOW_COPY_AND_ASSIGN > > Why do we need to require this here? Acknowledged. https://chromiumcodereview.appspot.com/1427183002/diff/140001/chrome/browser/... File chrome/browser/media/android/cdm/media_drm_credential_manager.cc (right): https://chromiumcodereview.appspot.com/1427183002/diff/140001/chrome/browser/... chrome/browser/media/android/cdm/media_drm_credential_manager.cc:98: net::URLRequestContextGetter* context = profile->GetRequestContext(); I did some more research... Can we use the BrowserProcess's system request context? I saw it used a lot in chrome/ code. But to be honest I have no idea what I am talking about :) https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/bro... https://chromiumcodereview.appspot.com/1427183002/diff/140001/content/browser... File content/browser/media/android/url_provision_fetcher.cc (right): https://chromiumcodereview.appspot.com/1427183002/diff/140001/content/browser... content/browser/media/android/url_provision_fetcher.cc:26: response_cb_ = media::BindToCurrentLoop(response_cb); Can response_cb_ be fired on a different thread? If not, we don't need BindToCurrentLoop... Otherwise, we should add a comment. https://chromiumcodereview.appspot.com/1427183002/diff/140001/content/browser... content/browser/media/android/url_provision_fetcher.cc:56: (void)(success); not needed? https://chromiumcodereview.appspot.com/1427183002/diff/140001/content/browser... File content/browser/media/cdm/browser_cdm_manager.cc (right): https://chromiumcodereview.appspot.com/1427183002/diff/140001/content/browser... content/browser/media/cdm/browser_cdm_manager.cc:281: #else Why making this change? Different embedders could implement CreateCdmFactory in the browser_content_client to inject it's own CdmFactory... https://chromiumcodereview.appspot.com/1427183002/diff/140001/media/base/andr... File media/base/android/android_cdm_factory.cc (right): https://chromiumcodereview.appspot.com/1427183002/diff/140001/media/base/andr... media/base/android/android_cdm_factory.cc:52: MediaDrmBridge::Create(key_system, provision_fetcher_.Pass(), We could use the same AndroidCdmFactory to create multiple CDMs, so this won't work :( To fix this, we should probably pass in a callback that creates scoped_ptr<ProvisionFetcher>. What do you think? https://chromiumcodereview.appspot.com/1427183002/diff/140001/media/base/andr... File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java (right): https://chromiumcodereview.appspot.com/1427183002/diff/140001/media/base/andr... media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:748: } +qinmin In theory, it's possible that resetDeviceCredentials() is called first and we called the native code to start provisioning. Then before the provisioning response is received, we try to update session which triggered startProvisioning(), and we are making another provisioning request, which IIRIC is not supported by the ProvisionFetcher interface... Currently this fine because resetDeviceCredentials() is only called by MediaDrmCredentialManager which always creates a separate MDB instance to make this call. But nothing prevents a normal MDB instance to call this method as well. It seems we should fix this. To fix that, probably we can set |mProvisioningPending| in resetDeviceCredentials() as well, and in both resetDeviceCredentials() and startProvisioning(), if |mProvisioningPending| is true, we do nothing. https://chromiumcodereview.appspot.com/1427183002/diff/140001/media/base/andr... media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:759: assert mProvisioningPending; Wondering how this didn't fire when resetting device credential and then provide pronision response here... https://chromiumcodereview.appspot.com/1427183002/diff/140001/media/base/andr... media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:772: return; Why making this change? Is that because currently we are not calling resetDeviceCredentials() and startProvisioning() in the same MDB instance? We probably shouldn't rely on this.. https://chromiumcodereview.appspot.com/1427183002/diff/140001/media/base/andr... File media/base/android/media_client_android.h (right): https://chromiumcodereview.appspot.com/1427183002/diff/140001/media/base/andr... media/base/android/media_client_android.h:14: #include "base/memory/scoped_ptr.h" not needed? https://chromiumcodereview.appspot.com/1427183002/diff/140001/media/base/andr... File media/base/android/media_drm_bridge.cc (right): https://chromiumcodereview.appspot.com/1427183002/diff/140001/media/base/andr... media/base/android/media_drm_bridge.cc:291: DCHECK(provision_fetcher); nit: Move this check to the constructor so that we also cover Create(). https://chromiumcodereview.appspot.com/1427183002/diff/140001/media/base/andr... media/base/android/media_drm_bridge.cc:788: return; Can this be removed now? https://chromiumcodereview.appspot.com/1427183002/diff/140001/media/base/andr... media/base/android/media_drm_bridge.cc:802: return; Upon failure, we should still call Java_MediaDrmBridge_processProvisionResponse() so that the Java code can fail gracefully, e.g. fire OnResetDeviceCredentialsCompleted() with "false". https://chromiumcodereview.appspot.com/1427183002/diff/140001/media/base/andr... File media/base/android/provision_fetcher.h (right): https://chromiumcodereview.appspot.com/1427183002/diff/140001/media/base/andr... media/base/android/provision_fetcher.h:27: // and calls |cb| callback with the response. The input parameters s/cb/response_cb
https://chromiumcodereview.appspot.com/1427183002/diff/140001/content/public/... File content/public/browser/android/cdm_provision_fetcher.h (right): https://chromiumcodereview.appspot.com/1427183002/diff/140001/content/public/... content/public/browser/android/cdm_provision_fetcher.h:28: }; Just checked again, yes you can just use a function here. Here's an example: https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...
Xiaohan, I tried to address all your comments except for provision request doc, but there are some more questions, most important, I think, are around ProvisionFetcherFactory. Please take another look. https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... File content/browser/media/android/url_provision_fetcher.cc (right): https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/android/url_provision_fetcher.cc:32: request_ = URLFetcher::Create(GURL(os.str()), URLFetcher::POST, this); On 2015/11/11 09:53:20, xhwang wrote: > On 2015/11/11 03:03:33, Tima Vaisburd wrote: > > On 2015/11/06 23:08:18, xhwang wrote: > > > sorry for the bikeshedding... > > > > > > I think we use streams mostly for logging purposes: > > > > > > https://google.github.io/styleguide/cppguide.html#Streams > > > > > > Though it's not banned for other use cases like this one, it's pretty rare > to > > > use it in Chromium other than logging and tests. For example, you only have > > > ostringstream in 11 files in base/, and most of them are for logging and > > tests: > > > > > > > > > https://code.google.com/p/chromium/codesearch#search/&q=ostringstream%20file:... > > > > > > Also os.str() will return a copy of the string anyways, I don't see why we > > > prefer it to normal string in terms of performance? > > > > > > Can we simply use std::string? > > > > Yes, we can. > > > > I think it would be nice to know what's undesirable or awkward or else wrong > in > > this use of streams before I would try to avoid them, because I haven't seen > the > > argument against them so far (except that they might be slow, but comparison > was > > with the printf and still I did not see the numbers). I think the argument of > > current usage, or habits, would not completely answer this question. > > > > Performance can be an argument here, but as far as I get this operation will > be > > rare? > > I won't worry about performance here, it's really just a style thing... > > Here's the text in the style guide: > > "Use streams only when they are the best tool for the job. This is typically the > case when the I/O is ad-hoc, local, human-readable, and targeted at other > developers rather than end-users. Be consistent with the code around you, and > with the codebase as a whole;" > > My 2-cents: > - |default_url| and |request_data| are already strings, I don't see why stream > is a better tool than simple string in this case. > - The string won't be local as we we'll send it to the server. > - I don't think a URL request with binary |request_data| is human-readable. > - As mentioned earlier, we rarely use streams for cases other than logging and > testing in our code base. So the "consistency" rule kicks in... My stubbornness won't stretch pass this point. Done! I was important to know that this is a style preference and not something inherently bad in the stringstream. https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/android/url_provision_fetcher.cc:70: scoped_ptr<media::ProvisionFetcher> CDMProvisionFetcher::CreateWithURLContext( On 2015/11/11 09:53:20, xhwang wrote: > On 2015/11/11 03:03:33, Tima Vaisburd wrote: > > On 2015/11/06 23:08:18, xhwang wrote: > > > Why do we need CDMProvisionFetcher? > > > > According to the current dependency rules chrome/ can include files only from > > content/public. This is an implementation of the class defined in > > content/public/. Shall we change the DEPS instead? > > Oh I see. I missed that. > > No we should not change DEPS. > > So CDMProvisionFetcher is not a fetcher, it's more a ProvisionFetcherFactory(). > Or, actually, can we just have a factory function declared in content/public? Yes, created a factory function CreateProvisionFetcherFactory() - added ...Factory() because now there is one more redirection. See my reply and question in BrowserCdmManager. https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... File content/browser/media/android/url_provision_fetcher.h (right): https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/android/url_provision_fetcher.h:17: class URLProvisionFetcher : public media::ProvisionFetcher, On 2015/11/11 09:53:21, xhwang wrote: > On 2015/11/11 03:03:33, Tima Vaisburd wrote: > > On 2015/11/06 23:08:18, xhwang wrote: > > > nit: All ProvisionFetch will use a URL, so it seems odd that this particular > > one > > > is a URLProvisionFetcher... > > > > > > How about just something like ProvisionFetcherImpl? > > > > I thought we will have another implementation for GPU process. Also, as > defined > > now, the ProvisionFetcher interface does not tell anything about URL or HTTP. > I > > meant, rather, direct HTTP call (versus GPU - Browser relay). We can change it > > to ProvisionFetcherImpl, especially if there will be just one impl. > > Yes, I'll need one in the GPU process. That can be named something like > MojoProvisionFetcher, since it uses mojo services to do provisioning. Shall we keep URLProvisionFetcher then? Or change to e.g. BrowserProvisionFetcher to stress the process name? https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/android/url_provision_fetcher.h:18: private net::URLFetcherDelegate { On 2015/11/11 09:53:21, xhwang wrote: > On 2015/11/11 03:03:34, Tima Vaisburd wrote: > > On 2015/11/06 23:08:18, xhwang wrote: > > > style: We don't use private inheritance. > > > > > > https://google.github.io/styleguide/cppguide.html#Inheritance > > > > Just changed to public, but, as the link says, I did mean impl. inheritance. > > Would you prefer another member? > > There are 117 cases where net::URLFetcherDelegate is inherited publicly, so > public inheritance should be fine. > > https://code.google.com/p/chromium/codesearch#search/&q=public%5C%20net::URLF... Acknowledged. https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/android/url_provision_fetcher.h:26: const media::ProvisionFetcher::ResponseCB& cb) override; On 2015/11/06 23:08:18, xhwang wrote: > nit: cb/response_cb/ Done. https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/android/url_provision_fetcher.h:26: const media::ProvisionFetcher::ResponseCB& cb) override; On 2015/11/06 23:08:18, xhwang wrote: > s/media::ProvisionFetcher::// since |this| is a media::ProvisionFetcher? Done. https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... File content/browser/media/cdm/browser_cdm_manager.cc (right): https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/cdm/browser_cdm_manager.cc:558: key_system, use_hw_secure_codecs, provision_fetcher.Pass(), On 2015/11/11 09:53:21, xhwang wrote: > On 2015/11/05 02:28:54, Tima Vaisburd wrote: > > On 2015/11/05 02:24:07, Tima Vaisburd wrote: > > > I pass |provision_fetcher| here as a parameter, and so I had to move the > > > ProvisionFetcher definition to media/base/, although it is used only for > > > Android. I wonder whether there is a better way. > > > > An alternative could be adding another factory method in MediaClientAndroid, > > e.g. CreateFetcherForRenderProcess(int id) and implementing it in every > subclass > > of MediaClientAndroid. WDYT? > > MediaClients are consumed by media/, which has no knowledge of what a render > process is. MediaClient is not used for the provisioning purpose any more, I think my question was outdated. Or did I miss your point here? https://chromiumcodereview.appspot.com/1427183002/diff/140001/chrome/browser/... File chrome/browser/media/android/cdm/media_drm_credential_manager.cc (right): https://chromiumcodereview.appspot.com/1427183002/diff/140001/chrome/browser/... chrome/browser/media/android/cdm/media_drm_credential_manager.cc:98: net::URLRequestContextGetter* context = profile->GetRequestContext(); On 2015/11/11 09:53:21, xhwang wrote: > I did some more research... Can we use the BrowserProcess's system request > context? I saw it used a lot in chrome/ code. But to be honest I have no idea > what I am talking about :) > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/bro... Done. https://chromiumcodereview.appspot.com/1427183002/diff/140001/content/browser... File content/browser/media/android/url_provision_fetcher.cc (right): https://chromiumcodereview.appspot.com/1427183002/diff/140001/content/browser... content/browser/media/android/url_provision_fetcher.cc:26: response_cb_ = media::BindToCurrentLoop(response_cb); On 2015/11/11 09:53:21, xhwang wrote: > Can response_cb_ be fired on a different thread? If not, we don't need > BindToCurrentLoop... Otherwise, we should add a comment. Yes, this was unaswered in PS2. The URLFetcher implementation seems to guarantee that OnURLFetchComplete is called on the same thread that called Start(): https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur... It also posts a task, so the call will be async. I put this into comment and removed BindToCurentLoop, however, should we rely on the current net implementation in this respect? https://chromiumcodereview.appspot.com/1427183002/diff/140001/content/browser... content/browser/media/android/url_provision_fetcher.cc:56: (void)(success); On 2015/11/11 09:53:21, xhwang wrote: > not needed? Sorry, it was bad thinking about unused variable. Removed. https://chromiumcodereview.appspot.com/1427183002/diff/140001/content/browser... File content/browser/media/cdm/browser_cdm_manager.cc (right): https://chromiumcodereview.appspot.com/1427183002/diff/140001/content/browser... content/browser/media/cdm/browser_cdm_manager.cc:281: #else On 2015/11/11 09:53:21, xhwang wrote: > Why making this change? Different embedders could implement CreateCdmFactory in > the browser_content_client to inject it's own CdmFactory... Restored your original code and passed ProvisionFetcherFactory object (see below) into AndroidCdmFactory constructor. https://chromiumcodereview.appspot.com/1427183002/diff/140001/content/public/... File content/public/browser/android/cdm_provision_fetcher.h (right): https://chromiumcodereview.appspot.com/1427183002/diff/140001/content/public/... content/public/browser/android/cdm_provision_fetcher.h:28: }; On 2015/11/11 09:58:20, xhwang wrote: > Just checked again, yes you can just use a function here. Here's an example: > > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... Done. https://chromiumcodereview.appspot.com/1427183002/diff/140001/media/base/andr... File media/base/android/android_cdm_factory.cc (right): https://chromiumcodereview.appspot.com/1427183002/diff/140001/media/base/andr... media/base/android/android_cdm_factory.cc:52: MediaDrmBridge::Create(key_system, provision_fetcher_.Pass(), On 2015/11/11 09:53:21, xhwang wrote: > We could use the same AndroidCdmFactory to create multiple CDMs, so this won't > work :( > > To fix this, we should probably pass in a callback that creates > scoped_ptr<ProvisionFetcher>. What do you think? Made another class - ProvisionFetcherFactory - and passed its subclass to AndroidCdmFactory constructor. AndroidCdmFactory owns ProvisonFetcherFactory. ProvisionFetcherFactory's sublass (URL...) holds the networking context and can create fetchers. MediaDrmCredentialManager also owns such factory for the browser system context. With this change MediaDrmBridge methods can accept the factory pointer instead of a fetcher pointer. This is not done yet. WDYT? https://chromiumcodereview.appspot.com/1427183002/diff/140001/media/base/andr... File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java (right): https://chromiumcodereview.appspot.com/1427183002/diff/140001/media/base/andr... media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:748: } On 2015/11/11 09:53:21, xhwang wrote: > +qinmin > > In theory, it's possible that resetDeviceCredentials() is called first and we > called the native code to start provisioning. Then before the provisioning > response is received, we try to update session which triggered > startProvisioning(), and we are making another provisioning request, which IIRIC > is not supported by the ProvisionFetcher interface... > > Currently this fine because resetDeviceCredentials() is only called by > MediaDrmCredentialManager which always creates a separate MDB instance to make > this call. But nothing prevents a normal MDB instance to call this method as > well. It seems we should fix this. > > To fix that, probably we can set |mProvisioningPending| in > resetDeviceCredentials() as well, and in both resetDeviceCredentials() and > startProvisioning(), if |mProvisioningPending| is true, we do nothing. Now I call startProvisioning() from resetDeviceCredentials(), and startProvisioning returns early if the provisioning is ongoing instead of asserting. https://chromiumcodereview.appspot.com/1427183002/diff/140001/media/base/andr... media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:759: assert mProvisioningPending; On 2015/11/11 09:53:21, xhwang wrote: > Wondering how this didn't fire when resetting device credential and then provide > pronision response here... Thank you for spotting this. Now I call startProvisioning() from this method. Java asserts have to be enabled separately: http://howto-and-stuff.blogspot.com/2014/02/enabling-java-asserts-on-android.... https://chromiumcodereview.appspot.com/1427183002/diff/140001/media/base/andr... media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:772: return; On 2015/11/11 09:53:21, xhwang wrote: > Why making this change? Is that because currently we are not calling > resetDeviceCredentials() and startProvisioning() in the same MDB instance? We > probably shouldn't rely on this.. Removed. https://chromiumcodereview.appspot.com/1427183002/diff/140001/media/base/andr... File media/base/android/media_client_android.h (right): https://chromiumcodereview.appspot.com/1427183002/diff/140001/media/base/andr... media/base/android/media_client_android.h:14: #include "base/memory/scoped_ptr.h" On 2015/11/11 09:53:21, xhwang wrote: > not needed? Done. https://chromiumcodereview.appspot.com/1427183002/diff/140001/media/base/andr... File media/base/android/media_drm_bridge.cc (right): https://chromiumcodereview.appspot.com/1427183002/diff/140001/media/base/andr... media/base/android/media_drm_bridge.cc:291: DCHECK(provision_fetcher); On 2015/11/11 09:53:21, xhwang wrote: > nit: Move this check to the constructor so that we also cover Create(). Done. https://chromiumcodereview.appspot.com/1427183002/diff/140001/media/base/andr... media/base/android/media_drm_bridge.cc:788: return; On 2015/11/11 09:53:21, xhwang wrote: > Can this be removed now? Done. https://chromiumcodereview.appspot.com/1427183002/diff/140001/media/base/andr... media/base/android/media_drm_bridge.cc:802: return; On 2015/11/11 09:53:21, xhwang wrote: > Upon failure, we should still call > Java_MediaDrmBridge_processProvisionResponse() so that the Java code can fail > gracefully, e.g. fire OnResetDeviceCredentialsCompleted() with "false". Done. https://chromiumcodereview.appspot.com/1427183002/diff/140001/media/base/andr... File media/base/android/provision_fetcher.h (right): https://chromiumcodereview.appspot.com/1427183002/diff/140001/media/base/andr... media/base/android/provision_fetcher.h:27: // and calls |cb| callback with the response. The input parameters On 2015/11/11 09:53:22, xhwang wrote: > s/cb/response_cb Done.
looking pretty good! Just a few more comments. The main question here is whether we want to remove the Factory class. We only need it in one place and we can easily replace it with a callback. https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... File content/browser/media/android/url_provision_fetcher.h (right): https://chromiumcodereview.appspot.com/1427183002/diff/40001/content/browser/... content/browser/media/android/url_provision_fetcher.h:17: class URLProvisionFetcher : public media::ProvisionFetcher, On 2015/11/11 23:26:35, Tima Vaisburd wrote: > On 2015/11/11 09:53:21, xhwang wrote: > > On 2015/11/11 03:03:33, Tima Vaisburd wrote: > > > On 2015/11/06 23:08:18, xhwang wrote: > > > > nit: All ProvisionFetch will use a URL, so it seems odd that this > particular > > > one > > > > is a URLProvisionFetcher... > > > > > > > > How about just something like ProvisionFetcherImpl? > > > > > > I thought we will have another implementation for GPU process. Also, as > > defined > > > now, the ProvisionFetcher interface does not tell anything about URL or > HTTP. > > I > > > meant, rather, direct HTTP call (versus GPU - Browser relay). We can change > it > > > to ProvisionFetcherImpl, especially if there will be just one impl. > > > > Yes, I'll need one in the GPU process. That can be named something like > > MojoProvisionFetcher, since it uses mojo services to do provisioning. > > Shall we keep URLProvisionFetcher then? Or change to e.g. > BrowserProvisionFetcher to stress the process name? I really have no preference. Now "URL" seems okay too since we use a bunch of URL* to implement it :) https://chromiumcodereview.appspot.com/1427183002/diff/140001/content/browser... File content/browser/media/android/url_provision_fetcher.cc (right): https://chromiumcodereview.appspot.com/1427183002/diff/140001/content/browser... content/browser/media/android/url_provision_fetcher.cc:26: response_cb_ = media::BindToCurrentLoop(response_cb); On 2015/11/11 23:26:35, Tima Vaisburd wrote: > On 2015/11/11 09:53:21, xhwang wrote: > > Can response_cb_ be fired on a different thread? If not, we don't need > > BindToCurrentLoop... Otherwise, we should add a comment. > > Yes, this was unaswered in PS2. The URLFetcher implementation seems to guarantee > that OnURLFetchComplete is called on the same thread that called Start(): > https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur... > > It also posts a task, so the call will be async. > I put this into comment and removed BindToCurentLoop, however, should we rely on > the current net implementation in this respect? This should really be part of the URLFetcher's public interface instead of implementation detail... https://chromiumcodereview.appspot.com/1427183002/diff/140001/media/base/andr... File media/base/android/android_cdm_factory.cc (right): https://chromiumcodereview.appspot.com/1427183002/diff/140001/media/base/andr... media/base/android/android_cdm_factory.cc:52: MediaDrmBridge::Create(key_system, provision_fetcher_.Pass(), On 2015/11/11 23:26:35, Tima Vaisburd wrote: > On 2015/11/11 09:53:21, xhwang wrote: > > We could use the same AndroidCdmFactory to create multiple CDMs, so this won't > > work :( > > > > To fix this, we should probably pass in a callback that creates > > scoped_ptr<ProvisionFetcher>. What do you think? > > Made another class - ProvisionFetcherFactory - and passed its subclass to > AndroidCdmFactory constructor. AndroidCdmFactory owns ProvisonFetcherFactory. > ProvisionFetcherFactory's sublass (URL...) holds the networking context and can > create fetchers. > > MediaDrmCredentialManager also owns such factory for the browser system context. > > With this change MediaDrmBridge methods can accept the factory pointer instead > of a fetcher pointer. This is not done yet. WDYT? I don't think MDB would allow >1 provisioning process at the same time. So it seems unnecessary, WDYT? https://chromiumcodereview.appspot.com/1427183002/diff/160001/chrome/browser/... File chrome/browser/media/android/cdm/media_drm_credential_manager.cc (right): https://chromiumcodereview.appspot.com/1427183002/diff/160001/chrome/browser/... chrome/browser/media/android/cdm/media_drm_credential_manager.cc:40: g_browser_process->system_request_context())); Did you try this and does it work? I don't see why not but just double checking... https://chromiumcodereview.appspot.com/1427183002/diff/160001/chrome/browser/... chrome/browser/media/android/cdm/media_drm_credential_manager.cc:103: kWidevineKeySystem, provision_fetcher.Pass()); nit: you can just put provision_fetcher_factory_->CreateFetcher() here. https://chromiumcodereview.appspot.com/1427183002/diff/160001/chrome/browser/... chrome/browser/media/android/cdm/media_drm_credential_manager.cc:103: kWidevineKeySystem, provision_fetcher.Pass()); Actually, if we have something like content::CreateProvisionFetcher(), we can call it directly here. This makes me wonder whether we really need a Factory class. See my comment at the bottom of this PS. https://chromiumcodereview.appspot.com/1427183002/diff/160001/content/browser... File content/browser/media/android/url_provision_fetcher.cc (right): https://chromiumcodereview.appspot.com/1427183002/diff/160001/content/browser... content/browser/media/android/url_provision_fetcher.cc:84: media::ProvisionFetcherFactory* CreateURLProvisionFetcherFactory( This should CreateProvisionFetcherFactory since the "URL" part is content/ implementation detail. https://chromiumcodereview.appspot.com/1427183002/diff/160001/content/browser... File content/browser/media/android/url_provision_fetcher.h (right): https://chromiumcodereview.appspot.com/1427183002/diff/160001/content/browser... content/browser/media/android/url_provision_fetcher.h:32: net::URLRequestContextGetter* context_; I saw both are used [1]: - net::URLRequestContextGetter* context - net::URLRequestContextGetter* context_getter IMHO the getter one is more clear and accurate. [1] https://code.google.com/p/chromium/codesearch#search/&q=%22URLRequestContextG... https://chromiumcodereview.appspot.com/1427183002/diff/160001/media/base/andr... File media/base/android/android_cdm_factory.cc (right): https://chromiumcodereview.appspot.com/1427183002/diff/160001/media/base/andr... media/base/android/android_cdm_factory.cc:55: MediaDrmBridge::Create(key_system, fetcher.Pass(), nit: you can pass fetcher_factory_->CreateFetcher() here directly. https://chromiumcodereview.appspot.com/1427183002/diff/160001/media/base/andr... File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java (right): https://chromiumcodereview.appspot.com/1427183002/diff/160001/media/base/andr... media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:416: */ remove? https://chromiumcodereview.appspot.com/1427183002/diff/160001/media/base/andr... File media/base/android/media_drm_bridge.cc (right): https://chromiumcodereview.appspot.com/1427183002/diff/160001/media/base/andr... media/base/android/media_drm_bridge.cc:805: j_response.obj()); The current code works. But wondering whether it's better to pass |success| to Java as well. No strong opinions. Just some idea for discussion... https://chromiumcodereview.appspot.com/1427183002/diff/160001/media/base/andr... File media/base/android/media_drm_bridge.h (right): https://chromiumcodereview.appspot.com/1427183002/diff/160001/media/base/andr... media/base/android/media_drm_bridge.h:266: // Exists for the duration of one request/response only. What does this mean? Isn't |provision_fetcher_| always non-null during the lifetime of |this|? https://chromiumcodereview.appspot.com/1427183002/diff/160001/media/base/andr... File media/base/android/provision_fetcher.h (right): https://chromiumcodereview.appspot.com/1427183002/diff/160001/media/base/andr... media/base/android/provision_fetcher.h:43: }; I can see this is following the model of CdmFactory. But it seems we have only one implementation for ProvisionFetcherFactory, which is used in only two cases, and we actually don't need it in one case (see above in MediaDrmCredentialManager). This makes me wonder whether a creation callback (e.g. create_provision_fetcher_cb) would just suffice (and be simpler)? base::Callback<scoped_ptr<ProvisionFetcher>()); And we only need this callback for AndroidCdmFactory.
Comments addressed, please take another look. https://codereview.chromium.org/1427183002/diff/40001/content/browser/media/a... File content/browser/media/android/url_provision_fetcher.h (right): https://codereview.chromium.org/1427183002/diff/40001/content/browser/media/a... content/browser/media/android/url_provision_fetcher.h:17: class URLProvisionFetcher : public media::ProvisionFetcher, On 2015/11/12 22:27:03, xhwang wrote: > On 2015/11/11 23:26:35, Tima Vaisburd wrote: > > On 2015/11/11 09:53:21, xhwang wrote: > > > On 2015/11/11 03:03:33, Tima Vaisburd wrote: > > > > On 2015/11/06 23:08:18, xhwang wrote: > > > > > nit: All ProvisionFetch will use a URL, so it seems odd that this > > particular > > > > one > > > > > is a URLProvisionFetcher... > > > > > > > > > > How about just something like ProvisionFetcherImpl? > > > > > > > > I thought we will have another implementation for GPU process. Also, as > > > defined > > > > now, the ProvisionFetcher interface does not tell anything about URL or > > HTTP. > > > I > > > > meant, rather, direct HTTP call (versus GPU - Browser relay). We can > change > > it > > > > to ProvisionFetcherImpl, especially if there will be just one impl. > > > > > > Yes, I'll need one in the GPU process. That can be named something like > > > MojoProvisionFetcher, since it uses mojo services to do provisioning. > > > > Shall we keep URLProvisionFetcher then? Or change to e.g. > > BrowserProvisionFetcher to stress the process name? > > I really have no preference. Now "URL" seems okay too since we use a bunch of > URL* to implement it :) kept current name. https://codereview.chromium.org/1427183002/diff/140001/media/base/android/and... File media/base/android/android_cdm_factory.cc (right): https://codereview.chromium.org/1427183002/diff/140001/media/base/android/and... media/base/android/android_cdm_factory.cc:52: MediaDrmBridge::Create(key_system, provision_fetcher_.Pass(), On 2015/11/12 22:27:03, xhwang wrote: > On 2015/11/11 23:26:35, Tima Vaisburd wrote: > > On 2015/11/11 09:53:21, xhwang wrote: > > > We could use the same AndroidCdmFactory to create multiple CDMs, so this > won't > > > work :( > > > > > > To fix this, we should probably pass in a callback that creates > > > scoped_ptr<ProvisionFetcher>. What do you think? > > > > Made another class - ProvisionFetcherFactory - and passed its subclass to > > AndroidCdmFactory constructor. AndroidCdmFactory owns ProvisonFetcherFactory. > > ProvisionFetcherFactory's sublass (URL...) holds the networking context and > can > > create fetchers. > > > > MediaDrmCredentialManager also owns such factory for the browser system > context. > > > > With this change MediaDrmBridge methods can accept the factory pointer instead > > of a fetcher pointer. This is not done yet. WDYT? > > I don't think MDB would allow >1 provisioning process at the same time. So it > seems unnecessary, WDYT? Yes, I removed ProvisionFetcherFactory altogether. https://codereview.chromium.org/1427183002/diff/160001/chrome/browser/media/a... File chrome/browser/media/android/cdm/media_drm_credential_manager.cc (right): https://codereview.chromium.org/1427183002/diff/160001/chrome/browser/media/a... chrome/browser/media/android/cdm/media_drm_credential_manager.cc:40: g_browser_process->system_request_context())); On 2015/11/12 22:27:03, xhwang wrote: > Did you try this and does it work? I don't see why not but just double > checking... Yes, I tested and it worked for me. https://codereview.chromium.org/1427183002/diff/160001/chrome/browser/media/a... chrome/browser/media/android/cdm/media_drm_credential_manager.cc:103: kWidevineKeySystem, provision_fetcher.Pass()); On 2015/11/12 22:27:03, xhwang wrote: > Actually, if we have something like content::CreateProvisionFetcher(), we can > call it directly here. This makes me wonder whether we really need a Factory > class. See my comment at the bottom of this PS. Done. https://codereview.chromium.org/1427183002/diff/160001/content/browser/media/... File content/browser/media/android/url_provision_fetcher.cc (right): https://codereview.chromium.org/1427183002/diff/160001/content/browser/media/... content/browser/media/android/url_provision_fetcher.cc:84: media::ProvisionFetcherFactory* CreateURLProvisionFetcherFactory( On 2015/11/12 22:27:03, xhwang wrote: > This should CreateProvisionFetcherFactory since the "URL" part is content/ > implementation detail. The function now creates the fetcher instead of a factory, hence CreateProvisionFetcher(). https://codereview.chromium.org/1427183002/diff/160001/content/browser/media/... File content/browser/media/android/url_provision_fetcher.h (right): https://codereview.chromium.org/1427183002/diff/160001/content/browser/media/... content/browser/media/android/url_provision_fetcher.h:32: net::URLRequestContextGetter* context_; On 2015/11/12 22:27:03, xhwang wrote: > I saw both are used [1]: > - net::URLRequestContextGetter* context > - net::URLRequestContextGetter* context_getter > > IMHO the getter one is more clear and accurate. > > [1] > https://code.google.com/p/chromium/codesearch#search/&q=%22URLRequestContextG... Done. https://codereview.chromium.org/1427183002/diff/160001/media/base/android/and... File media/base/android/android_cdm_factory.cc (right): https://codereview.chromium.org/1427183002/diff/160001/media/base/android/and... media/base/android/android_cdm_factory.cc:55: MediaDrmBridge::Create(key_system, fetcher.Pass(), On 2015/11/12 22:27:03, xhwang wrote: > nit: you can pass fetcher_factory_->CreateFetcher() here directly. Used the callback method instead the factory. https://codereview.chromium.org/1427183002/diff/160001/media/base/android/jav... File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java (right): https://codereview.chromium.org/1427183002/diff/160001/media/base/android/jav... media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:416: */ On 2015/11/12 22:27:03, xhwang wrote: > remove? Oops. Removed. https://codereview.chromium.org/1427183002/diff/160001/media/base/android/med... File media/base/android/media_drm_bridge.cc (right): https://codereview.chromium.org/1427183002/diff/160001/media/base/android/med... media/base/android/media_drm_bridge.cc:805: j_response.obj()); On 2015/11/12 22:27:03, xhwang wrote: > The current code works. But wondering whether it's better to pass |success| to > Java as well. No strong opinions. Just some idea for discussion... Done. https://codereview.chromium.org/1427183002/diff/160001/media/base/android/med... File media/base/android/media_drm_bridge.h (right): https://codereview.chromium.org/1427183002/diff/160001/media/base/android/med... media/base/android/media_drm_bridge.h:266: // Exists for the duration of one request/response only. On 2015/11/12 22:27:03, xhwang wrote: > What does this mean? Isn't |provision_fetcher_| always non-null during the > lifetime of |this|? Yes, this was my mistake. Removed the line. https://codereview.chromium.org/1427183002/diff/160001/media/base/android/pro... File media/base/android/provision_fetcher.h (right): https://codereview.chromium.org/1427183002/diff/160001/media/base/android/pro... media/base/android/provision_fetcher.h:43: }; On 2015/11/12 22:27:03, xhwang wrote: > I can see this is following the model of CdmFactory. But it seems we have only > one implementation for ProvisionFetcherFactory, which is used in only two cases, > and we actually don't need it in one case (see above in > MediaDrmCredentialManager). This makes me wonder whether a creation callback > (e.g. create_provision_fetcher_cb) would just suffice (and be simpler)? > > base::Callback<scoped_ptr<ProvisionFetcher>()); > > And we only need this callback for AndroidCdmFactory. Done.
LGTM % nits! Thanks for the patience! https://codereview.chromium.org/1427183002/diff/180001/content/browser/media/... File content/browser/media/android/url_provision_fetcher.cc (right): https://codereview.chromium.org/1427183002/diff/180001/content/browser/media/... content/browser/media/android/url_provision_fetcher.cc:73: net::URLRequestContextGetter* context_getter) { DCHECK(context_getter); https://codereview.chromium.org/1427183002/diff/180001/content/browser/media/... content/browser/media/android/url_provision_fetcher.cc:75: new URLProvisionFetcher(context_getter)); will make_scoped_ptr(new URLProvisionFetcher(context_getter)) work? https://codereview.chromium.org/1427183002/diff/180001/media/base/android/and... File media/base/android/android_cdm_factory.cc (right): https://codereview.chromium.org/1427183002/diff/180001/media/base/android/and... media/base/android/android_cdm_factory.cc:51: MediaDrmBridge::Create(key_system, create_fetcher_cb_.Run().Pass(), no need to Pass() here https://codereview.chromium.org/1427183002/diff/180001/media/base/android/and... File media/base/android/android_cdm_factory.h (right): https://codereview.chromium.org/1427183002/diff/180001/media/base/android/and... media/base/android/android_cdm_factory.h:19: using CreateFetcherCB = base::Callback<scoped_ptr<ProvisionFetcher>()>; include scoped_ptr.h https://codereview.chromium.org/1427183002/diff/180001/media/base/android/jav... File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java (right): https://codereview.chromium.org/1427183002/diff/180001/media/base/android/jav... media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:767: success = provideProvisionResponse(response); nit: usually we don't change the input parameter. can we have a different variable?
lgtm % nits https://codereview.chromium.org/1427183002/diff/180001/chrome/browser/media/a... File chrome/browser/media/android/cdm/media_drm_credential_manager.cc (right): https://codereview.chromium.org/1427183002/diff/180001/chrome/browser/media/a... chrome/browser/media/android/cdm/media_drm_credential_manager.cc:96: DCHECK(g_browser_process); nit: no need for this, if this is null, the following statement would crash https://codereview.chromium.org/1427183002/diff/180001/content/browser/media/... File content/browser/media/android/url_provision_fetcher.cc (right): https://codereview.chromium.org/1427183002/diff/180001/content/browser/media/... content/browser/media/android/url_provision_fetcher.cc:13: using net::URLFetcher; nit: move this out side namespace content https://codereview.chromium.org/1427183002/diff/180001/content/browser/media/... content/browser/media/android/url_provision_fetcher.cc:29: std::string request_string = default_url + "&signedRequest=" + request_data; nit: consider using constants for all the const strings here. https://codereview.chromium.org/1427183002/diff/180001/content/browser/media/... content/browser/media/android/url_provision_fetcher.cc:47: DCHECK(source); no need https://codereview.chromium.org/1427183002/diff/180001/content/browser/media/... content/browser/media/android/url_provision_fetcher.cc:63: DCHECK(!response_cb_.is_null()); no need
timav@chromium.org changed reviewers: + yfriedman@chromium.org
Review: adding yfriedman@ as content/public owner. PTAL. https://codereview.chromium.org/1427183002/diff/180001/chrome/browser/media/a... File chrome/browser/media/android/cdm/media_drm_credential_manager.cc (right): https://codereview.chromium.org/1427183002/diff/180001/chrome/browser/media/a... chrome/browser/media/android/cdm/media_drm_credential_manager.cc:96: DCHECK(g_browser_process); On 2015/11/13 19:07:40, qinmin wrote: > nit: no need for this, if this is null, the following statement would crash Removed. https://codereview.chromium.org/1427183002/diff/180001/content/browser/media/... File content/browser/media/android/url_provision_fetcher.cc (right): https://codereview.chromium.org/1427183002/diff/180001/content/browser/media/... content/browser/media/android/url_provision_fetcher.cc:13: using net::URLFetcher; On 2015/11/13 19:07:40, qinmin wrote: > nit: move this out side namespace content Done. https://codereview.chromium.org/1427183002/diff/180001/content/browser/media/... content/browser/media/android/url_provision_fetcher.cc:29: std::string request_string = default_url + "&signedRequest=" + request_data; On 2015/11/13 19:07:40, qinmin wrote: > nit: consider using constants for all the const strings here. Changed this to const std::string, the second one in OnURLFtechComplete() has to be non-const. https://codereview.chromium.org/1427183002/diff/180001/content/browser/media/... content/browser/media/android/url_provision_fetcher.cc:47: DCHECK(source); On 2015/11/13 19:07:40, qinmin wrote: > no need Xiaohan seems to encourage DCHECKs for every input parameter where applicable. Keeping this one. https://codereview.chromium.org/1427183002/diff/180001/content/browser/media/... content/browser/media/android/url_provision_fetcher.cc:63: DCHECK(!response_cb_.is_null()); On 2015/11/13 19:07:40, qinmin wrote: > no need Removed. https://codereview.chromium.org/1427183002/diff/180001/content/browser/media/... content/browser/media/android/url_provision_fetcher.cc:73: net::URLRequestContextGetter* context_getter) { On 2015/11/13 05:15:42, xhwang wrote: > DCHECK(context_getter); Done. https://codereview.chromium.org/1427183002/diff/180001/content/browser/media/... content/browser/media/android/url_provision_fetcher.cc:75: new URLProvisionFetcher(context_getter)); On 2015/11/13 05:15:42, xhwang wrote: > will make_scoped_ptr(new URLProvisionFetcher(context_getter)) work? Done. https://codereview.chromium.org/1427183002/diff/180001/media/base/android/and... File media/base/android/android_cdm_factory.cc (right): https://codereview.chromium.org/1427183002/diff/180001/media/base/android/and... media/base/android/android_cdm_factory.cc:51: MediaDrmBridge::Create(key_system, create_fetcher_cb_.Run().Pass(), On 2015/11/13 05:15:42, xhwang wrote: > no need to Pass() here Done. https://codereview.chromium.org/1427183002/diff/180001/media/base/android/and... File media/base/android/android_cdm_factory.h (right): https://codereview.chromium.org/1427183002/diff/180001/media/base/android/and... media/base/android/android_cdm_factory.h:19: using CreateFetcherCB = base::Callback<scoped_ptr<ProvisionFetcher>()>; On 2015/11/13 05:15:42, xhwang wrote: > include scoped_ptr.h Done. https://codereview.chromium.org/1427183002/diff/180001/media/base/android/jav... File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java (right): https://codereview.chromium.org/1427183002/diff/180001/media/base/android/jav... media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:767: success = provideProvisionResponse(response); On 2015/11/13 05:15:42, xhwang wrote: > nit: usually we don't change the input parameter. can we have a different > variable? Done.
lgtm
The CQ bit was checked by timav@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org, xhwang@chromium.org Link to the patchset: https://codereview.chromium.org/1427183002/#ps240001 (title: "Android Mojo client uses dummy provision fetcher instead of no fetcher at all")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1427183002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1427183002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by timav@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yfriedman@chromium.org, xhwang@chromium.org, qinmin@chromium.org Link to the patchset: https://codereview.chromium.org/1427183002/#ps260001 (title: "Reformatted media/")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1427183002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1427183002/260001
Message was sent while issue was closed.
Committed patchset #13 (id:260001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/b7a0b9a3e5f55858148fdb1e36a4b0421a22467b Cr-Commit-Position: refs/heads/master@{#359923} |
