Pass Chrome user agent string to Android media player

Pass Chrome user agent string to Android media player

Android Media player, by default, uses its own user agent string in the format
of "stagefright/1.2 (Linux; Android x.x)" when sending out requests.
This CL changes it to that of Chrome for Android when the media player instance is hosted by
Chrome for Android (or ChromiumWebView) to make the UA used across the browser
and its media source consistent.

R=qinmin@chromium.org
BUG=332344
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=244130

[Jinsuk Kim, 2014-01-09 01:38:31]

Hi xhwang, I added you to the reviewer to review the change made in media/DEPS.
Would you take a look? Thanks.

[ycheo (away), 2014-01-09 01:49:05]

On 2014/01/09 01:38:31, jindor wrote:
> Hi xhwang, I added you to the reviewer to review the change made in
media/DEPS.
> Would you take a look? Thanks.

Can you consider to inject UA into MediaPlayerBridge from
BrowserMediaPlayerManager in order to avoid from changing DEPS?

[Jinsuk Kim, 2014-01-09 03:58:06]

On 2014/01/09 01:49:05, Yuncheol Heo wrote:
> On 2014/01/09 01:38:31, jindor wrote:
> > Hi xhwang, I added you to the reviewer to review the change made in
> media/DEPS.
> > Would you take a look? Thanks.
> 
> Can you consider to inject UA into MediaPlayerBridge from
> BrowserMediaPlayerManager in order to avoid from changing DEPS?

Thanks for the suggestion. That sounds better. Done.

Hi xhwang, You need not review the code any more but let me still keep you in
the reviewer.

[qinmin, 2014-01-09 04:48:43]

https://codereview.chromium.org/130363002/diff/50001/content/browser/media/an...
File content/browser/media/android/media_resource_getter_impl.cc (right):

https://codereview.chromium.org/130363002/diff/50001/content/browser/media/an...
content/browser/media/android/media_resource_getter_impl.cc:48: env,
GetUserAgent(GURL(url)));
you can pass useragent to GetMediaMetadata() from MediaPlayerBridge

[Jinsuk Kim, 2014-01-09 05:22:54]

Thanks for the review. Please take another look.

Hi Marcus, I added you as a reviewer since the CL touches content/public/android
of which you are one of the owners. Would you review the change and give the
owner's seal if it looks good? 

Thanks!

https://codereview.chromium.org/130363002/diff/50001/content/browser/media/an...
File content/browser/media/android/media_resource_getter_impl.cc (right):

https://codereview.chromium.org/130363002/diff/50001/content/browser/media/an...
content/browser/media/android/media_resource_getter_impl.cc:48: env,
GetUserAgent(GURL(url)));
On 2014/01/09 04:48:44, qinmin wrote:
> you can pass useragent to GetMediaMetadata() from MediaPlayerBridge

Done. See if this is done as you suggested.

[ycheo (away), 2014-01-09 05:45:00]

On 2014/01/09 05:22:54, jindor wrote:
> Thanks for the review. Please take another look.
> 
> Hi Marcus, I added you as a reviewer since the CL touches
content/public/android
> of which you are one of the owners. Would you review the change and give the
> owner's seal if it looks good? 
> 
> Thanks!
> 
>
https://codereview.chromium.org/130363002/diff/50001/content/browser/media/an...
> File content/browser/media/android/media_resource_getter_impl.cc (right):
> 
>
https://codereview.chromium.org/130363002/diff/50001/content/browser/media/an...
> content/browser/media/android/media_resource_getter_impl.cc:48: env,
> GetUserAgent(GURL(url)));
> On 2014/01/09 04:48:44, qinmin wrote:
> > you can pass useragent to GetMediaMetadata() from MediaPlayerBridge
> 
> Done. See if this is done as you suggested.

lgtm

[qinmin, 2014-01-09 06:01:34]

lgtm

[bulach, 2014-01-09 14:23:01]

lgtm % nit, thanks!

avayvod may want to take a look at the media player bridge..

https://codereview.chromium.org/130363002/diff/110001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/130363002/diff/110001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:80: const
std::string& user_agent = GetUserAgent(url);
nit: keep a string value rather than a reference

[whywhat, 2014-01-09 15:00:18]

Thanks for a heads up, Marcus.

This will certainly break our downstream class that inherits from
MediaPlayerBridge, I will prepare a fix and let the gardeners know.

Otherwise, LGTM.

[xhwang, 2014-01-09 17:51:14]

drive-by comments

https://codereview.chromium.org/130363002/diff/110001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/130363002/diff/110001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:80: const
std::string& user_agent = GetUserAgent(url);
Based on the documentation:
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com...

"SetContentClient() must be called prior to calling this..."

Do you know where we called SetContentClient()?

https://codereview.chromium.org/130363002/diff/110001/media/base/android/medi...
File media/base/android/media_player_bridge.cc (right):

https://codereview.chromium.org/130363002/diff/110001/media/base/android/medi...
media/base/android/media_player_bridge.cc:33: manager),
nit: this can fit in one line

https://codereview.chromium.org/130363002/diff/110001/media/base/android/medi...
File media/base/android/media_player_bridge.h (right):

https://codereview.chromium.org/130363002/diff/110001/media/base/android/medi...
media/base/android/media_player_bridge.h:145: const std::string& user_agent_;
Just make a copy instead of using the ref?

[scherkus (not reviewing), 2014-01-10 00:08:54]

any possibility for a test?

[Jinsuk Kim, 2014-01-10 01:19:12]

Thanks for the review.

https://codereview.chromium.org/130363002/diff/110001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/130363002/diff/110001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:80: const
std::string& user_agent = GetUserAgent(url);
On 2014/01/09 14:23:02, bulach wrote:
> nit: keep a string value rather than a reference

Done.

https://codereview.chromium.org/130363002/diff/110001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:80: const
std::string& user_agent = GetUserAgent(url);
On 2014/01/09 17:51:14, xhwang wrote:
> Based on the documentation:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com...
> 
> "SetContentClient() must be called prior to calling this..."
> 
> Do you know where we called SetContentClient()?

It's called when Chrome is initialized in
ChromeMainDelegate::BasicStartupComplete. For ChromiumWebView, it's called in
AwMainDelegate::BasicStartupComplete.

https://codereview.chromium.org/130363002/diff/110001/media/base/android/medi...
File media/base/android/media_player_bridge.h (right):

https://codereview.chromium.org/130363002/diff/110001/media/base/android/medi...
media/base/android/media_player_bridge.h:145: const std::string& user_agent_;
On 2014/01/09 17:51:14, xhwang wrote:
> Just make a copy instead of using the ref?

Done.

[Jinsuk Kim, 2014-01-10 01:23:25]

On 2014/01/10 00:08:54, scherkus wrote:
> any possibility for a test?

I don't have a good idea as to how to test this change. Let me take some time
for it and come back later. Thanks.

[Jinsuk Kim, 2014-01-10 01:26:09]

On 2014/01/09 15:00:18, whywhat wrote:
> Thanks for a heads up, Marcus.
> 
> This will certainly break our downstream class that inherits from
> MediaPlayerBridge, I will prepare a fix and let the gardeners know.
> 
> Otherwise, LGTM.

Thanks for informing the gardeners of it. I've got a counterpart CL on clank
side to be submitted right after this CL but it will likely break the build
temporarily.

[commit-bot: I haz the power, 2014-01-10 01:27:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jinsukkim@chromium.org/130363002/310001

[commit-bot: I haz the power, 2014-01-10 05:21:36]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-10 06:30:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jinsukkim@chromium.org/130363002/310001

[commit-bot: I haz the power, 2014-01-10 08:44:43]

Change committed as 244130