Fix the issue that any tab/window closing hotkey will crash the Pepper Flash Fullscreen.

Fix the issue that any tab/window closing hotkey will crash the Pepper Flash Fullscreen.

BUG=149821
TEST=None


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

[yzshen1, 2012-09-21 18:16:50]

Hi, John.

Would you please take a look at this CL?

Two notes:
1) After a second thought, I didn't let WebContents shut down render widgets
created by it. Because the comment in render_widget_host.h says that select
popups (flapper fullscreen is the same) are responsible for destroying
themselves. The WebContents's only responsibility to select popups is to create
them when it is told to.

I don't want to change the ownership rule, especially when this CL is about to
go to the Stable channel very soon.
So I just made RenderWidgetHostImpl correctly handle the case that it outlives
the delegate.

2) I made RenderWidgetHostDelegate::RenderWidget{Created,Deleted} deal with
RenderWidgetHostImpl instead of RenderWidgetHost. Because
RenderWidgetHostDelegate lives in content/browser/render_host instead of
content/public/browser. RenderWidgetHost should not be aware of the delegate.

Please let me know if this look okay to you. Thanks!

[jam, 2012-09-21 18:46:17]

RenderWidgetHostDelegate is just an interface now, so it seems preferable to not
add implementation to it. Why not keep the list of created widgets in
WebContentsImpl as we discussed (it's the only RenderWidgetHostDelegate).

[yzshen1, 2012-09-21 19:14:21]

Thanks, John.

The reason is that InterstitialPageImpl is also a RenderWidgetHostDelegate
subclass.

Is it okay to keep it this way? Or maybe I can create a
RenderWidgetHostDelegateBase to host the implementation?

On 2012/09/21 18:46:17, John Abd-El-Malek wrote:
> RenderWidgetHostDelegate is just an interface now, so it seems preferable to
not
> add implementation to it. Why not keep the list of created widgets in
> WebContentsImpl as we discussed (it's the only RenderWidgetHostDelegate).

[jam, 2012-09-21 20:32:16]

On 2012/09/21 19:14:21, yzshen1 wrote:
> Thanks, John.
> 
> The reason is that InterstitialPageImpl is also a RenderWidgetHostDelegate
> subclass.

ah, I forgot about InterstitialPageImpl.

However, isn't this only a problem with WebContentsImpl? It's the only one that
creates RenderWidgetHosts and sets itself up as a delegate and then goes away
before them. So it seems that the fix should just be there.

> 
> Is it okay to keep it this way? Or maybe I can create a
> RenderWidgetHostDelegateBase to host the implementation?
> 
> On 2012/09/21 18:46:17, John Abd-El-Malek wrote:
> > RenderWidgetHostDelegate is just an interface now, so it seems preferable to
> not
> > add implementation to it. Why not keep the list of created widgets in
> > WebContentsImpl as we discussed (it's the only RenderWidgetHostDelegate).

[yzshen1, 2012-09-21 22:11:59]

Hi, John.

Thanks for your suggestion!
I have made changes accordingly. Please take another look.

On 2012/09/21 20:32:16, John Abd-El-Malek wrote:
> On 2012/09/21 19:14:21, yzshen1 wrote:
> > Thanks, John.
> > 
> > The reason is that InterstitialPageImpl is also a RenderWidgetHostDelegate
> > subclass.
> 
> ah, I forgot about InterstitialPageImpl.
> 
> However, isn't this only a problem with WebContentsImpl? It's the only one
that
> creates RenderWidgetHosts and sets itself up as a delegate and then goes away
> before them. So it seems that the fix should just be there.
> 
> > 
> > Is it okay to keep it this way? Or maybe I can create a
> > RenderWidgetHostDelegateBase to host the implementation?
> > 
> > On 2012/09/21 18:46:17, John Abd-El-Malek wrote:
> > > RenderWidgetHostDelegate is just an interface now, so it seems preferable
to
> > not
> > > add implementation to it. Why not keep the list of created widgets in
> > > WebContentsImpl as we discussed (it's the only RenderWidgetHostDelegate).

[jam, 2012-09-22 00:01:34]

https://codereview.chromium.org/10968037/diff/10005/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/10968037/diff/10005/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.h:130: // |widget_delegate|
goes away.
since this derives from RenderWidgetHostImpl, is this comment needed?

https://codereview.chromium.org/10968037/diff/10005/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/10968037/diff/10005/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.h:417: void
DetachDelegate() { delegate_ = NULL; }
per the style guide, if this is inline this should be unix_hacker.

since this is just a setter, I would just make it

set_delegate(RenderWidgetHostDelegate* d) { delegate_ = d; }

https://codereview.chromium.org/10968037/diff/10005/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/10968037/diff/10005/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.h:838: // All alive
RenderWidgetHostImpls that are created by this object and may
nit: "All live"

[yzshen1, 2012-09-22 00:28:08]

Thanks, John.
Please take another look.

https://codereview.chromium.org/10968037/diff/10005/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/10968037/diff/10005/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.h:130: // |widget_delegate|
goes away.
I imagine that people wouldn't be careful enough to read the comments of the
base class, and figure out this parameter would be forwarded to the base class.

Do you think it is okay to keep it?

On 2012/09/22 00:01:34, John Abd-El-Malek wrote:
> since this derives from RenderWidgetHostImpl, is this comment needed?

https://codereview.chromium.org/10968037/diff/10005/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/10968037/diff/10005/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.h:417: void
DetachDelegate() { delegate_ = NULL; }
set_delegate() implies that |delegate_| can be changed to a different value
other than NULL. This is incorrect: the previous |delegate_| won't receive
RenderWidgetDeleted() notification.

I moved the definition to the .cc file (but put it at the end of the file since
the order of the definitions are really out of sync with the declarations.)


On 2012/09/22 00:01:34, John Abd-El-Malek wrote:
> per the style guide, if this is inline this should be unix_hacker.
> 
> since this is just a setter, I would just make it
> 
> set_delegate(RenderWidgetHostDelegate* d) { delegate_ = d; }

https://codereview.chromium.org/10968037/diff/10005/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/10968037/diff/10005/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.h:838: // All alive
RenderWidgetHostImpls that are created by this object and may
Done. Thanks. :)
On 2012/09/22 00:01:34, John Abd-El-Malek wrote:
> nit: "All live"

[jam, 2012-09-24 16:47:00]

https://codereview.chromium.org/10968037/diff/10005/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/10968037/diff/10005/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.h:130: // |widget_delegate|
goes away.
On 2012/09/22 00:28:08, yzshen1 wrote:
> I imagine that people wouldn't be careful enough to read the comments of the
> base class, and figure out this parameter would be forwarded to the base
class.
> 
> Do you think it is okay to keep it?
> 
> On 2012/09/22 00:01:34, John Abd-El-Malek wrote:
> > since this derives from RenderWidgetHostImpl, is this comment needed?
> 

what I was trying to say is that this class doesn't have to do anything because
its parents class already does, so the comment is needless.

[yzshen1, 2012-09-24 16:59:41]

https://codereview.chromium.org/10968037/diff/10005/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/10968037/diff/10005/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.h:130: // |widget_delegate|
goes away.
Removed. Thanks!
On 2012/09/24 16:47:00, John Abd-El-Malek wrote:
> On 2012/09/22 00:28:08, yzshen1 wrote:
> > I imagine that people wouldn't be careful enough to read the comments of the
> > base class, and figure out this parameter would be forwarded to the base
> class.
> > 
> > Do you think it is okay to keep it?
> > 
> > On 2012/09/22 00:01:34, John Abd-El-Malek wrote:
> > > since this derives from RenderWidgetHostImpl, is this comment needed?
> > 
> 
> what I was trying to say is that this class doesn't have to do anything
because
> its parents class already does, so the comment is needless.

[jam, 2012-09-24 19:20:51]

lgtm

https://codereview.chromium.org/10968037/diff/6008/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://codereview.chromium.org/10968037/diff/6008/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_delegate.h:22: // The
RenderWidgetHostImpl is going to be deleted.
nit: "RenderWidgetHost"

[yzshen1, 2012-09-24 19:56:18]

Thanks, John!

http://codereview.chromium.org/10968037/diff/6008/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

http://codereview.chromium.org/10968037/diff/6008/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_delegate.h:22: // The
RenderWidgetHostImpl is going to be deleted.
On 2012/09/24 19:20:51, John Abd-El-Malek wrote:
> nit: "RenderWidgetHost"

Done.

[commit-bot: I haz the power, 2012-09-24 19:57:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/10968037/6010

[commit-bot: I haz the power, 2012-09-24 21:57:27]

Change committed as 158408