Skip video overlay surface management when in VR.

Skip video overlay surface management when in VR.

This changes the overlay surface from being disabled when VrShell flag was
enabled at all to only when VrShell is actually active. It also re-enables
VrShell testing config.

BUG=654851
Review-Url: https://codereview.chromium.org/2643893002
Cr-Commit-Position: refs/heads/master@{#444634}
Committed: https://chromium.googlesource.com/chromium/src/+/ea73f799eb7fe7a9b5836f2c491bd99dbf933089

[amp, 2017-01-18 18:17:03]

amp@chromium.org changed reviewers:
+ bshe@chromium.org, creis@chromium.org, liberato@chromium.org,
mthiesse@chromium.org, rkaplow@chromium.org

[amp, 2017-01-18 18:17:04]

I'm changing the way that overlay surfaces are disabled for
http://crbug.com/654851. As the current solution prevents turning on the VrShell
flag for any non-test/development scenario's.

This change adds a getter to the 'is_in_vr_' state of rwhva which was landed in
the change that disabled android keyboard in vr
https://codereview.chromium.org/2632493005/

A getter on rwhv allows a simple check from the browser surface manager to
disable overlays when in vr (rather than relying on a flag which disabled
overlays any time the flag was on even if the user never used vr).

+creis please review content (content changes should be very similar to changes
referenced above).  They are also similarly temporary until a more complete
solution can be implemented (see http://crbug.com/673886 and
https://codereview.chromium.org/2567233002/ for what the long term solution
might look like, but there is still design work ongoing there).

+liberato please review wmpi and browser surface manager.  I moved the overlay
experiment check back into wmpi now that there is no need to check the VrShell
feature flag.

+rkaplow please review testing config.  We still don't have an active experiment
for VrShell, but we should be able to turn the test config back on in
preparation for one.

+mtheisse and +bshe mostly fyi for general vr.

[liberato (no reviews please), 2017-01-18 18:21:49]

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

https://codereview.chromium.org/2643893002/diff/1/content/browser/media/andro...
content/browser/media/android/browser_surface_view_manager.cc:69: if
(render_frame_host_->GetView()->IsInVR()) {
one migh tneed SendSurfaceID(kNoSurfaceID), else WMPI will never find out that
the request failed.

[amp, 2017-01-18 19:30:25]

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

https://codereview.chromium.org/2643893002/diff/1/content/browser/media/andro...
content/browser/media/android/browser_surface_view_manager.cc:69: if
(render_frame_host_->GetView()->IsInVR()) {
On 2017/01/18 18:21:48, liberato wrote:
> one migh tneed SendSurfaceID(kNoSurfaceID), else WMPI will never find out that
> the request failed.
Done.

Nice catch, although it seems to work fine without it...

Line 95 below also seems to be case where it can return without sending back an
id.

[liberato (no reviews please), 2017-01-18 19:35:08]

On 2017/01/18 19:30:25, amp wrote:
>
https://codereview.chromium.org/2643893002/diff/1/content/browser/media/andro...
> File content/browser/media/android/browser_surface_view_manager.cc (right):
> 
>
https://codereview.chromium.org/2643893002/diff/1/content/browser/media/andro...
> content/browser/media/android/browser_surface_view_manager.cc:69: if
> (render_frame_host_->GetView()->IsInVR()) {
> On 2017/01/18 18:21:48, liberato wrote:
> > one migh tneed SendSurfaceID(kNoSurfaceID), else WMPI will never find out
that
> > the request failed.
> Done.
> 
> Nice catch, although it seems to work fine without it...
> 
> Line 95 below also seems to be case where it can return without sending back
an
> id.

media/, content/browser/media/android lgtm.

[watk, 2017-01-18 20:57:56]

On 2017/01/18 19:35:08, liberato wrote:
> On 2017/01/18 19:30:25, amp wrote:
> >
>
https://codereview.chromium.org/2643893002/diff/1/content/browser/media/andro...
> > File content/browser/media/android/browser_surface_view_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/2643893002/diff/1/content/browser/media/andro...
> > content/browser/media/android/browser_surface_view_manager.cc:69: if
> > (render_frame_host_->GetView()->IsInVR()) {
> > On 2017/01/18 18:21:48, liberato wrote:
> > > one migh tneed SendSurfaceID(kNoSurfaceID), else WMPI will never find out
> that
> > > the request failed.
> > Done.
> > 
> > Nice catch, although it seems to work fine without it...
> > 
> > Line 95 below also seems to be case where it can return without sending back
> an
> > id.

Line 95 is almost certainly a bug :( None of us were on that review by the look
of it.

[amp, 2017-01-18 21:02:39]

On 2017/01/18 20:57:56, watk wrote:
> On 2017/01/18 19:35:08, liberato wrote:
> > On 2017/01/18 19:30:25, amp wrote:
> > >
> >
>
https://codereview.chromium.org/2643893002/diff/1/content/browser/media/andro...
> > > File content/browser/media/android/browser_surface_view_manager.cc
(right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2643893002/diff/1/content/browser/media/andro...
> > > content/browser/media/android/browser_surface_view_manager.cc:69: if
> > > (render_frame_host_->GetView()->IsInVR()) {
> > > On 2017/01/18 18:21:48, liberato wrote:
> > > > one migh tneed SendSurfaceID(kNoSurfaceID), else WMPI will never find
out
> > that
> > > > the request failed.
> > > Done.
> > > 
> > > Nice catch, although it seems to work fine without it...
> > > 
> > > Line 95 below also seems to be case where it can return without sending
back
> > an
> > > id.
> 
> Line 95 is almost certainly a bug :( None of us were on that review by the
look
> of it.

I'll go ahead and add sending back NoSurfaceId on line 95 as well.

[watk, 2017-01-18 21:07:52]

On 2017/01/18 21:02:39, amp wrote:
> On 2017/01/18 20:57:56, watk wrote:
> > On 2017/01/18 19:35:08, liberato wrote:
> > > On 2017/01/18 19:30:25, amp wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2643893002/diff/1/content/browser/media/andro...
> > > > File content/browser/media/android/browser_surface_view_manager.cc
> (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2643893002/diff/1/content/browser/media/andro...
> > > > content/browser/media/android/browser_surface_view_manager.cc:69: if
> > > > (render_frame_host_->GetView()->IsInVR()) {
> > > > On 2017/01/18 18:21:48, liberato wrote:
> > > > > one migh tneed SendSurfaceID(kNoSurfaceID), else WMPI will never find
> out
> > > that
> > > > > the request failed.
> > > > Done.
> > > > 
> > > > Nice catch, although it seems to work fine without it...
> > > > 
> > > > Line 95 below also seems to be case where it can return without sending
> back
> > > an
> > > > id.
> > 
> > Line 95 is almost certainly a bug :( None of us were on that review by the
> look
> > of it.
> 
> I'll go ahead and add sending back NoSurfaceId on line 95 as well.

Oh thanks, I was just about to fix it :)

[rkaplow, 2017-01-18 21:33:46]

lgtm

[Charlie Reis, 2017-01-18 22:34:41]

Mostly looks good, except for one requirement of the content API.

https://codereview.chromium.org/2643893002/diff/40001/content/public/browser/...
File content/public/browser/render_widget_host_view.h (right):

https://codereview.chromium.org/2643893002/diff/40001/content/public/browser/...
content/public/browser/render_widget_host_view.h:71: virtual bool IsInVR() const
= 0;
We can't add this to the public API unless there are callers outside content:

https://www.chromium.org/developers/content-module/content-api
"only expose methods in the public API that embedders need. If a method is only
used by other code in content, it belongs in foo_impl.h and not foo.h."

It looks like it's only being used from
BrowserSurfaceViewManager::OnCreateFullscreenSurface inside content.  Can we
declare it on RenderWidgetHostViewBase instead of RenderWidgetHostView and then
cast at the call site?

(I would have recommended changing RenderFrameHostImpl::GetView() to return a
RenderWidgetHostViewBase, since it's a covariant return type for
RenderFrameHostImpl::GetView().  It looks like
BrowserSurfaceViewManager::render_frame_host_ isn't a RenderFrameHostImpl,
though.)

[amp, 2017-01-18 22:39:24]

On 2017/01/18 22:34:41, Charlie Reis wrote:
> Mostly looks good, except for one requirement of the content API.
> 
>
https://codereview.chromium.org/2643893002/diff/40001/content/public/browser/...
> File content/public/browser/render_widget_host_view.h (right):
> 
>
https://codereview.chromium.org/2643893002/diff/40001/content/public/browser/...
> content/public/browser/render_widget_host_view.h:71: virtual bool IsInVR()
const
> = 0;
> We can't add this to the public API unless there are callers outside content:
> 
> https://www.chromium.org/developers/content-module/content-api
> "only expose methods in the public API that embedders need. If a method is
only
> used by other code in content, it belongs in foo_impl.h and not foo.h."
> 
> It looks like it's only being used from
> BrowserSurfaceViewManager::OnCreateFullscreenSurface inside content.  Can we
> declare it on RenderWidgetHostViewBase instead of RenderWidgetHostView and
then
> cast at the call site?
> 
> (I would have recommended changing RenderFrameHostImpl::GetView() to return a
> RenderWidgetHostViewBase, since it's a covariant return type for
> RenderFrameHostImpl::GetView().  It looks like
> BrowserSurfaceViewManager::render_frame_host_ isn't a RenderFrameHostImpl,
> though.)

Yea, it's only being used from BrowserSurfaceViewManager.  I'll try to switch it
over to be on the base and cast instead of the public API (thanks for the
references, I just copied what the setter was doing without checking what was
allowed).

[amp, 2017-01-18 23:03:36]

On 2017/01/18 22:39:24, amp wrote:
> On 2017/01/18 22:34:41, Charlie Reis wrote:
> > Mostly looks good, except for one requirement of the content API.
> > 
> >
>
https://codereview.chromium.org/2643893002/diff/40001/content/public/browser/...
> > File content/public/browser/render_widget_host_view.h (right):
> > 
> >
>
https://codereview.chromium.org/2643893002/diff/40001/content/public/browser/...
> > content/public/browser/render_widget_host_view.h:71: virtual bool IsInVR()
> const
> > = 0;
> > We can't add this to the public API unless there are callers outside
content:
> > 
> > https://www.chromium.org/developers/content-module/content-api
> > "only expose methods in the public API that embedders need. If a method is
> only
> > used by other code in content, it belongs in foo_impl.h and not foo.h."
> > 
> > It looks like it's only being used from
> > BrowserSurfaceViewManager::OnCreateFullscreenSurface inside content.  Can we
> > declare it on RenderWidgetHostViewBase instead of RenderWidgetHostView and
> then
> > cast at the call site?
> > 
> > (I would have recommended changing RenderFrameHostImpl::GetView() to return
a
> > RenderWidgetHostViewBase, since it's a covariant return type for
> > RenderFrameHostImpl::GetView().  It looks like
> > BrowserSurfaceViewManager::render_frame_host_ isn't a RenderFrameHostImpl,
> > though.)
> 
> Yea, it's only being used from BrowserSurfaceViewManager.  I'll try to switch
it
> over to be on the base and cast instead of the public API (thanks for the
> references, I just copied what the setter was doing without checking what was
> allowed).

Ok that works.  PTAL at latest patch.

[Charlie Reis, 2017-01-18 23:08:43]

Thanks!  content/ LGTM.

[amp, 2017-01-19 02:26:07]

The CQ bit was checked by amp@chromium.org

[amp, 2017-01-19 02:26:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
rkaplow@chromium.org, liberato@chromium.org
Link to the patchset: https://codereview.chromium.org/2643893002/#ps60001
(title: "Removed public API changes and changed references to cast to base
instead.")

[commit-bot: I haz the power, 2017-01-19 02:26:36]

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, 2017-01-19 04:05:43]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484792767476830,
"parent_rev": "4626de7927e6a1a37ab024e39bd6f336032b9fbd", "commit_rev":
"ea73f799eb7fe7a9b5836f2c491bd99dbf933089"}

[commit-bot: I haz the power, 2017-01-19 04:06:44]

Description was changed from

==========
Skip video overlay surface management when in VR.

This changes the overlay surface from being disabled when VrShell flag was
enabled at all to only when VrShell is actually active. It also re-enables
VrShell testing config.

BUG=654851
==========

to

==========
Skip video overlay surface management when in VR.

This changes the overlay surface from being disabled when VrShell flag was
enabled at all to only when VrShell is actually active. It also re-enables
VrShell testing config.

BUG=654851

Review-Url: https://codereview.chromium.org/2643893002
Cr-Commit-Position: refs/heads/master@{#444634}
Committed:
https://chromium.googlesource.com/chromium/src/+/ea73f799eb7fe7a9b5836f2c491b...
==========

[commit-bot: I haz the power, 2017-01-19 04:06:45]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/ea73f799eb7fe7a9b5836f2c491b...