aura: Close fullscreen RenderWidgetHostViewAura on blur.

aura: Close fullscreen RenderWidgetHostViewAura on blur.

This makes fullscreen RWHVAs close themselves when they lose
the focus; unlike NPAPI Flash, Pepper Flash doesn't close
automatically on blur.

BUG=123083
TEST=manual: alt-tab or escape exits fullscreen flash


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137077

[piman, 2012-05-09 20:19:21]

https://chromiumcodereview.appspot.com/10377066/diff/1/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://chromiumcodereview.appspot.com/10377066/diff/1/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:942: in_shutdown_
= true;
drive-by: should we test !in_shutdown_ before this? Or are we guaranteed that
once we lose the focus we'll never get it back (e.g. rapid alt-tab), and that
we'll never add another case that would also cause a shutdown?

[Daniel Erat, 2012-05-09 20:25:21]

https://chromiumcodereview.appspot.com/10377066/diff/1/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://chromiumcodereview.appspot.com/10377066/diff/1/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:942: in_shutdown_
= true;
On 2012/05/09 20:19:22, piman wrote:
> drive-by: should we test !in_shutdown_ before this? Or are we guaranteed that
> once we lose the focus we'll never get it back (e.g. rapid alt-tab), and that
> we'll never add another case that would also cause a shutdown?

I don't think we return to the event loop before we get killed, so I don't think
that that particular issue is a concern here.  But I was thinking about this
too, and it's safe to do and makes the code safer, so added. :-P

[sky, 2012-05-09 21:18:31]

LGTM

[piman, 2012-05-09 21:34:41]

lgtm

[Daniel Erat, 2012-05-09 23:05:21]

cc-ing Avi in case he has ideas about the testing code.

(RenderWidgetHostViewAuraTest is disgusting; I deserve the blame for that. :-( )

I'm trying to add the DestroyFullscreenOnLostFocus test.  The test succeeds, but
there's a segfault in the call to RenderViewHostImplTestHarness::TearDown()
afterwards.  I think that the problem is that rwhv_aura_ calls
RenderWidgetHostImpl::Shutdown() in the test when the RWHV loses the focus,
which destroys the RWH, and then RenderViewHostManager's d'tor calls it again
during teardown.

Abusing RenderViewHostImplTestHarness by swapping in a different RWHV seems like
a horrible idea.  I'd be happy to switch away to some other method of testing
this code if you have any suggestions.

[Daniel Erat, 2012-05-10 20:13:19]

Jochen, any ideas about this test-related question?

(RenderWidgetHostViewAuraTest is disgusting; I deserve the blame for that. :-( )

I'm trying to add the DestroyFullscreenOnLostFocus test.  The test succeeds, but
there's a segfault in the call to RenderViewHostImplTestHarness::TearDown()
afterwards.  I think that the problem is that rwhv_aura_ calls
RenderWidgetHostImpl::Shutdown() in the test when the RWHV loses the focus,
which destroys the RWH, and then RenderViewHostManager's d'tor calls it again
during teardown.

Abusing RenderViewHostImplTestHarness by swapping in a different RWHV seems like
a horrible idea.  I'd be happy to switch away to some other method of testing
this code if you have any suggestions.

[jochen (gone - plz use gerrit), 2012-05-10 22:38:41]

+creis for RVHM

[Charlie Reis, 2012-05-11 20:50:31]

On 2012/05/10 22:38:41, jochen wrote:
> +creis for RVHM

From chatting, sounds like you figured it out yesterday.  If I understand
correctly, fullscreen should never call Shutdown on a RVH that is owned by a
WebContents + RenderViewHostManager.

Is there a way to CHECK that is_fullscreen_ and being owned by a WebContents are
incompatible?

Charlie

[Daniel Erat, 2012-05-11 22:56:47]

I've reworked the tests a bit.  Jochen and Charlie, mind telling me what I'm
doing wrong? :-)

https://chromiumcodereview.appspot.com/10377066/diff/11001/content/browser/re...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://chromiumcodereview.appspot.com/10377066/diff/11001/content/browser/re...
content/browser/renderer_host/render_widget_host_view_aura.cc:404: in_shutdown_
= true;
The destruction process for RWHVA seems very fragile to me.  There are multiple
paths to trigger it (e.g. Escape key or blur make us send call the host's
Shutdown method, but we also get notified about blur if we close our window for
some other reason while we have the focus).

https://chromiumcodereview.appspot.com/10377066/diff/11001/content/browser/re...
File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10377066/diff/11001/content/browser/re...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:71: rph_
= new RenderProcessHostImpl(browser_context_.get());
I'm having a difficult time wrapping my head around the lifetimes for these
objects.  What I currently have seems to work for the FocusFullscreen test, but
Valgrind says that the RPH is being leaked in DestroyFullscreenOnBlur.  I've
tried many different variations, but I either end up with leaks or segfaults
from double-freeing.

[Daniel Erat, 2012-05-11 23:10:05]

On Fri, May 11, 2012 at 1:50 PM,  <creis@chromium.org> wrote:
> On 2012/05/10 22:38:41, jochen wrote:
>>
>> +creis for RVHM
>
>
> From chatting, sounds like you figured it out yesterday.  If I understand
> correctly, fullscreen should never call Shutdown on a RVH that is owned by a
> WebContents + RenderViewHostManager.
>
> Is there a way to CHECK that is_fullscreen_ and being owned by a WebContents
> are
> incompatible?

I'm not sure; is this straightforward to see from the RWHV side?  It's
non-obvious to me, but perhaps I'm missing something.

> Charlie
>
> https://chromiumcodereview.appspot.com/10377066/

[jochen (gone - plz use gerrit), 2012-05-12 00:09:54]

https://chromiumcodereview.appspot.com/10377066/diff/11001/content/browser/re...
File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10377066/diff/11001/content/browser/re...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:71: rph_
= new RenderProcessHostImpl(browser_context_.get());
On 2012/05/11 22:56:48, Daniel Erat wrote:
> I'm having a difficult time wrapping my head around the lifetimes for these
> objects.  What I currently have seems to work for the FocusFullscreen test,
but
> Valgrind says that the RPH is being leaked in DestroyFullscreenOnBlur.  I've
> tried many different variations, but I either end up with leaks or segfaults
> from double-freeing.

Your code only shuts down the render view host, but not the process host. So my
guess is that before you delete the browser context and after destrying the
rwvh, you should delete the process host. Also, I'd recommend using a
MockRenderProcessHost

[Charlie Reis, 2012-05-12 00:55:54]

On 2012/05/11 23:10:05, Daniel Erat wrote:
> > Is there a way to CHECK that is_fullscreen_ and being owned by a WebContents
> > are
> > incompatible?
> 
> I'm not sure; is this straightforward to see from the RWHV side?  It's
> non-obvious to me, but perhaps I'm missing something.
> 

This may not be the best way, but at whatever point that we're setting
is_fullscreen_ to true, you could DCHECK based on the type of view.  You can
tell if it's in a WebContents by checking if RWH::IsRenderView() is true and
then comparing RVH's GetDelegate()->GetRenderViewType() to
VIEW_TYPE_WEB_CONTENTS.

There may be something easier to DCHECK, though, since I'm not familiar with how
fullscreen widgets work.

[Daniel Erat, 2012-05-14 22:32:50]

Thanks, what I have now seems to work.  Another look?

[Daniel Erat, 2012-05-14 22:43:53]

On 2012/05/12 00:55:54, creis wrote:
> On 2012/05/11 23:10:05, Daniel Erat wrote:
> > > Is there a way to CHECK that is_fullscreen_ and being owned by a
WebContents
> > > are
> > > incompatible?
> > 
> > I'm not sure; is this straightforward to see from the RWHV side?  It's
> > non-obvious to me, but perhaps I'm missing something.
> > 
> 
> This may not be the best way, but at whatever point that we're setting
> is_fullscreen_ to true, you could DCHECK based on the type of view.  You can
> tell if it's in a WebContents by checking if RWH::IsRenderView() is true and
> then comparing RVH's GetDelegate()->GetRenderViewType() to
> VIEW_TYPE_WEB_CONTENTS.
> 
> There may be something easier to DCHECK, though, since I'm not familiar with
how
> fullscreen widgets work.

Has this changed?  All I see in view_type.h is:

enum ViewTypeValues {
  VIEW_TYPE_INVALID,
  VIEW_TYPE_INTERSTITIAL_PAGE,
  VIEW_TYPE_DEV_TOOLS_UI,
  VIEW_TYPE_CONTENT_END
};

typedef int ViewType;

[Charlie Reis, 2012-05-14 22:59:36]

Aaron just removed it over the weekend, unfortunately (
https://chromiumcodereview.appspot.com/10387110).  I don't have a good
answer for you anymore.

What about the other way around?  Could a WebContents verify that any RVH
it owns has is_fullscreen_ set to false?  Again, I'm not familiar with
fullscreen, so I'm not sure if that's something that gets set at creation
and doesn't change, or something that might change at runtime.

Charlie


On Mon, May 14, 2012 at 3:43 PM, <derat@chromium.org> wrote:

> On 2012/05/12 00:55:54, creis wrote:
>
>> On 2012/05/11 23:10:05, Daniel Erat wrote:
>> > > Is there a way to CHECK that is_fullscreen_ and being owned by a
>>
> WebContents
>
>> > > are
>> > > incompatible?
>> >
>> > I'm not sure; is this straightforward to see from the RWHV side?  It's
>> > non-obvious to me, but perhaps I'm missing something.
>> >
>>
>
>  This may not be the best way, but at whatever point that we're setting
>> is_fullscreen_ to true, you could DCHECK based on the type of view.  You
>> can
>> tell if it's in a WebContents by checking if RWH::IsRenderView() is true
>> and
>> then comparing RVH's GetDelegate()->**GetRenderViewType() to
>> VIEW_TYPE_WEB_CONTENTS.
>>
>
>  There may be something easier to DCHECK, though, since I'm not familiar
>> with
>>
> how
>
>> fullscreen widgets work.
>>
>
> Has this changed?  All I see in view_type.h is:
>
> enum ViewTypeValues {
>  VIEW_TYPE_INVALID,
>  VIEW_TYPE_INTERSTITIAL_PAGE,
>  VIEW_TYPE_DEV_TOOLS_UI,
>  VIEW_TYPE_CONTENT_END
> };
>
> typedef int ViewType;
>
>
http://codereview.chromium.**org/10377066/<http://codereview.chromium.org/103...
>

[Daniel Erat, 2012-05-15 00:35:25]

On 2012/05/14 22:59:36, creis wrote:
> Aaron just removed it over the weekend, unfortunately (
> https://chromiumcodereview.appspot.com/10387110).  I don't have a good
> answer for you anymore.
> 
> What about the other way around?  Could a WebContents verify that any RVH
> it owns has is_fullscreen_ set to false?  Again, I'm not familiar with
> fullscreen, so I'm not sure if that's something that gets set at creation
> and doesn't change, or something that might change at runtime.

I'll give it a shot.  Mind if I file a separate bug to track it?

Does the testing code here look reasonable to you?

> Charlie
> 
> 
> On Mon, May 14, 2012 at 3:43 PM, <mailto:derat@chromium.org> wrote:
> 
> > On 2012/05/12 00:55:54, creis wrote:
> >
> >> On 2012/05/11 23:10:05, Daniel Erat wrote:
> >> > > Is there a way to CHECK that is_fullscreen_ and being owned by a
> >>
> > WebContents
> >
> >> > > are
> >> > > incompatible?
> >> >
> >> > I'm not sure; is this straightforward to see from the RWHV side?  It's
> >> > non-obvious to me, but perhaps I'm missing something.
> >> >
> >>
> >
> >  This may not be the best way, but at whatever point that we're setting
> >> is_fullscreen_ to true, you could DCHECK based on the type of view.  You
> >> can
> >> tell if it's in a WebContents by checking if RWH::IsRenderView() is true
> >> and
> >> then comparing RVH's GetDelegate()->**GetRenderViewType() to
> >> VIEW_TYPE_WEB_CONTENTS.
> >>
> >
> >  There may be something easier to DCHECK, though, since I'm not familiar
> >> with
> >>
> > how
> >
> >> fullscreen widgets work.
> >>
> >
> > Has this changed?  All I see in view_type.h is:
> >
> > enum ViewTypeValues {
> >  VIEW_TYPE_INVALID,
> >  VIEW_TYPE_INTERSTITIAL_PAGE,
> >  VIEW_TYPE_DEV_TOOLS_UI,
> >  VIEW_TYPE_CONTENT_END
> > };
> >
> > typedef int ViewType;
> >
> >
>
http://codereview.chromium.**org/10377066/%3Chttp://codereview.chromium.org/1...>
> >

[Charlie Reis, 2012-05-15 01:06:59]

Yes, a separate bug for the check sounds fine.

Rubber stamp LGTM on the existing patch.  Most of the code changed here is
outside my expertise, so I'll defer to the other reviewers on it.

[commit-bot: I haz the power, 2012-05-15 02:14:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/derat@chromium.org/10377066/6

[commit-bot: I haz the power, 2012-05-15 05:13:05]

Change committed as 137077