Mac: Fix crash when destroying RWHVMacs that use accelerated drawing.

Mac: Fix crash when destroying RWHVMacs that use accelerated drawing.

Also fix a memory leak in that area.

BUG=56721
, None
TEST=None

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

[stuartmorgan, 2010-10-05 21:55:24]

http://codereview.chromium.org/3534010/diff/2001/3002
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/3534010/diff/2001/3002#newcode176
chrome/browser/renderer_host/render_widget_host_view_mac.mm:176: // RWHVMac is
about to be deleted (but it's still valid while this method runs).
Instead of adding a new method, would it make more sense to just have RWHVMac
remove these views in Destroy, and use viewDidMoveToSuperview nil?

http://codereview.chromium.org/3534010/diff/2001/3002#newcode903
chrome/browser/renderer_host/render_widget_host_view_mac.mm:903: // remove the
handle from |plugin_container_manager_|.
This comment is pretty confusing in light of the other comment about how the
view will outlive this object. I understand why it works, but as written they
seem contradictory, so one or both comments should be clarified.

[Nico, 2010-10-05 22:06:32]

Thanks!

http://codereview.chromium.org/3534010/diff/2001/3002
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/3534010/diff/2001/3002#newcode176
chrome/browser/renderer_host/render_widget_host_view_mac.mm:176: // RWHVMac is
about to be deleted (but it's still valid while this method runs).
On 2010/10/05 21:55:24, stuartmorgan wrote:
> Instead of adding a new method, would it make more sense to just have RWHVMac
> remove these views in Destroy, and use viewDidMoveToSuperview nil?

That will work too I suppose, but then I need to store all acclerated plugin
views in a temp array before I remove them because I'm iterating over
|subviews|.

This was it's a bit more explicit. I slightly prefer it how it is, but if you
want, I can change it.

http://codereview.chromium.org/3534010/diff/2001/3002#newcode903
chrome/browser/renderer_host/render_widget_host_view_mac.mm:903: // remove the
handle from |plugin_container_manager_|.
On 2010/10/05 21:55:24, stuartmorgan wrote:
> This comment is pretty confusing in light of the other comment about how the
> view will outlive this object. I understand why it works, but as written they
> seem contradictory, so one or both comments should be clarified.

Expanded this comment a bit.

[stuartmorgan, 2010-10-05 22:18:42]

> This was it's a bit more explicit.

It's more explicit in the one case, but it creates two different teardown
paths--depending on the case, the relative object lifetimes are different--which
seems worse. There's enough documentation that it should be fine either way
though.

LGTM with comment nit.

http://codereview.chromium.org/3534010/diff/6001/7002
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/3534010/diff/6001/7002#newcode175
chrome/browser/renderer_host/render_widget_host_view_mac.mm:175: // will be
around a little longer than the RWHVMac. This is called when the
If you stick with your current approach, this should say "can be", rather than
"will be".