Improve extension uninstall flow

Improve extension uninstall flow

When uninstall is triggered by a user on chrome://extensions
default the confirmation dialog box to "OK" instead of "Cancel".

BUG=365793
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=265441

[Yoyo Zhou, 2014-04-18 23:07:13]

https://codereview.chromium.org/243643003/diff/1/chrome/browser/ui/cocoa/exte...
File chrome/browser/ui/cocoa/extensions/extension_uninstall_dialog_cocoa.mm
(right):

https://codereview.chromium.org/243643003/diff/1/chrome/browser/ui/cocoa/exte...
chrome/browser/ui/cocoa/extensions/extension_uninstall_dialog_cocoa.mm:55: //
Default to cancel when uninstall is triggered by API.
This should also say "Default to accept when triggered by user gesture"

https://codereview.chromium.org/243643003/diff/1/chrome/browser/ui/cocoa/exte...
chrome/browser/ui/cocoa/extensions/extension_uninstall_dialog_cocoa.mm:57:
[cancelButton setKeyEquivalent:@"\r"];
This indentation level looks like a mistake.

https://codereview.chromium.org/243643003/diff/1/chrome/browser/ui/views/exte...
File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc
(right):

https://codereview.chromium.org/243643003/diff/1/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:87: //
Default to cancel when uninstall is triggered by API.
ditto

[Isaac (away), 2014-04-18 23:42:44]

https://codereview.chromium.org/243643003/diff/1/chrome/browser/ui/cocoa/exte...
File chrome/browser/ui/cocoa/extensions/extension_uninstall_dialog_cocoa.mm
(right):

https://codereview.chromium.org/243643003/diff/1/chrome/browser/ui/cocoa/exte...
chrome/browser/ui/cocoa/extensions/extension_uninstall_dialog_cocoa.mm:57:
[cancelButton setKeyEquivalent:@"\r"];
On 2014/04/18 23:07:13, Yoyo Zhou wrote:
> This indentation level looks like a mistake.

I believe both of these lines are needed to switch button default.

[meacer, 2014-04-18 23:45:23]

Lgtm

https://codereview.chromium.org/243643003/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc
(right):

https://codereview.chromium.org/243643003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:87: //
Default to accept when triggered by user gesture.
This dialog is always triggered by a user gesture so this comment isn't correct
(chrome.management.uninstall API also requires a user gesture). Something like
"Default to accept when triggered from the chrome://extensions page rather than
chrome.management.uninstall API" makes more sense.

[Yoyo Zhou, 2014-04-18 23:48:05]

https://codereview.chromium.org/243643003/diff/1/chrome/browser/ui/cocoa/exte...
File chrome/browser/ui/cocoa/extensions/extension_uninstall_dialog_cocoa.mm
(right):

https://codereview.chromium.org/243643003/diff/1/chrome/browser/ui/cocoa/exte...
chrome/browser/ui/cocoa/extensions/extension_uninstall_dialog_cocoa.mm:57:
[cancelButton setKeyEquivalent:@"\r"];
On 2014/04/18 23:42:44, Isaac wrote:
> On 2014/04/18 23:07:13, Yoyo Zhou wrote:
> > This indentation level looks like a mistake.
> 
> I believe both of these lines are needed to switch button default.

How about some braces?

[Isaac (away), 2014-04-18 23:54:51]

https://codereview.chromium.org/243643003/diff/1/chrome/browser/ui/cocoa/exte...
File chrome/browser/ui/cocoa/extensions/extension_uninstall_dialog_cocoa.mm
(right):

https://codereview.chromium.org/243643003/diff/1/chrome/browser/ui/cocoa/exte...
chrome/browser/ui/cocoa/extensions/extension_uninstall_dialog_cocoa.mm:57:
[cancelButton setKeyEquivalent:@"\r"];
On 2014/04/18 23:48:05, Yoyo Zhou wrote:
> How about some braces?

Done.  Was trying to copy style from line 63-66.

https://codereview.chromium.org/243643003/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc
(right):

https://codereview.chromium.org/243643003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:87: //
Default to accept when triggered by user gesture.
On 2014/04/18 23:45:23, Mustafa Emre Acer wrote:
> This dialog is always triggered by a user gesture so this comment isn't
correct
> (chrome.management.uninstall API also requires a user gesture). Something like
> "Default to accept when triggered from the chrome://extensions page rather
than
> chrome.management.uninstall API" makes more sense.

Done.

[Yoyo Zhou, 2014-04-19 00:02:59]

LGTM

[Isaac (away), 2014-04-19 00:10:31]

+mmentovai for owners.

[Isaac (away), 2014-04-19 00:45:18]

+msw for views/ owner.

[msw - DO NOT USE, 2014-04-19 00:52:57]

lgtm with a nit.

https://codereview.chromium.org/243643003/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc
(right):

https://codereview.chromium.org/243643003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:88: return
triggering_extension_ ?
nit: seems like you could just keep a bool |triggered_via_extension_|, or an int
|default_dialog_button_| (that's changed on
ExtensionUninstallDialogDelegateView), instead of keeping a pointer to the
extension object.

[msw, 2014-04-19 00:53:54]

Sorry, I somehow used my @google address...
lgtm with the aforementioned nit.

[Isaac (away), 2014-04-19 01:06:19]

https://codereview.chromium.org/243643003/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc
(right):

https://codereview.chromium.org/243643003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:88: return
triggering_extension_ ?
On 2014/04/19 00:52:58, msw - DO NOT USE wrote:
> nit: seems like you could just keep a bool |triggered_via_extension_|, or an
int
> |default_dialog_button_| (that's changed on
> ExtensionUninstallDialogDelegateView), instead of keeping a pointer to the
> extension object.

Switched to bool.

[msw, 2014-04-19 01:29:36]

https://codereview.chromium.org/243643003/diff/110001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc
(right):

https://codereview.chromium.org/243643003/diff/110001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:109: bool
triggered_by_extension_;
Explicitly initialize this in the ctor initialization list.

[Isaac (away), 2014-04-19 02:42:22]

https://codereview.chromium.org/243643003/diff/110001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc
(right):

https://codereview.chromium.org/243643003/diff/110001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:109: bool
triggered_by_extension_;
On 2014/04/19 01:29:37, msw wrote:
> Explicitly initialize this in the ctor initialization list.

Thanks for catching that.  Done.

[msw, 2014-04-19 03:03:56]

lgtm

[Mark Mentovai, 2014-04-19 16:10:15]

So the same dialog has a different default action depending on how the user
reached it? That seems weird.

How did you arrive at this? What are the possible paths to reach the dialog?

[Isaac (away), 2014-04-19 19:37:03]

On 2014/04/19 16:10:15, Mark Mentovai wrote:
> So the same dialog has a different default action depending on how the user
> reached it? That seems weird.
> 
> How did you arrive at this?

My original plan was to make 'Proceed' the default all the time.  Mustafa
requested this behavior.

> What are the possible paths to reach the dialog?

Mustafa mentioned an extension API, which I assume is
https://developer.chrome.com/extensions/management#method-uninstall

Mustafa thought the api could trigger this popup without context; hence the
divergence in default.  However it appears that extensions with management
permission can choose to forgo the confirmation dialog entirely, so maybe it's
OK for confirmation to always preselect 'Proceed'.

In my opinion, uninstalling extensions is something we should make easy, as it
is safe and generally improves user experience.

[meacer, 2014-04-21 16:52:35]

> However it appears that extensions with management
> permission can choose to forgo the confirmation dialog entirely, so maybe it's
> OK for confirmation to always preselect 'Proceed'.

This is actually changed now, the confirmation dialog always shows disregarding
the showConfirmDialog flag. The documentation needs to be updated.

[Finnur, 2014-04-22 11:12:45]

LGTM, but you should link this against a bug, if only to facilitate QA's
workflow.

[Isaac (away), 2014-04-22 19:11:14]

The CQ bit was checked by ilevy@chromium.org

[commit-bot: I haz the power, 2014-04-22 19:11:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ilevy@chromium.org/243643003/20002

[Mark Mentovai, 2014-04-22 19:27:19]

The code LGTM, but I’m concerned about the same dialog having a different
default depending on how it’s reached. Please solicit UI review on the
newly-filed bug. Let’s make sure there’s a paper trail for this.

[commit-bot: I haz the power, 2014-04-23 00:38:15]

Change committed as 265441