Add support for MediaUrlInterceptor

Add MediaUrlInterceptor support to MediaPlayerRenderer

This change allows webview apps to register a custom
MediaUrlInterceptor. This change simply copies the code in
BrowserMediaPlayerManager.

BUG=636588
Committed: https://crrev.com/251a73761e1da752b87eeb5ea7e22a5b1f164a85
Cr-Commit-Position: refs/heads/master@{#432060}

[tguilbert, 2016-10-28 22:06:17]

Description was changed from

==========
Add support for MediaUrlInterceptor

This change allows webview apps to register a custom
MediaUrlInterceptor. This blatantly copies the code in
BrowserMediaPlayerManager.

BUG=636588
==========

to

==========
Add support for MediaUrlInterceptor

This change allows webview apps to register a custom
MediaUrlInterceptor. This change simply copies the code in
BrowserMediaPlayerManager.

BUG=636588
==========

[tguilbert, 2016-10-28 22:06:37]

tguilbert@chromium.org changed reviewers:
+ watk@chromium.org

[tguilbert, 2016-10-28 22:06:38]

PTAL!

Thanks

[watk, 2016-10-28 22:34:48]

lgtm % comment

https://codereview.chromium.org/2463533002/diff/1/content/browser/media/andro...
File content/browser/media/android/media_player_renderer.cc (right):

https://codereview.chromium.org/2463533002/diff/1/content/browser/media/andro...
content/browser/media/android/media_player_renderer.cc:24: static
media::MediaUrlInterceptor* media_url_interceptor_ = nullptr;
This should have the g_ prefix (for global). And I believe the more common
convention is to use an anonymous namespace and remove the static keyword. e.g.,
https://cs.chromium.org/chromium/src/base/logging.cc?sq=package:chromium&l=82...

[watk, 2016-10-28 22:35:30]

And perhaps change the description to Add support for MediaUrlInterceptor to
MediaPlayerRenderer

[tguilbert, 2016-10-28 23:03:04]

Description was changed from

==========
Add support for MediaUrlInterceptor

This change allows webview apps to register a custom
MediaUrlInterceptor. This change simply copies the code in
BrowserMediaPlayerManager.

BUG=636588
==========

to

==========
Add MediaUrlInterceptor to MediaPlayerRenderer

This change allows webview apps to register a custom
MediaUrlInterceptor. This change simply copies the code in
BrowserMediaPlayerManager.

BUG=636588
==========

[tguilbert, 2016-10-28 23:08:05]

tguilbert@chromium.org changed reviewers:
+ boliu@chromium.org

[tguilbert, 2016-10-28 23:08:05]

@watk -- Updated. Thanks!

@boliu -- Please review changes in content/public/browser/*

Thank you!
Thomas

[tguilbert, 2016-10-28 23:08:20]

Description was changed from

==========
Add MediaUrlInterceptor to MediaPlayerRenderer

This change allows webview apps to register a custom
MediaUrlInterceptor. This change simply copies the code in
BrowserMediaPlayerManager.

BUG=636588
==========

to

==========
Add MediaUrlInterceptor support to MediaPlayerRenderer

This change allows webview apps to register a custom
MediaUrlInterceptor. This change simply copies the code in
BrowserMediaPlayerManager.

BUG=636588
==========

[boliu, 2016-10-28 23:14:34]

rs lgtm

[tguilbert, 2016-10-28 23:15:43]

The CQ bit was checked by tguilbert@chromium.org

[tguilbert, 2016-10-28 23:15:43]

The patchset sent to the CQ was uploaded after l-g-t-m from watk@chromium.org
Link to the patchset: https://codereview.chromium.org/2463533002/#ps20001
(title: "Addressed comments")

[commit-bot: I haz the power, 2016-10-28 23:16:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-29 00:32:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-29 00:32:53]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[tguilbert, 2016-10-31 20:53:37]

The CQ bit was checked by tguilbert@chromium.org

[commit-bot: I haz the power, 2016-10-31 20:54:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-31 22:20:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-31 22:20:48]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[tguilbert, 2016-11-03 22:43:14]

On 2016/10/31 22:20:48, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
>
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

Mini update by the way:
The test failures are caused by this check --
https://cs.chromium.org/chromium/src/base/feature_list.cc?sq=package:chromium...

This happens because we are not calling
base::FeatureList::ClearInstanceForTesting(). However, all the webview tests are
in Java, and there is no way to call this method. It's not worth plumbing it to
Java IMO, and I will wait until we switch MediaPlayerRenderer to be enabled by
default to make this change unconditional.

[boliu, 2016-11-03 22:50:22]

On 2016/11/03 22:43:14, tguilbert wrote:
> On 2016/10/31 22:20:48, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
> >
>
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
> 
> Mini update by the way:
> The test failures are caused by this check --
>
https://cs.chromium.org/chromium/src/base/feature_list.cc?sq=package:chromium...
> 
> This happens because we are not calling
> base::FeatureList::ClearInstanceForTesting(). However, all the webview tests
are
> in Java, and there is no way to call this method. It's not worth plumbing it
to
> Java IMO, and I will wait until we switch MediaPlayerRenderer to be enabled by
> default to make this change unconditional.

why is it only a problem on webview? it's enabled on chrome already? I mean
surely this is not the only FeatureList thing that webview tests hit..

[tguilbert, 2016-11-15 00:04:56]

On 2016/11/03 22:50:22, boliu wrote:
> On 2016/11/03 22:43:14, tguilbert wrote:
> > On 2016/10/31 22:20:48, commit-bot: I haz the power wrote:
> > > Try jobs failed on following builders:
> > >   android_n5x_swarming_rel on master.tryserver.chromium.android
(JOB_FAILED,
> > >
> >
>
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
> > 
> > Mini update by the way:
> > The test failures are caused by this check --
> >
>
https://cs.chromium.org/chromium/src/base/feature_list.cc?sq=package:chromium...
> > 
> > This happens because we are not calling
> > base::FeatureList::ClearInstanceForTesting(). However, all the webview tests
> are
> > in Java, and there is no way to call this method. It's not worth plumbing it
> to
> > Java IMO, and I will wait until we switch MediaPlayerRenderer to be enabled
by
> > default to make this change unconditional.
> 
> why is it only a problem on webview? it's enabled on chrome already? I mean
> surely this is not the only FeatureList thing that webview tests hit..

Ah, sorry, only saw this now.

This is called here:
https://cs.chromium.org/chromium/src/android_webview/lib/main/aw_main_delegat...

This method is only called in webview, and it causes failures because there is
no JNI in place to allow for the webview Java tests to call
FeatureList::ClearInstanceForTesting().

I am going to submit the Media portion while only adding a comment to the
browser portion.

[tguilbert, 2016-11-15 00:05:43]

The CQ bit was checked by tguilbert@chromium.org

[tguilbert, 2016-11-15 00:05:43]

The patchset sent to the CQ was uploaded after l-g-t-m from watk@chromium.org,
boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2463533002/#ps40001
(title: "Removed registration, added TODO")

[commit-bot: I haz the power, 2016-11-15 00:06:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 02:14:20]

Description was changed from

==========
Add MediaUrlInterceptor support to MediaPlayerRenderer

This change allows webview apps to register a custom
MediaUrlInterceptor. This change simply copies the code in
BrowserMediaPlayerManager.

BUG=636588
==========

to

==========
Add MediaUrlInterceptor support to MediaPlayerRenderer

This change allows webview apps to register a custom
MediaUrlInterceptor. This change simply copies the code in
BrowserMediaPlayerManager.

BUG=636588
==========

[commit-bot: I haz the power, 2016-11-15 02:14:23]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-15 03:00:56]

Description was changed from

==========
Add MediaUrlInterceptor support to MediaPlayerRenderer

This change allows webview apps to register a custom
MediaUrlInterceptor. This change simply copies the code in
BrowserMediaPlayerManager.

BUG=636588
==========

to

==========
Add MediaUrlInterceptor support to MediaPlayerRenderer

This change allows webview apps to register a custom
MediaUrlInterceptor. This change simply copies the code in
BrowserMediaPlayerManager.

BUG=636588

Committed: https://crrev.com/251a73761e1da752b87eeb5ea7e22a5b1f164a85
Cr-Commit-Position: refs/heads/master@{#432060}
==========

[commit-bot: I haz the power, 2016-11-15 03:00:57]

Patchset 3 (id:??) landed as
https://crrev.com/251a73761e1da752b87eeb5ea7e22a5b1f164a85
Cr-Commit-Position: refs/heads/master@{#432060}