Move MediaDrmBridge provision communication to native side.

content/browser/media/android/url_provision_fetcher.cc

Index: content/browser/media/android/url_provision_fetcher.cc
diff --git a/content/browser/media/android/url_provision_fetcher.cc b/content/browser/media/android/url_provision_fetcher.cc
new file mode 100644
index 0000000000000000000000000000000000000000..2612260bf39be7b0fee5459de3144a48dfd54e27
--- /dev/null
+++ b/content/browser/media/android/url_provision_fetcher.cc
@@ -0,0 +1,75 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/browser/media/android/url_provision_fetcher.h"
+
+#include <sstream>
+
+#include "content/public/browser/android/cdm_provision_fetcher.h"
+#include "media/base/bind_to_current_loop.h"
+#include "net/url_request/url_fetcher.h"
+
+namespace content {
+
+using net::URLFetcher;
+
+URLProvisionFetcher::URLProvisionFetcher(net::URLRequestContextGetter* context)
+    : url_request_context_(context) {}
+
+URLProvisionFetcher::~URLProvisionFetcher() {}
+
+void URLProvisionFetcher::Retrieve(
+    const std::string& default_url,
+    const std::string& request_data,
+    const media::ProvisionFetcher::ResponseCB& response_cb) {

[xhwang, 2015/11/06 23:08:18]

I suppose we can't make multiple requests? How about

DCHECK(!request_);

[Tima Vaisburd, 2015/11/11 03:03:33]

Done.

+  response_cb_ = media::BindToCurrentLoop(response_cb);

[xhwang, 2015/11/06 23:08:18]

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...

+
+  std::ostringstream os;
+  os << default_url << "&signedRequest=" << request_data;

[xhwang, 2015/11/06 23:08:18]

Is this "signedRequest" part documented anywhere?

[Tima Vaisburd, 2015/11/11 03:03:33]

This is what the java code did. The only doc I found is this:
http://developer.android.com/reference/android/media/MediaDrm.ProvisionReques...

[xhwang, 2015/11/11 09:53:21]

Yeah, it's like magic...

+qinmin, do you remember where get this?

+  DVLOG(1) << __FUNCTION__ << ": request:" << os.str();
+
+  request_ = URLFetcher::Create(GURL(os.str()), URLFetcher::POST, this);

[xhwang, 2015/11/06 23:08:18]

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?

[Tima Vaisburd, 2015/11/11 03:03:33]

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?

[xhwang, 2015/11/11 09:53:20]

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...

[Tima Vaisburd, 2015/11/11 23:26:35]

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.

+
+  // SetUploadData is mandatory even if we are not uploading anything.
+  request_->SetUploadData("", "");
+  request_->AddExtraRequestHeader("User-Agent: Widevine CDM v1.0");
+  request_->AddExtraRequestHeader("Content-Type: application/json");
+
+  DCHECK(url_request_context_);
+  request_->SetRequestContext(url_request_context_);
+
+  request_->Start();
+}
+
+void URLProvisionFetcher::OnURLFetchComplete(const net::URLFetcher* source) {
+  DCHECK(source);
+
+  int response_code = source->GetResponseCode();
+  DVLOG(1) << __FUNCTION__ << ": response code:" << source->GetResponseCode();
+
+  std::string response;
+  bool success = false;
+  if (response_code == 200) {
+    success = source->GetResponseAsString(&response);
+    if (!success)
+      DVLOG(1) << __FUNCTION__ << ": GetResponseAsString() failed";

[xhwang, 2015/11/06 23:08:18]

nit: Use DVLOG_IF

[Tima Vaisburd, 2015/11/11 03:03:33]

Done.

+  } else {
+    DVLOG(1) << "CDM provision: server returned error code " << response_code;
+  }
+
+  request_.reset();
+
+  DCHECK(!response_cb_.is_null());
+  response_cb_.Run(success, response);
+}
+
+// CDMProvisionFetcher implementation.
+
+// static
+scoped_ptr<media::ProvisionFetcher> CDMProvisionFetcher::CreateWithURLContext(

[xhwang, 2015/11/06 23:08:18]

Why do we need CDMProvisionFetcher?

[Tima Vaisburd, 2015/11/11 03:03:33]

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?

[xhwang, 2015/11/11 09:53:20]

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?

[Tima Vaisburd, 2015/11/11 23:26:35]

Yes, created a factory function CreateProvisionFetcherFactory() - added
...Factory() because now there is one more redirection. See my reply and
question in BrowserCdmManager.

+    net::URLRequestContextGetter* context) {
+  return scoped_ptr<media::ProvisionFetcher>(new URLProvisionFetcher(context));
+}
+
+}  // namespace content