Fix crashes related to the extension uninstall dialog.

Fix crashes related to the extension uninstall dialog.

The uninstall prompt is owned by the settings page (the dialog's delegate), which gets destroyed when the user navigates to a new page. We need to make sure we invalidate pointers to the settings page when this occurs.

This adds an ExtensionUninstallDialog with platform specific implementations.

GTK: The settings page owns the ExtensionUninstallDialogGtk, which closes the GTK prompt when being destroyed.

VIEWS: The views framework is a bit more convoluted because views owns the prompt's views::View. We have two classes: ExtensionUninstallDialogDelegateView owned by views, and ExtensionUninstallDialogViews owned by the settings page. If the user accepts or denies the prompt, we proxy the events through ExtensionUninstallDialogViews so we can invalidate pointers back to the views widget. If the settings page is destroyed, the ExtensionUninstallDialogViews closes the views widget after invalidating pointers back to the settings page.

COCOA: You can't navigate away from the page when the prompt is open, so the dialog can't outlive its delegate.

BUG=75011
TEST=see bug.

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

[jstritar, 2011-09-16 22:27:52]

Can you take a look? The views part is a little strange, but it is working now.

[jstritar, 2011-09-19 14:50:01]

On 2011/09/16 22:27:52, jstritar wrote:
> Can you take a look? The views part is a little strange, but it is working
now.

Actually, maybe you can hold off on this. I think I can simplify the views code
by using the WIDGET_OWNS_NATIVE_WIDGET ownership flag.

[jstritar, 2011-09-20 14:33:30]

You can go ahead and take a look at this. I'm not sure that ownership flag does
what I was looking for.

[Mihai Parparita -not on Chrome, 2011-09-20 23:54:47]

Can you write in the CL description a brief overview of how this fixes the
lifecycle problems? I'm not 100% clear I understand the need for having both
ExtensionUninstallDialog and ExtensionUninstallUi. Would it have been enough to
only have ExtensionUninstallDialog, make the destructor implementation be
platform specific, and in the GTK and views ones close the dialog if it's still
open?

Also, the TEST= line in the description should probably say "see bug."

http://codereview.chromium.org/7920023/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_uninstall_ui.h (right):

http://codereview.chromium.org/7920023/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_uninstall_ui.h:45: void ShowDialog();
Can you add a comment "The implementations of this function are
platform-specific." since it wasn't immediately obvious why the implementation
wasn't in extension_install_ui.cc?

http://codereview.chromium.org/7920023/diff/1/chrome/browser/ui/cocoa/extensi...
File chrome/browser/ui/cocoa/extensions/extension_uninstall_dialog_cocoa.mm
(right):

http://codereview.chromium.org/7920023/diff/1/chrome/browser/ui/cocoa/extensi...
chrome/browser/ui/cocoa/extensions/extension_uninstall_dialog_cocoa.mm:46: //
dialog prevents the page from changing so the widget is only closed by
Can you make this comment a bit more explicit (add something like "so there's no
way for the dialog to outlive the page/its delegate")?

[jstritar, 2011-09-22 16:47:13]

Thanks for the feedback. This is ready for another review.

I didn't end up needing the ExtensionUninstallUI. I had based this off of the
install dialog, but the uninstall prompt doesn't actually have logic deciding
what to display. The lesson there is that if you can't describe what the class
does then you don't need it :)

http://codereview.chromium.org/7920023/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_uninstall_ui.h (right):

http://codereview.chromium.org/7920023/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_uninstall_ui.h:45: void ShowDialog();
On 2011/09/20 23:54:47, Mihai Parparita wrote:
> Can you add a comment "The implementations of this function are
> platform-specific." since it wasn't immediately obvious why the implementation
> wasn't in extension_install_ui.cc?

Done.

http://codereview.chromium.org/7920023/diff/1/chrome/browser/ui/cocoa/extensi...
File chrome/browser/ui/cocoa/extensions/extension_uninstall_dialog_cocoa.mm
(right):

http://codereview.chromium.org/7920023/diff/1/chrome/browser/ui/cocoa/extensi...
chrome/browser/ui/cocoa/extensions/extension_uninstall_dialog_cocoa.mm:46: //
dialog prevents the page from changing so the widget is only closed by
On 2011/09/20 23:54:47, Mihai Parparita wrote:
> Can you make this comment a bit more explicit (add something like "so there's
no
> way for the dialog to outlive the page/its delegate")?

Done.

[Mihai Parparita -not on Chrome, 2011-09-22 16:56:33]

http://codereview.chromium.org/7920023/diff/9001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_uninstall_dialog.h (right):

http://codereview.chromium.org/7920023/diff/9001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_uninstall_dialog.h:70: virtual void Show();
Shouldn't this be a pure virtual function? I don't think you want any port
getting the default no-op behavior.

[jstritar, 2011-09-22 17:12:02]

http://codereview.chromium.org/7920023/diff/9001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_uninstall_dialog.h (right):

http://codereview.chromium.org/7920023/diff/9001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_uninstall_dialog.h:70: virtual void Show();
On 2011/09/22 16:56:33, Mihai Parparita wrote:
> Shouldn't this be a pure virtual function? I don't think you want any port
> getting the default no-op behavior.

Yeah we don't need the no-op behavior. Would I do this by just removing the
no-op definition from extension_uninstall_dialog.cc? When I do that it fails
linking for some reason.

[jstritar, 2011-09-22 17:56:52]

http://codereview.chromium.org/7920023/diff/9001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_uninstall_dialog.h (right):

http://codereview.chromium.org/7920023/diff/9001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_uninstall_dialog.h:70: virtual void Show();
On 2011/09/22 16:56:33, Mihai Parparita wrote:
> Shouldn't this be a pure virtual function? I don't think you want any port
> getting the default no-op behavior.

Done.

[Mihai Parparita -not on Chrome, 2011-09-22 18:08:27]

lgtm

[commit-bot: I haz the power, 2011-09-22 20:24:26]

CQ is trying tha patch. Follow status at
https://chromium-status.appspot.com/cq/jstritar@chromium.org/7920023/10002

[commit-bot: I haz the power, 2011-09-22 20:27:18]

Presubmit check for 7920023-10002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc, line 82:
new code should not use wstrings.  If you are calling an API that accepts a
wstring, fix the API.

chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc, line 162:
new code should not use wstrings.  If you are calling an API that accepts a
wstring, fix the API.

chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc, line 188:
new code should not use wstrings.  If you are calling an API that accepts a
wstring, fix the API.

Presubmit checks took 2.4s to calculate.