media: Add ProvisionFetcher mojo interface and implementation.

media: Add ProvisionFetcher mojo interface and implementation.

Add a mojo servive to do provisioning for MediaDrm in a non-browser process.

BUG=546107
TEST=Tested running MediaDrmBridge in the GPU process and performing
provisioning in the Browser process through this mojo service.

Committed: https://crrev.com/ee71a525527d3ae607bb4f0d7a747014d310ecae
Cr-Commit-Position: refs/heads/master@{#361521}

[xhwang, 2015-11-20 21:27:58]

rebase only

[xhwang, 2015-11-20 22:28:34]

xhwang@chromium.org changed reviewers:
+ rockot@chromium.org, timav@chromium.org

[xhwang, 2015-11-20 22:28:34]

rockot: Please review from mojo's perspective.
timav: Please review from media's perspective.

https://chromiumcodereview.appspot.com/1466633002/diff/60001/content/browser/...
File content/browser/frame_host/frame_mojo_shell.cc (right):

https://chromiumcodereview.appspot.com/1466633002/diff/60001/content/browser/...
content/browser/frame_host/frame_mojo_shell.cc:101: registry.get(),
frame_host_);
rockot:

These services will always be provided by the browser no matter what app we are
connecting to. But really some services are only provided to certain app. For
example, the ProvisionFetcher service is only needed by the MojoMediaApplication
on Android.

I wonder whether we should has some means to only provide some service when
certain app is being connected.

https://chromiumcodereview.appspot.com/1466633002/diff/60001/content/browser/...
File content/browser/media/android/provision_fetcher_impl.h (right):

https://chromiumcodereview.appspot.com/1466633002/diff/60001/content/browser/...
content/browser/media/android/provision_fetcher_impl.h:44:
scoped_ptr<media::ProvisionFetcher> provision_fetcher_;
rockot:

We have this "sandwich" model in many places to support some functionality that
is implemented in one process but used in another process. In this particular
case, we have:

media::ProvisionFetcher (in Browser) <-
media::interfaces::ProvisionFetcher <-
IPC/Mojo <-
media::interfaces::ProvisionFetcherPtr <-
media::ProvisionFetcher (in  GPU)

It's working well but I feel we have too many boilerplate code. Is there any way
to simply this model?

https://chromiumcodereview.appspot.com/1466633002/diff/60001/media/mojo/inter...
File media/mojo/interfaces/mojo_bindings.gyp (right):

https://chromiumcodereview.appspot.com/1466633002/diff/60001/media/mojo/inter...
media/mojo/interfaces/mojo_bindings.gyp:50: ],
rockot:

Does Mandoline support gyp build at all?

I have to add these because content_browser is still gypi-only :(

https://chromiumcodereview.appspot.com/1466633002/diff/60001/media/mojo/servi...
File media/mojo/services/android_mojo_media_client.cc (right):

https://chromiumcodereview.appspot.com/1466633002/diff/60001/media/mojo/servi...
media/mojo/services/android_mojo_media_client.cc:37:
base::Bind(&CreateProvisionFetcher, service_provider)));
timav: Now when we create a MediaDrmBridge, we'll automatically create the
Fetcher, which will connect to the mojo service, and create a
UrlProvisionFetcher in the browser process. Since we don't really need
provisioning service all the time, maybe we should only create the Fetcher when
we need it. I'll address this in a separate CL.

[Ken Rockot(use gerrit already), 2015-11-24 17:35:58]

mojo bits lgtm modulo some nits

https://codereview.chromium.org/1466633002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/frame_mojo_shell.cc (right):

https://codereview.chromium.org/1466633002/diff/60001/content/browser/frame_h...
content/browser/frame_host/frame_mojo_shell.cc:101: registry.get(),
frame_host_);
On 2015/11/20 at 22:28:34, xhwang wrote:
> rockot:
> 
> These services will always be provided by the browser no matter what app we
are connecting to. But really some services are only provided to certain app.
For example, the ProvisionFetcher service is only needed by the
MojoMediaApplication on Android.
> 
> I wonder whether we should has some means to only provide some service when
certain app is being connected.

Side note: After getting thoroughly confused by this I think I'm realizing we
need to reconsider how things are done here so it's more clear.

In general though I think it's OK to leave things as they are for now. Maybe we
should just initialize a new ServiceProvider for every outgoing application
connection. It wouldn't be much additional overhead.

No need to worry about it for this CL though IMHO.

https://codereview.chromium.org/1466633002/diff/60001/content/browser/media/a...
File content/browser/media/android/provision_fetcher_impl.cc (right):

https://codereview.chromium.org/1466633002/diff/60001/content/browser/media/a...
content/browser/media/android/provision_fetcher_impl.cc:25: request.Pass());
exciting (IMHO!) C++11 nit: here and elsewhere, please use std::move(foo)
instead of foo.Pass().

https://codereview.chromium.org/1466633002/diff/60001/media/mojo/interfaces/m...
File media/mojo/interfaces/mojo_bindings.gyp (right):

https://codereview.chromium.org/1466633002/diff/60001/media/mojo/interfaces/m...
media/mojo/interfaces/mojo_bindings.gyp:50: ],
On 2015/11/20 at 22:28:34, xhwang wrote:
> rockot:
> 
> Does Mandoline support gyp build at all?

No, and it won't

> 
> I have to add these because content_browser is still gypi-only :(

Some day soon we won't have to worry about this anymore. :)

https://codereview.chromium.org/1466633002/diff/60001/media/mojo/interfaces/p...
File media/mojo/interfaces/provision_fetcher.mojom (right):

https://codereview.chromium.org/1466633002/diff/60001/media/mojo/interfaces/p...
media/mojo/interfaces/provision_fetcher.mojom:5: module media.interfaces;
BTW: I think we'd like to standardize on .mojom for module suffixes rather than
.interfaces. I'm not asking you to change this here or update all media mojom
now, but it might be worth at least filing a bug to update these names over
time.

https://codereview.chromium.org/1466633002/diff/60001/media/mojo/interfaces/p...
media/mojo/interfaces/provision_fetcher.mojom:15: // |response| will be null iff
|result| is false.
Why bother having |result| at all if |response| is nullable and (result ==
!!response)?

[xhwang, 2015-11-24 19:05:22]

comments addressed

[xhwang, 2015-11-24 19:43:22]

https://chromiumcodereview.appspot.com/1466633002/diff/60001/content/browser/...
File content/browser/frame_host/frame_mojo_shell.cc (right):

https://chromiumcodereview.appspot.com/1466633002/diff/60001/content/browser/...
content/browser/frame_host/frame_mojo_shell.cc:101: registry.get(),
frame_host_);
On 2015/11/24 17:35:58, Ken Rockot (OOO until 4 Dec) wrote:
> On 2015/11/20 at 22:28:34, xhwang wrote:
> > rockot:
> > 
> > These services will always be provided by the browser no matter what app we
> are connecting to. But really some services are only provided to certain app.
> For example, the ProvisionFetcher service is only needed by the
> MojoMediaApplication on Android.
> > 
> > I wonder whether we should has some means to only provide some service when
> certain app is being connected.
> 
> Side note: After getting thoroughly confused by this I think I'm realizing we
> need to reconsider how things are done here so it's more clear.
> 
> In general though I think it's OK to leave things as they are for now. Maybe
we
> should just initialize a new ServiceProvider for every outgoing application
> connection. It wouldn't be much additional overhead.
> 
> No need to worry about it for this CL though IMHO.

Ack. Added a TODO to remind us of this issue/discussion.

https://chromiumcodereview.appspot.com/1466633002/diff/60001/content/browser/...
File content/browser/media/android/provision_fetcher_impl.cc (right):

https://chromiumcodereview.appspot.com/1466633002/diff/60001/content/browser/...
content/browser/media/android/provision_fetcher_impl.cc:25: request.Pass());
On 2015/11/24 17:35:58, Ken Rockot (OOO until 4 Dec) wrote:
> exciting (IMHO!) C++11 nit: here and elsewhere, please use std::move(foo)
> instead of foo.Pass().

Done.

https://chromiumcodereview.appspot.com/1466633002/diff/60001/media/mojo/inter...
File media/mojo/interfaces/mojo_bindings.gyp (right):

https://chromiumcodereview.appspot.com/1466633002/diff/60001/media/mojo/inter...
media/mojo/interfaces/mojo_bindings.gyp:50: ],
On 2015/11/24 17:35:58, Ken Rockot (OOO until 4 Dec) wrote:
> On 2015/11/20 at 22:28:34, xhwang wrote:
> > rockot:
> > 
> > Does Mandoline support gyp build at all?
> 
> No, and it won't
> 
> > 
> > I have to add these because content_browser is still gypi-only :(
> 
> Some day soon we won't have to worry about this anymore. :)

Acknowledged.

https://chromiumcodereview.appspot.com/1466633002/diff/60001/media/mojo/inter...
File media/mojo/interfaces/provision_fetcher.mojom (right):

https://chromiumcodereview.appspot.com/1466633002/diff/60001/media/mojo/inter...
media/mojo/interfaces/provision_fetcher.mojom:5: module media.interfaces;
On 2015/11/24 17:35:58, Ken Rockot (OOO until 4 Dec) wrote:
> BTW: I think we'd like to standardize on .mojom for module suffixes rather
than
> .interfaces. I'm not asking you to change this here or update all media mojom
> now, but it might be worth at least filing a bug to update these names over
> time.

I asked about this before and at that time there was no standard. That's why we
are following some existing code and use "interfaces" as our standard.

So what's the new standard now?

https://chromiumcodereview.appspot.com/1466633002/diff/60001/media/mojo/inter...
media/mojo/interfaces/provision_fetcher.mojom:15: // |response| will be null iff
|result| is false.
On 2015/11/24 17:35:58, Ken Rockot (OOO until 4 Dec) wrote:
> Why bother having |result| at all if |response| is nullable and (result ==
> !!response)?

That would work. But personally I'd like to be explicit. Also this matches the
C++ interface, where you don't have a nullable string. You could use empty
string to mean "failure" but again I feel that's confusing.

[xhwang, 2015-11-24 19:45:44]

xhwang@chromium.org changed reviewers:
+ jam@chromium.org

[xhwang, 2015-11-24 19:45:44]

timav: PTAL as well.

jam: Please OWNERS review changes in 
content/browser/frame_host/frame_mojo_shell.cc.

[Tima Vaisburd, 2015-11-24 19:49:51]

lgtm

[Ken Rockot(use gerrit already), 2015-11-24 20:05:29]

https://chromiumcodereview.appspot.com/1466633002/diff/60001/media/mojo/inter...
File media/mojo/interfaces/provision_fetcher.mojom (right):

https://chromiumcodereview.appspot.com/1466633002/diff/60001/media/mojo/inter...
media/mojo/interfaces/provision_fetcher.mojom:5: module media.interfaces;
On 2015/11/24 at 19:43:21, xhwang wrote:
> On 2015/11/24 17:35:58, Ken Rockot (OOO until 4 Dec) wrote:
> > BTW: I think we'd like to standardize on .mojom for module suffixes rather
than
> > .interfaces. I'm not asking you to change this here or update all media
mojom
> > now, but it might be worth at least filing a bug to update these names over
> > time.
> 
> I asked about this before and at that time there was no standard. That's why
we are following some existing code and use "interfaces" as our standard.
> 

Indeed, and totally understandable

> So what's the new standard now?

Yeah there were a lot of ultimately unresolved discussions around whether to
have any convention at all, or whether the impls should be in an inner impl
namespace, or whatever.

I think there's now a consensus that it's beneficial to have an inner namespace
for mojom types. Given that, it's only a question of what conventional naming to
use. Other components (such as mus) are going with mojom for that name, and
given that len("mojom") < len("interfaces"), I think mojom wins in the long term
:)

Again, not a big issue, but we should be eventually consistent across src

[xhwang, 2015-11-24 20:13:14]

On 2015/11/24 20:05:29, Ken Rockot (OOO slow til 12-4) wrote:
>
https://chromiumcodereview.appspot.com/1466633002/diff/60001/media/mojo/inter...
> File media/mojo/interfaces/provision_fetcher.mojom (right):
> 
>
https://chromiumcodereview.appspot.com/1466633002/diff/60001/media/mojo/inter...
> media/mojo/interfaces/provision_fetcher.mojom:5: module media.interfaces;
> On 2015/11/24 at 19:43:21, xhwang wrote:
> > On 2015/11/24 17:35:58, Ken Rockot (OOO until 4 Dec) wrote:
> > > BTW: I think we'd like to standardize on .mojom for module suffixes rather
> than
> > > .interfaces. I'm not asking you to change this here or update all media
> mojom
> > > now, but it might be worth at least filing a bug to update these names
over
> > > time.
> > 
> > I asked about this before and at that time there was no standard. That's why
> we are following some existing code and use "interfaces" as our standard.
> > 
> 
> Indeed, and totally understandable
> 
> > So what's the new standard now?
> 
> Yeah there were a lot of ultimately unresolved discussions around whether to
> have any convention at all, or whether the impls should be in an inner impl
> namespace, or whatever.
> 
> I think there's now a consensus that it's beneficial to have an inner
namespace
> for mojom types. Given that, it's only a question of what conventional naming
to
> use. Other components (such as mus) are going with mojom for that name, and
> given that len("mojom") < len("interfaces"), I think mojom wins in the long
term
> :)
> 
> Again, not a big issue, but we should be eventually consistent across src

Not sure I am a fan of a "mojom" namespace :) Currently we have so many tasks so
I'll wait a bit to make this change. By then, hopefully we'll have a more stable
convention on this.

[jam, 2015-11-25 00:04:44]

lgtm

[xhwang, 2015-11-25 00:05:50]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2015-11-25 00:05:51]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
timav@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1466633002/#ps100001 (title: "fix
std::move use")

[commit-bot: I haz the power, 2015-11-25 00:09:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1466633002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1466633002/100001

[commit-bot: I haz the power, 2015-11-25 02:12:14]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-11-25 02:12:55]

Patchset 6 (id:??) landed as
https://crrev.com/ee71a525527d3ae607bb4f0d7a747014d310ecae
Cr-Commit-Position: refs/heads/master@{#361521}

[dominickn, 2015-11-25 05:01:08]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1473233005/ by dominickn@chromium.org.

The reason for reverting is: Breaks GN Android compilation with error:

In file included from
../../content/browser/media/android/provision_fetcher_impl.cc:5:
../../content/browser/media/android/provision_fetcher_impl.h:13:10: fatal error:
'media/mojo/interfaces/provision_fetcher.mojom.h' file not found
#include "media/mojo/interfaces/provision_fetcher.mojom.h"

CC=lizeb,tedchoc,xhwang.