MediaUrlInterceptor for embedders to handle custom urls.

MediaUrlInterceptor for embedders to handle custom urls.

This change adds a MediaUrlInterceptor that content embedders
can register to handle custom urls. The MediaUrlInterceptor
translates a url into a media file. The motivation for this change
is to allow the Android WebView to play media files from the
assets in the apk.

BUG=387898
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=286506

[Ignacio Solla, 2014-07-24 14:20:49]

hi Min,

Are there any tests already for browser_media_player_manager and
media_player_bridge that I could extend? I was not able to find any.

benm@chromium.org: Please review changes in 

qinmin@chromium.org: Please review changes in

[qinmin, 2014-07-24 21:05:28]

https://codereview.chromium.org/411353002/diff/1/content/browser/media/androi...
File content/browser/media/android/media_resource_getter_impl.cc (right):

https://codereview.chromium.org/411353002/diff/1/content/browser/media/androi...
content/browser/media/android/media_resource_getter_impl.cc:119: // Get the
metadata from a file descriptor. When finished, a task is posted to
s/Get/Gets/

https://codereview.chromium.org/411353002/diff/1/content/browser/media/androi...
content/browser/media/android/media_resource_getter_impl.cc:122: const int& fd,
const int64& offset, const int64& size,
same for const &

https://codereview.chromium.org/411353002/diff/1/content/browser/media/androi...
content/browser/media/android/media_resource_getter_impl.cc:355: const int& fd,
const int64& offset, const int64& size,
same for const &

https://codereview.chromium.org/411353002/diff/1/content/browser/media/androi...
File content/browser/media/android/media_resource_getter_impl.h (right):

https://codereview.chromium.org/411353002/diff/1/content/browser/media/androi...
content/browser/media/android/media_resource_getter_impl.h:62: const int& fd,
no const reference needed

https://codereview.chromium.org/411353002/diff/1/media/base/android/media_pla...
File media/base/android/media_player_bridge.cc (right):

https://codereview.chromium.org/411353002/diff/1/media/base/android/media_pla...
media/base/android/media_player_bridge.cc:5: #include
<media/base/android/media_url_interceptor.h>
remote this, the file is included below

https://codereview.chromium.org/411353002/diff/1/media/base/android/media_pla...
media/base/android/media_player_bridge.cc:184: ScopedJavaLocalRef<jstring>
j_cookies = ConvertUTF8ToJavaString(
j_cookies and j_user_agent declaration can be moved after the dataUri if
statement, so for data uri we don't need to create these 2 variables.

https://codereview.chromium.org/411353002/diff/1/media/base/android/media_pla...
File media/base/android/media_player_manager.h (right):

https://codereview.chromium.org/411353002/diff/1/media/base/android/media_pla...
media/base/android/media_player_manager.h:27: // Return a pointer to the
MediaUrlInterceptor object or null.
s/Return/Returns/

https://codereview.chromium.org/411353002/diff/1/media/base/android/media_res...
File media/base/android/media_resource_getter.h (right):

https://codereview.chromium.org/411353002/diff/1/media/base/android/media_res...
media/base/android/media_resource_getter.h:8: #include
<media/base/android/media_url_interceptor.h>
remote this

https://codereview.chromium.org/411353002/diff/1/media/base/android/media_res...
media/base/android/media_resource_getter.h:62: // Extract the metadata from a
file descriptor. Once completed, the
s/Extract/Extracts/

https://codereview.chromium.org/411353002/diff/1/media/base/android/media_res...
media/base/android/media_resource_getter.h:65: const int& fd,
primitive types, just use int and int64

https://codereview.chromium.org/411353002/diff/1/media/base/android/media_url...
File media/base/android/media_url_interceptor.h (right):

https://codereview.chromium.org/411353002/diff/1/media/base/android/media_url...
media/base/android/media_url_interceptor.h:21: // Return true if the embedder
has intercepted the url and
s/Return/Returns/

https://codereview.chromium.org/411353002/diff/1/media/base/android/media_url...
media/base/android/media_url_interceptor.h:29: int* fd,
align the indentations with the param above

[mnaganov (inactive), 2014-07-25 08:17:49]

Some drive-by comments.

https://codereview.chromium.org/411353002/diff/1/content/browser/media/androi...
File content/browser/media/android/browser_media_player_manager.h (right):

https://codereview.chromium.org/411353002/diff/1/content/browser/media/androi...
content/browser/media/android/browser_media_player_manager.h:49: static void
RegisterMediaUrlInterceptor(
I really don't like the idea that the interceptor is static. Although probably
in reality there can be no more than one BrowserMediaPlayerManager instance,
this class is technically not a singleton. Can we set the interceptor an an
instance instead?

https://codereview.chromium.org/411353002/diff/1/content/browser/media/androi...
File content/browser/media/android/media_resource_getter_impl.cc (right):

https://codereview.chromium.org/411353002/diff/1/content/browser/media/androi...
content/browser/media/android/media_resource_getter_impl.cc:85:
ScopedJavaLocalRef<jobject>& j_metadata, JNIEnv* env) {
nit: JNIEnv usually goes before any other Java arguments.

https://codereview.chromium.org/411353002/diff/1/media/base/android/media_pla...
File media/base/android/media_player_bridge.cc (right):

https://codereview.chromium.org/411353002/diff/1/media/base/android/media_pla...
media/base/android/media_player_bridge.cc:175: if
(!Java_MediaPlayerBridge_setDataSourceFromFd(
I recommend initializing fd, offset and size to "invalid" values and adding
DCHECKs that the values are now valid, if the interceptor has returned 'true'.

https://codereview.chromium.org/411353002/diff/1/media/base/android/media_pla...
media/base/android/media_player_bridge.cc:254: fd, offset, size,
nit: 'ExtractMediaMetadata' arguments alignment is incoherent between the 'if'
statement branches.

https://codereview.chromium.org/411353002/diff/1/media/base/android/media_url...
File media/base/android/media_url_interceptor.h (right):

https://codereview.chromium.org/411353002/diff/1/media/base/android/media_url...
media/base/android/media_url_interceptor.h:31: int64* size) const = 0;
Pro-tip: use Clang format to do the dirty job for you
(https://code.google.com/p/chromium/wiki/ClangFormat)

[Ignacio Solla, 2014-07-25 10:35:07]

https://codereview.chromium.org/411353002/diff/1/content/browser/media/androi...
File content/browser/media/android/browser_media_player_manager.h (right):

https://codereview.chromium.org/411353002/diff/1/content/browser/media/androi...
content/browser/media/android/browser_media_player_manager.h:49: static void
RegisterMediaUrlInterceptor(
On 2014/07/25 08:17:49, Mikhail Naganov (Cr) wrote:
> I really don't like the idea that the interceptor is static. Although probably
> in reality there can be no more than one BrowserMediaPlayerManager instance,
> this class is technically not a singleton. Can we set the interceptor an an
> instance instead?

The interceptor is static because it should be shared across all the instances
of BrowserMediaPlayerManager. The binary decides whether to register an
interceptor at start-up time.

I had also tried to make it a field, but that requires providing our  own
BrowserMediaPlayerManager.Factory and AwBrowserMediaPlayerManager just to
register the interceptor, which makes the code obscure and is over-kill IMO. I
prefer the current version because it is simpler and the intent is clearer.

https://codereview.chromium.org/411353002/diff/1/content/browser/media/androi...
File content/browser/media/android/media_resource_getter_impl.cc (right):

https://codereview.chromium.org/411353002/diff/1/content/browser/media/androi...
content/browser/media/android/media_resource_getter_impl.cc:85:
ScopedJavaLocalRef<jobject>& j_metadata, JNIEnv* env) {
On 2014/07/25 08:17:49, Mikhail Naganov (Cr) wrote:
> nit: JNIEnv usually goes before any other Java arguments.

Done.

https://codereview.chromium.org/411353002/diff/1/content/browser/media/androi...
content/browser/media/android/media_resource_getter_impl.cc:119: // Get the
metadata from a file descriptor. When finished, a task is posted to
On 2014/07/24 21:05:27, qinmin wrote:
> s/Get/Gets/

Done.

https://codereview.chromium.org/411353002/diff/1/content/browser/media/androi...
content/browser/media/android/media_resource_getter_impl.cc:122: const int& fd,
const int64& offset, const int64& size,
On 2014/07/24 21:05:27, qinmin wrote:
> same for const &

Changed to const int and const int64.

https://codereview.chromium.org/411353002/diff/1/content/browser/media/androi...
content/browser/media/android/media_resource_getter_impl.cc:355: const int& fd,
const int64& offset, const int64& size,
On 2014/07/24 21:05:27, qinmin wrote:
> same for const &

Done.

https://codereview.chromium.org/411353002/diff/1/content/browser/media/androi...
File content/browser/media/android/media_resource_getter_impl.h (right):

https://codereview.chromium.org/411353002/diff/1/content/browser/media/androi...
content/browser/media/android/media_resource_getter_impl.h:62: const int& fd,
On 2014/07/24 21:05:27, qinmin wrote:
> no const reference needed

Done

https://codereview.chromium.org/411353002/diff/1/media/base/android/media_pla...
File media/base/android/media_player_bridge.cc (right):

https://codereview.chromium.org/411353002/diff/1/media/base/android/media_pla...
media/base/android/media_player_bridge.cc:5: #include
<media/base/android/media_url_interceptor.h>
On 2014/07/24 21:05:27, qinmin wrote:
> remote this, the file is included below

Done.

https://codereview.chromium.org/411353002/diff/1/media/base/android/media_pla...
media/base/android/media_player_bridge.cc:175: if
(!Java_MediaPlayerBridge_setDataSourceFromFd(
On 2014/07/25 08:17:49, Mikhail Naganov (Cr) wrote:
> I recommend initializing fd, offset and size to "invalid" values and adding
> DCHECKs that the values are now valid, if the interceptor has returned 'true'.

Done (and moved to private method to avoid duplication)

https://codereview.chromium.org/411353002/diff/1/media/base/android/media_pla...
media/base/android/media_player_bridge.cc:184: ScopedJavaLocalRef<jstring>
j_cookies = ConvertUTF8ToJavaString(
On 2014/07/24 21:05:27, qinmin wrote:
> j_cookies and j_user_agent declaration can be moved after the dataUri if
> statement, so for data uri we don't need to create these 2 variables.

Done.

https://codereview.chromium.org/411353002/diff/1/media/base/android/media_pla...
media/base/android/media_player_bridge.cc:254: fd, offset, size,
On 2014/07/25 08:17:49, Mikhail Naganov (Cr) wrote:
> nit: 'ExtractMediaMetadata' arguments alignment is incoherent between the 'if'
> statement branches.

Done.

https://codereview.chromium.org/411353002/diff/1/media/base/android/media_pla...
File media/base/android/media_player_manager.h (right):

https://codereview.chromium.org/411353002/diff/1/media/base/android/media_pla...
media/base/android/media_player_manager.h:27: // Return a pointer to the
MediaUrlInterceptor object or null.
On 2014/07/24 21:05:27, qinmin wrote:
> s/Return/Returns/

Done.

https://codereview.chromium.org/411353002/diff/1/media/base/android/media_res...
File media/base/android/media_resource_getter.h (right):

https://codereview.chromium.org/411353002/diff/1/media/base/android/media_res...
media/base/android/media_resource_getter.h:8: #include
<media/base/android/media_url_interceptor.h>
On 2014/07/24 21:05:27, qinmin wrote:
> remote this

Done.

https://codereview.chromium.org/411353002/diff/1/media/base/android/media_res...
media/base/android/media_resource_getter.h:62: // Extract the metadata from a
file descriptor. Once completed, the
On 2014/07/24 21:05:27, qinmin wrote:
> s/Extract/Extracts/

Done.

https://codereview.chromium.org/411353002/diff/1/media/base/android/media_res...
media/base/android/media_resource_getter.h:65: const int& fd,
On 2014/07/24 21:05:27, qinmin wrote:
> primitive types, just use int and int64

I use int and int64 now instead of references to them, but keep the const as
this method should not change their values. Let me know if removing the const is
preferable when the arguments are passed by value.

https://codereview.chromium.org/411353002/diff/1/media/base/android/media_url...
File media/base/android/media_url_interceptor.h (right):

https://codereview.chromium.org/411353002/diff/1/media/base/android/media_url...
media/base/android/media_url_interceptor.h:21: // Return true if the embedder
has intercepted the url and
On 2014/07/24 21:05:28, qinmin wrote:
> s/Return/Returns/

Done.

https://codereview.chromium.org/411353002/diff/1/media/base/android/media_url...
media/base/android/media_url_interceptor.h:29: int* fd,
On 2014/07/24 21:05:28, qinmin wrote:
> align the indentations with the param above

Done.

https://codereview.chromium.org/411353002/diff/1/media/base/android/media_url...
media/base/android/media_url_interceptor.h:31: int64* size) const = 0;
On 2014/07/25 08:17:49, Mikhail Naganov (Cr) wrote:
> Pro-tip: use Clang format to do the dirty job for you
> (https://code.google.com/p/chromium/wiki/ClangFormat)

Thanks for the tip.

(In this case I have decided not to use the format produced by ClangFormat as it
is inconcistent with the existing format in some of the files, and in some cases
I prefer having all the arguments in one line)

[qinmin, 2014-07-28 21:16:01]

lgtm % nits

https://codereview.chromium.org/411353002/diff/20001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/411353002/diff/20001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:53:
media::MediaUrlInterceptor* media_url_interceptor) {
fix the indent

https://codereview.chromium.org/411353002/diff/20001/media/base/android/media...
File media/base/android/media_player_bridge.cc (right):

https://codereview.chromium.org/411353002/diff/20001/media/base/android/media...
media/base/android/media_player_bridge.cc:221:
manager()->GetMediaUrlInterceptor();
-2 indent

[mnaganov (inactive), 2014-07-29 08:40:49]

LGTM % nits

https://codereview.chromium.org/411353002/diff/20001/media/base/android/media...
File media/base/android/media_player_bridge.cc (right):

https://codereview.chromium.org/411353002/diff/20001/media/base/android/media...
media/base/android/media_player_bridge.cc:215: const int unset_value = -1;
kUnsetValue

https://codereview.chromium.org/411353002/diff/20001/media/base/android/media...
File media/base/android/media_url_interceptor.h (right):

https://codereview.chromium.org/411353002/diff/20001/media/base/android/media...
media/base/android/media_url_interceptor.h:23: // Output arguments (only when
url intercepted):
nit: when url has been intercepted

[Ignacio Solla, 2014-07-29 09:03:52]

https://codereview.chromium.org/411353002/diff/20001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/411353002/diff/20001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:53:
media::MediaUrlInterceptor* media_url_interceptor) {
On 2014/07/28 21:16:01, qinmin wrote:
> fix the indent

Done.

https://codereview.chromium.org/411353002/diff/20001/media/base/android/media...
File media/base/android/media_player_bridge.cc (right):

https://codereview.chromium.org/411353002/diff/20001/media/base/android/media...
media/base/android/media_player_bridge.cc:215: const int unset_value = -1;
On 2014/07/29 08:40:49, Mikhail Naganov (Cr) wrote:
> kUnsetValue

Done.

https://codereview.chromium.org/411353002/diff/20001/media/base/android/media...
media/base/android/media_player_bridge.cc:221:
manager()->GetMediaUrlInterceptor();
On 2014/07/28 21:16:01, qinmin wrote:
> -2 indent

Done.

https://codereview.chromium.org/411353002/diff/20001/media/base/android/media...
File media/base/android/media_url_interceptor.h (right):

https://codereview.chromium.org/411353002/diff/20001/media/base/android/media...
media/base/android/media_url_interceptor.h:23: // Output arguments (only when
url intercepted):
On 2014/07/29 08:40:49, Mikhail Naganov (Cr) wrote:
> nit: when url has been intercepted

Done.

[Ignacio Solla, 2014-07-29 09:06:45]

xhwang@chromium.org: I'm missing an LGTM for the gyp file, so please review the
changes to media.gyp. Thanks!

[benm (inactive), 2014-07-29 12:22:07]

lgtm

[xhwang, 2014-07-29 17:08:20]

media.gyp lgtm

https://codereview.chromium.org/411353002/diff/40001/media/base/android/media...
File media/base/android/media_url_interceptor.h (right):

https://codereview.chromium.org/411353002/diff/40001/media/base/android/media...
media/base/android/media_url_interceptor.h:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
nit: (c) not needed

[Ignacio Solla, 2014-07-30 08:45:13]

https://codereview.chromium.org/411353002/diff/40001/media/base/android/media...
File media/base/android/media_url_interceptor.h (right):

https://codereview.chromium.org/411353002/diff/40001/media/base/android/media...
media/base/android/media_url_interceptor.h:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
On 2014/07/29 17:08:20, xhwang wrote:
> nit: (c) not needed

Done.

[Ignacio Solla, 2014-07-30 08:45:23]

The CQ bit was checked by igsolla@chromium.org

[commit-bot: I haz the power, 2014-07-30 08:47:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/igsolla@chromium.org/411353002/60001

[commit-bot: I haz the power, 2014-07-30 12:02:16]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)

[commit-bot: I haz the power, 2014-07-30 14:02:04]

Change committed as 286506