Location/size change notification when external rendering is enabled

Location/size change notification when external rendering is enabled

Extract location/size of the video at renderer process and
pass it to browser process where the external surface
resides, so that app can readjust the location of external
surface when it is changed.

BUG=180197
R=scherkus@chromium.org,yfriedman@chromium.org,sky@chromium.org

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

[wonsik, 2013-04-05 09:05:16]

yfriedman: content/browser/android/* ; content/public/android/*
scherkus: content/renderer/media/* ; webkit/media/android/*
sky: content/browser/web_contents/*
palmer: content/common/media/media_player_messages.h (IPC security)

Thanks!

[sky, 2013-04-05 16:09:01]

LGTM

[palmer, 2013-04-05 17:56:00]

Please write a more informative commit log message.

[palmer, 2013-04-05 18:05:15]

https://codereview.chromium.org/13688004/diff/3001/content/browser/android/me...
File content/browser/android/media_player_manager_android.h (right):

https://codereview.chromium.org/13688004/diff/3001/content/browser/android/me...
content/browser/android/media_player_manager_android.h:86: void
OnNotifyGeometryChange(int player_id, gfx::RectF rect);
Is it possible to pass a reference instead?

https://codereview.chromium.org/13688004/diff/3001/webkit/media/android/webme...
File webkit/media/android/webmediaplayer_manager_android.cc (right):

https://codereview.chromium.org/13688004/diff/3001/webkit/media/android/webme...
webkit/media/android/webmediaplayer_manager_android.cc:76: std::map<int32,
WebMediaPlayerAndroid*>::iterator player_it;
This should really be scoped to the for loop. If the problem is that it makes
the line too long, maybe use a typedef?

Note also that you are using an int32 to index a map whose key type is int. That
is not guaranteed to work; use the same type in both places.

[wonsik, 2013-04-08 04:39:22]

PTAL

https://codereview.chromium.org/13688004/diff/3001/content/browser/android/me...
File content/browser/android/media_player_manager_android.h (right):

https://codereview.chromium.org/13688004/diff/3001/content/browser/android/me...
content/browser/android/media_player_manager_android.h:86: void
OnNotifyGeometryChange(int player_id, gfx::RectF rect);
On 2013/04/05 18:05:15, Chris P. wrote:
> Is it possible to pass a reference instead?

Done.

https://codereview.chromium.org/13688004/diff/3001/webkit/media/android/webme...
File webkit/media/android/webmediaplayer_manager_android.cc (right):

https://codereview.chromium.org/13688004/diff/3001/webkit/media/android/webme...
webkit/media/android/webmediaplayer_manager_android.cc:76: std::map<int32,
WebMediaPlayerAndroid*>::iterator player_it;
On 2013/04/05 18:05:15, Chris P. wrote:
> This should really be scoped to the for loop. If the problem is that it makes
> the line too long, maybe use a typedef?
> 
> Note also that you are using an int32 to index a map whose key type is int.
That
> is not guaranteed to work; use the same type in both places.

Done.

[scherkus (not reviewing), 2013-04-08 14:58:23]

https://codereview.chromium.org/13688004/diff/9001/webkit/media/android/webme...
File webkit/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/13688004/diff/9001/webkit/media/android/webme...
webkit/media/android/webmediaplayer_android.cc:494: if (rect_ == *rect)
based on your usage, |rect| appears to always be (0,0,0,0)

does this condition ever evaluate to true?

https://codereview.chromium.org/13688004/diff/9001/webkit/media/android/webme...
File webkit/media/android/webmediaplayer_android.h (right):

https://codereview.chromium.org/13688004/diff/9001/webkit/media/android/webme...
webkit/media/android/webmediaplayer_android.h:136: bool
GetGeometryChange(gfx::RectF* rect);
needs docs

https://codereview.chromium.org/13688004/diff/9001/webkit/media/android/webme...
webkit/media/android/webmediaplayer_android.h:264: gfx::RectF rect_;
perhaps there's a more descriptive name? comment?

[scherkus (not reviewing), 2013-04-08 14:59:05]

also, how much of the new code should be wrapped with GOOGLE_TV?

for example... the code that sends the IPC is guarded, but not the IPC message
definition itself

[wonsik, 2013-04-09 09:13:05]

On 2013/04/08 14:59:05, scherkus wrote:
> also, how much of the new code should be wrapped with GOOGLE_TV?
> 
> for example... the code that sends the IPC is guarded, but not the IPC message
> definition itself

None of the code is TV dependent --- should I unguard them?

My first thought was that since the feature will be necessary only for TV for a
while, I tried to minimize impact to other platforms. And IPC sending part
seemed to have most impact, so I guarded it.

[wonsik, 2013-04-09 09:13:21]

PTAL

https://codereview.chromium.org/13688004/diff/9001/webkit/media/android/webme...
File webkit/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/13688004/diff/9001/webkit/media/android/webme...
webkit/media/android/webmediaplayer_android.cc:494: if (rect_ == *rect)
On 2013/04/08 14:58:23, scherkus wrote:
> based on your usage, |rect| appears to always be (0,0,0,0)
> 
> does this condition ever evaluate to true?

|rect| represents the geometry of the video frame (offset and size). |rect_|
will contain the geometry information when it was last computed. It can differ
from (0,0,0,0) in most of cases. Perhaps I confused you with lack of comments..

https://codereview.chromium.org/13688004/diff/9001/webkit/media/android/webme...
File webkit/media/android/webmediaplayer_android.h (right):

https://codereview.chromium.org/13688004/diff/9001/webkit/media/android/webme...
webkit/media/android/webmediaplayer_android.h:136: bool
GetGeometryChange(gfx::RectF* rect);
On 2013/04/08 14:58:23, scherkus wrote:
> needs docs

Done.

https://codereview.chromium.org/13688004/diff/9001/webkit/media/android/webme...
webkit/media/android/webmediaplayer_android.h:264: gfx::RectF rect_;
On 2013/04/08 14:58:23, scherkus wrote:
> perhaps there's a more descriptive name? comment?

Done.

[palmer, 2013-04-09 19:02:41]

https://codereview.chromium.org/13688004/diff/19001/content/browser/android/m...
File content/browser/android/media_player_manager_android.cc (right):

https://codereview.chromium.org/13688004/diff/19001/content/browser/android/m...
content/browser/android/media_player_manager_android.cc:228: if (web_contents_)
{
NIT: You could save a level of indentation by:

if (!web_contents_)
  return;

https://codereview.chromium.org/13688004/diff/19001/content/renderer/media/we...
File content/renderer/media/webmediaplayer_proxy_impl_android.cc (right):

https://codereview.chromium.org/13688004/diff/19001/content/renderer/media/we...
content/renderer/media/webmediaplayer_proxy_impl_android.cc:188: #if
defined(GOOGLE_TV)
So, the only client that sends this IPC message is the renderers on Google TV
systems, but the browser-side end-point is always built in? Regardless of safety
and attack surface, is that a bloat problem?

I see scherkus asked about this too. I think you should either guard all the
code with defined(GOOGLE_TV), or guard none of it (iff it really will be used
everywhere eventually).

https://codereview.chromium.org/13688004/diff/19001/webkit/media/android/webm...
File webkit/media/android/webmediaplayer_manager_android.cc (right):

https://codereview.chromium.org/13688004/diff/19001/webkit/media/android/webm...
webkit/media/android/webmediaplayer_manager_android.cc:74: std::map<int,
gfx::RectF>* changes) {
I always wish we could use a vector<Foo> rather than a map<int, Foo>. A change
like that is out of scope for this CL, but it could be an efficiency
improvement...

[scherkus (not reviewing), 2013-04-09 23:43:39]

https://codereview.chromium.org/13688004/diff/19001/content/renderer/media/we...
File content/renderer/media/webmediaplayer_proxy_impl_android.cc (right):

https://codereview.chromium.org/13688004/diff/19001/content/renderer/media/we...
content/renderer/media/webmediaplayer_proxy_impl_android.cc:188: #if
defined(GOOGLE_TV)
On 2013/04/09 19:02:41, Chris P. wrote:
> So, the only client that sends this IPC message is the renderers on Google TV
> systems, but the browser-side end-point is always built in? Regardless of
safety
> and attack surface, is that a bloat problem?
> 
> I see scherkus asked about this too. I think you should either guard all the
> code with defined(GOOGLE_TV), or guard none of it (iff it really will be used
> everywhere eventually).

I believe the long term path is to move away hole punching, in which case we
should try to wrap as much hole-punching related code w/ defined(GOOGLE_TV).

Even if we're wrong it's easy to drop the defines. Not so much the other way
around (i.e., determining which functions are unused hole punching code)

[wonsik, 2013-04-10 06:29:22]

PTAL

https://codereview.chromium.org/13688004/diff/19001/content/browser/android/m...
File content/browser/android/media_player_manager_android.cc (right):

https://codereview.chromium.org/13688004/diff/19001/content/browser/android/m...
content/browser/android/media_player_manager_android.cc:228: if (web_contents_)
{
On 2013/04/09 19:02:41, Chris P. wrote:
> NIT: You could save a level of indentation by:
> 
> if (!web_contents_)
>   return;

Done.

https://codereview.chromium.org/13688004/diff/19001/content/renderer/media/we...
File content/renderer/media/webmediaplayer_proxy_impl_android.cc (right):

https://codereview.chromium.org/13688004/diff/19001/content/renderer/media/we...
content/renderer/media/webmediaplayer_proxy_impl_android.cc:188: #if
defined(GOOGLE_TV)
On 2013/04/09 23:43:39, scherkus wrote:
> On 2013/04/09 19:02:41, Chris P. wrote:
> > So, the only client that sends this IPC message is the renderers on Google
TV
> > systems, but the browser-side end-point is always built in? Regardless of
> safety
> > and attack surface, is that a bloat problem?
> > 
> > I see scherkus asked about this too. I think you should either guard all the
> > code with defined(GOOGLE_TV), or guard none of it (iff it really will be
used
> > everywhere eventually).
> 
> I believe the long term path is to move away hole punching, in which case we
> should try to wrap as much hole-punching related code w/ defined(GOOGLE_TV).
> 
> Even if we're wrong it's easy to drop the defines. Not so much the other way
> around (i.e., determining which functions are unused hole punching code)

Done --- IPC and related code ifdef'ed except for Java code and JNI hooks where
there is no obvious way to guard them.

https://codereview.chromium.org/13688004/diff/19001/webkit/media/android/webm...
File webkit/media/android/webmediaplayer_manager_android.cc (right):

https://codereview.chromium.org/13688004/diff/19001/webkit/media/android/webm...
webkit/media/android/webmediaplayer_manager_android.cc:74: std::map<int,
gfx::RectF>* changes) {
On 2013/04/09 19:02:41, Chris P. wrote:
> I always wish we could use a vector<Foo> rather than a map<int, Foo>. A change
> like that is out of scope for this CL, but it could be an efficiency
> improvement...

Filed http://crbug.com/229769

[wonsik, 2013-04-12 04:31:06]

gentle reminder :)

[ycheo, 2013-04-12 07:54:32]

https://codereview.chromium.org/13688004/diff/41001/webkit/media/android/webm...
File webkit/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/13688004/diff/41001/webkit/media/android/webm...
webkit/media/android/webmediaplayer_android.cc:61: #endif
'#else'?

[wonsik, 2013-04-12 09:58:54]

PTAL

https://codereview.chromium.org/13688004/diff/41001/webkit/media/android/webm...
File webkit/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/13688004/diff/41001/webkit/media/android/webm...
webkit/media/android/webmediaplayer_android.cc:61: #endif
On 2013/04/12 07:54:32, ycheo wrote:
> '#else'?

Done.

[palmer, 2013-04-12 18:51:21]

IPC security LGTM, but you'll probably want to rename |GetGeometryChange|.

https://codereview.chromium.org/13688004/diff/53001/content/browser/android/c...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/13688004/diff/53001/content/browser/android/c...
content/browser/android/content_view_core_impl.cc:605: void
ContentViewCoreImpl::NotifyGeometryChange(int player_id,
Should this be guarded with OS_ANDROID, too?

https://codereview.chromium.org/13688004/diff/53001/webkit/media/android/webm...
File webkit/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/13688004/diff/53001/webkit/media/android/webm...
webkit/media/android/webmediaplayer_android.cc:528:
rect->set_size(layer->bounds());
Maybe this function should be called |SetGeometryChange|, since it updates its
argument rather than retrieving a field value.

[wonsik, 2013-04-15 02:44:15]

Thanks --- PTAL.

https://codereview.chromium.org/13688004/diff/53001/content/browser/android/c...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/13688004/diff/53001/content/browser/android/c...
content/browser/android/content_view_core_impl.cc:605: void
ContentViewCoreImpl::NotifyGeometryChange(int player_id,
On 2013/04/12 18:51:21, Chris P. wrote:
> Should this be guarded with OS_ANDROID, too?

This whole file is only included in Android build, so OS_ANDROID wouldn't be too
useful.

If you meant GOOGLE_TV, the part calling this method is generated by JNI script
at build time, so it is not obvious how to restrict that for GOOGLE_TV only.

https://codereview.chromium.org/13688004/diff/53001/webkit/media/android/webm...
File webkit/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/13688004/diff/53001/webkit/media/android/webm...
webkit/media/android/webmediaplayer_android.cc:528:
rect->set_size(layer->bounds());
On 2013/04/12 18:51:21, Chris P. wrote:
> Maybe this function should be called |SetGeometryChange|, since it updates its
> argument rather than retrieving a field value.

Good point. I will use |RetrieveGeometryChange| to clearly distinguish it from
getter/setter patterns.

[wonsik, 2013-04-17 02:21:35]

ping? :)

[Yaron, 2013-04-17 17:30:00]

https://codereview.chromium.org/13688004/diff/60001/content/browser/android/c...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/13688004/diff/60001/content/browser/android/c...
content/browser/android/content_view_core_impl.cc:607: JNIEnv* env =
AttachCurrentThread();
Can we guard this too for GOOGLE_TV?

[scherkus (not reviewing), 2013-04-17 18:12:41]

https://codereview.chromium.org/13688004/diff/60001/content/browser/android/c...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/13688004/diff/60001/content/browser/android/c...
content/browser/android/content_view_core_impl.cc:1243: #if defined(GOOGLE_TV)
if we #ifdef out the declarations in the .h these should be moved outside of the
method

https://codereview.chromium.org/13688004/diff/60001/content/browser/android/c...
File content/browser/android/content_view_core_impl.h (right):

https://codereview.chromium.org/13688004/diff/60001/content/browser/android/c...
content/browser/android/content_view_core_impl.h:219: void
AttachExternalVideoSurface(JNIEnv* env,
should these be #ifdef'd ?

[wonsik, 2013-04-18 05:13:21]

PTAL

https://codereview.chromium.org/13688004/diff/60001/content/browser/android/c...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/13688004/diff/60001/content/browser/android/c...
content/browser/android/content_view_core_impl.cc:607: JNIEnv* env =
AttachCurrentThread();
On 2013/04/17 17:30:00, Yaron wrote:
> Can we guard this too for GOOGLE_TV?

Guarding it causes build failure for non-GoogleTV build because the Java_*
definition lies in script-generated jni headers.

https://codereview.chromium.org/13688004/diff/60001/content/browser/android/c...
content/browser/android/content_view_core_impl.cc:1243: #if defined(GOOGLE_TV)
On 2013/04/17 18:12:41, scherkus wrote:
> if we #ifdef out the declarations in the .h these should be moved outside of
the
> method

Please see my comment in the .h file.

https://codereview.chromium.org/13688004/diff/60001/content/browser/android/c...
File content/browser/android/content_view_core_impl.h (right):

https://codereview.chromium.org/13688004/diff/60001/content/browser/android/c...
content/browser/android/content_view_core_impl.h:219: void
AttachExternalVideoSurface(JNIEnv* env,
On 2013/04/17 18:12:41, scherkus wrote:
> should these be #ifdef'd ?

Guarding it causes build failure for non-GoogleTV build because the calls to
these functions lies in script-generated jni headers.

[scherkus (not reviewing), 2013-04-18 17:55:20]

lgtm ... but see my question re Java_* methods

https://codereview.chromium.org/13688004/diff/60001/content/browser/android/c...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/13688004/diff/60001/content/browser/android/c...
content/browser/android/content_view_core_impl.cc:607: JNIEnv* env =
AttachCurrentThread();
On 2013/04/18 05:13:21, wonsik wrote:
> On 2013/04/17 17:30:00, Yaron wrote:
> > Can we guard this too for GOOGLE_TV?
> 
> Guarding it causes build failure for non-GoogleTV build because the Java_*
> definition lies in script-generated jni headers.

Hmm... and there's no way to only generate the Java_* methods when GOOGLE_TV is
defined? (i.e., using some GYP variables?)

[Yaron, 2013-04-18 18:13:52]

lgtm

https://codereview.chromium.org/13688004/diff/60001/content/browser/android/c...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/13688004/diff/60001/content/browser/android/c...
content/browser/android/content_view_core_impl.cc:607: JNIEnv* env =
AttachCurrentThread();
On 2013/04/18 17:55:21, scherkus wrote:
> On 2013/04/18 05:13:21, wonsik wrote:
> > On 2013/04/17 17:30:00, Yaron wrote:
> > > Can we guard this too for GOOGLE_TV?
> > 
> > Guarding it causes build failure for non-GoogleTV build because the Java_*
> > definition lies in script-generated jni headers.
> 
> Hmm... and there's no way to only generate the Java_* methods when GOOGLE_TV
is
> defined? (i.e., using some GYP variables?)

Ahh, right. Otherwise Java_ContentViewCore_notifyGeometryChange is unused and we
don't currently have support for conditionally generating bindings.

[scherkus (not reviewing), 2013-04-18 18:14:23]

On 2013/04/18 18:13:52, Yaron wrote:
> lgtm
> 
>
https://codereview.chromium.org/13688004/diff/60001/content/browser/android/c...
> File content/browser/android/content_view_core_impl.cc (right):
> 
>
https://codereview.chromium.org/13688004/diff/60001/content/browser/android/c...
> content/browser/android/content_view_core_impl.cc:607: JNIEnv* env =
> AttachCurrentThread();
> On 2013/04/18 17:55:21, scherkus wrote:
> > On 2013/04/18 05:13:21, wonsik wrote:
> > > On 2013/04/17 17:30:00, Yaron wrote:
> > > > Can we guard this too for GOOGLE_TV?
> > > 
> > > Guarding it causes build failure for non-GoogleTV build because the Java_*
> > > definition lies in script-generated jni headers.
> > 
> > Hmm... and there's no way to only generate the Java_* methods when GOOGLE_TV
> is
> > defined? (i.e., using some GYP variables?)
> 
> Ahh, right. Otherwise Java_ContentViewCore_notifyGeometryChange is unused and
we
> don't currently have support for conditionally generating bindings.

OOC is that something we're interested in having?

[Yaron, 2013-04-18 18:22:57]

On Thu, Apr 18, 2013 at 11:14 AM, <scherkus@chromium.org> wrote:

> On 2013/04/18 18:13:52, Yaron wrote:
>
>> lgtm
>>
>
>
> https://codereview.chromium.**org/13688004/diff/60001/**
>
content/browser/android/**content_view_core_impl.cc<https://codereview.chromium.org/13688004/diff/60001/content/browser/android/content_view_core_impl.cc>
>
>> File content/browser/android/**content_view_core_impl.cc (right):
>>
>
>
> https://codereview.chromium.**org/13688004/diff/60001/**
>
content/browser/android/**content_view_core_impl.cc#**newcode607<https://codereview.chromium.org/13688004/diff/60001/content/browser/android/content_view_core_impl.cc#newcode607>
>
>> content/browser/android/**content_view_core_impl.cc:607: JNIEnv* env =
>> AttachCurrentThread();
>> On 2013/04/18 17:55:21, scherkus wrote:
>> > On 2013/04/18 05:13:21, wonsik wrote:
>> > > On 2013/04/17 17:30:00, Yaron wrote:
>> > > > Can we guard this too for GOOGLE_TV?
>> > >
>> > > Guarding it causes build failure for non-GoogleTV build because the
>> Java_*
>> > > definition lies in script-generated jni headers.
>> >
>> > Hmm... and there's no way to only generate the Java_* methods when
>> GOOGLE_TV
>> is
>> > defined? (i.e., using some GYP variables?)
>>
>
>  Ahh, right. Otherwise Java_ContentViewCore_**notifyGeometryChange is
>> unused and
>>
> we
>
>> don't currently have support for conditionally generating bindings.
>>
>
> OOC is that something we're interested in having?
>

AFAIK, this is the first time it's come up. I don't think it's a priority.

>
>
https://codereview.chromium.**org/13688004/<https://codereview.chromium.org/1...
>

[commit-bot: I haz the power, 2013-04-19 01:29:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wonsik@chromium.org/13688004/68001

[commit-bot: I haz the power, 2013-04-19 01:38:15]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2013-04-19 01:47:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wonsik@chromium.org/13688004/68001

[commit-bot: I haz the power, 2013-04-19 11:01:55]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is android_dbg, revision is HEAD

[commit-bot: I haz the power, 2013-04-19 11:02:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wonsik@chromium.org/13688004/68001

[commit-bot: I haz the power, 2013-04-19 12:57:56]

Change committed as 195148