[Extensions] Make chrome://extensions use management.uninstall

[Extensions] Make chrome://extensions use management.uninstall

Make the chrome://extensions page use chrome.management API
to uninstall extensions. Also add unit tests for the API
function, and convert it to a UIThreadExtensionFunction.

BUG=461039
Committed: https://crrev.com/529ad1a90fdcd3b97aaa64d3c26081ddd9871f21
Cr-Commit-Position: refs/heads/master@{#318317}

[Devlin, 2015-02-25 20:33:32]

Patchset #1 (id:1) has been deleted

[Devlin, 2015-02-25 20:33:42]

rdevlin.cronin@chromium.org changed reviewers:
+ kalman@chromium.org

[not at google - send to devlin, 2015-02-25 21:37:58]

This is presumably more than you asked for, but don't blame the messenger!

https://codereview.chromium.org/948413005/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/management/chrome_management_api_delegate.cc
(right):

https://codereview.chromium.org/948413005/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/management/chrome_management_api_delegate.cc:94:
// If this is a self uninstall, or triggered by webui (in which case
This decision belongs in the API implementation, not the Delegate. See comment
there.

https://codereview.chromium.org/948413005/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/management/management_api_unittest.cc
(right):

https://codereview.chromium.org/948413005/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api_unittest.cc:141:
TEST_F(ManagementApiUnitTest, ManagementUninstall) {
Looks like this test can be split up into several smaller tests.
- uninstall w/o user gesture
- uninstall w/ user gesture
- uninstall but cancelled
- uninstall self

https://codereview.chromium.org/948413005/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api_unittest.cc:143:
ResetThreadBundle(content::TestBrowserThreadBundle::DEFAULT);
See comment in ExtensionServiceTestBase.

https://codereview.chromium.org/948413005/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api_unittest.cc:177: // The
install should have failed.
uninstall?

https://codereview.chromium.org/948413005/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_service_test_base.cc (right):

https://codereview.chromium.org/948413005/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_service_test_base.cc:37: const int
kThreadOptions = content::TestBrowserThreadBundle::IO_MAINLOOP;
UI thread makes more sense for a default test thread. Very few functions live on
the IO thread.

https://codereview.chromium.org/948413005/diff/20001/chrome/browser/resources...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/948413005/diff/20001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_list.js:301: this.uninstallShown_
= true;
uninstallIsShowing would be better, Shown could mean it was shown in the past.

https://codereview.chromium.org/948413005/diff/20001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_list.js:302: // TODO(devlin): What
should we do if the uninstall fails?
TODO belongs in the callback body.

https://codereview.chromium.org/948413005/diff/20001/extensions/browser/api/m...
File extensions/browser/api/management/management_api.cc (right):

https://codereview.chromium.org/948413005/diff/20001/extensions/browser/api/m...
extensions/browser/api/management/management_api.cc:520: extension_id_ =
target_extension_id;
ugh, can we call |extension_id_| actually |target_extension_id_|?

https://codereview.chromium.org/948413005/diff/20001/extensions/browser/api/m...
extensions/browser/api/management/management_api.cc:536: if (show_confirm_dialog
&& auto_confirm_for_test == DO_NOT_SKIP) {
There are just too many checks for |auto_confirm_for_test| for me to really have
much confidence in those unit tests being useful. Bah. I have some suggestions
below.

https://codereview.chromium.org/948413005/diff/20001/extensions/browser/api/m...
extensions/browser/api/management/management_api.cc:538: // TODO(devlin): A
method called "UninstallFunctionDelegate" does not in
Indeed!

https://codereview.chromium.org/948413005/diff/20001/extensions/browser/api/m...
extensions/browser/api/management/management_api.cc:542: } else {  // No confirm
dialog.
This is (at least, allowing me to not squint at this code too much) a behavioral
change? Maybe it's still correct, but it's too cute for me to tell without
squinting.

I'd rather keep at least the old structure... sort of. A case for showing a
confirm dialogue, a case otherwise. But better. Importantly, scope reading
|auto_confirm_for_test| to as minimal as possible.

Could you write it more like:


// The confirm dialogue needs to be shown for any extension trying
// to uninstall another extension.
// Note: null extension means it's WebUI.
show_confirm_dialog |= extension.get() &&
                       extension->id() != target_extension_id_;

if (show_confirm_dialog) {
  if (auto_confirm_for_test != DO_NOT_SKIP) {
    // Just kidding. If a test wants to skip it, so be it.
    base::MessageLoop::current()->PostTask(FROM_HERE, base::Bind(
        &Finish, this, auto_confirm_for_test == PROCEED));
  } else {
    AddRef();  // Balanced somewhere.
    uninstall_dialog_ = ...;
  }
} else {
  base::MessageLoop::current()->PostTask(FROM_HERE, base::Bind(
      &Finish, this, true));
}
return RespondLater();


Which happy means moving that bit of logic out of the delegate.

https://codereview.chromium.org/948413005/diff/20001/extensions/browser/api/m...
extensions/browser/api/management/management_api.cc:553: should_uninstall));  //
This bind adds a ref.
Seems like an obvious thing to comment?

https://codereview.chromium.org/948413005/diff/20001/extensions/browser/api/m...
extensions/browser/api/management/management_api.cc:592:
Respond(response.Pass());
Inline |response|?

https://codereview.chromium.org/948413005/diff/20001/extensions/browser/api/m...
extensions/browser/api/management/management_api.cc:622:
*params->options->show_confirm_dialog;
The Base already does this checking logic, no need to do it twice. This whole
function can be 1 (long) line.

https://codereview.chromium.org/948413005/diff/20001/extensions/browser/api/m...
extensions/browser/api/management/management_api.cc:624: if (show_confirm_dialog
&& !user_gesture())
The user gesture logic should be moved into the base function.

[Devlin, 2015-02-25 23:12:48]

https://codereview.chromium.org/948413005/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/management/chrome_management_api_delegate.cc
(right):

https://codereview.chromium.org/948413005/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/management/chrome_management_api_delegate.cc:94:
// If this is a self uninstall, or triggered by webui (in which case
On 2015/02/25 21:37:57, kalman wrote:
> This decision belongs in the API implementation, not the Delegate. See comment
> there.

Done.

https://codereview.chromium.org/948413005/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/management/management_api_unittest.cc
(right):

https://codereview.chromium.org/948413005/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api_unittest.cc:141:
TEST_F(ManagementApiUnitTest, ManagementUninstall) {
On 2015/02/25 21:37:57, kalman wrote:
> Looks like this test can be split up into several smaller tests.
> - uninstall w/o user gesture
> - uninstall w/ user gesture
> - uninstall but cancelled
> - uninstall self

Given the amount of set-up/tear-down needed for each test, this seems wasteful
to me, and makes me sad.  I still think that this is reasonably targeted (it's
not ManagementApiUnittestEverything), so I'd like to keep it as-is, if that's
okay.

https://codereview.chromium.org/948413005/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api_unittest.cc:143:
ResetThreadBundle(content::TestBrowserThreadBundle::DEFAULT);
On 2015/02/25 21:37:57, kalman wrote:
> See comment in ExtensionServiceTestBase.

Acknowledged.

https://codereview.chromium.org/948413005/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api_unittest.cc:177: // The
install should have failed.
On 2015/02/25 21:37:57, kalman wrote:
> uninstall?

Done.

https://codereview.chromium.org/948413005/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_service_test_base.cc (right):

https://codereview.chromium.org/948413005/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_service_test_base.cc:37: const int
kThreadOptions = content::TestBrowserThreadBundle::IO_MAINLOOP;
On 2015/02/25 21:37:57, kalman wrote:
> UI thread makes more sense for a default test thread. Very few functions live
on
> the IO thread.

But breaks tons of other stuff, as discussed offline. Leaving like this for now.

https://codereview.chromium.org/948413005/diff/20001/chrome/browser/resources...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/948413005/diff/20001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_list.js:301: this.uninstallShown_
= true;
On 2015/02/25 21:37:57, kalman wrote:
> uninstallIsShowing would be better, Shown could mean it was shown in the past.

Called it uninstallShown to match, e.g., optionsShown, but I'm not picky. Done.

https://codereview.chromium.org/948413005/diff/20001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_list.js:302: // TODO(devlin): What
should we do if the uninstall fails?
On 2015/02/25 21:37:57, kalman wrote:
> TODO belongs in the callback body.

Done.

https://codereview.chromium.org/948413005/diff/20001/extensions/browser/api/m...
File extensions/browser/api/management/management_api.cc (right):

https://codereview.chromium.org/948413005/diff/20001/extensions/browser/api/m...
extensions/browser/api/management/management_api.cc:520: extension_id_ =
target_extension_id;
On 2015/02/25 21:37:58, kalman wrote:
> ugh, can we call |extension_id_| actually |target_extension_id_|?

Done.

https://codereview.chromium.org/948413005/diff/20001/extensions/browser/api/m...
extensions/browser/api/management/management_api.cc:536: if (show_confirm_dialog
&& auto_confirm_for_test == DO_NOT_SKIP) {
On 2015/02/25 21:37:58, kalman wrote:
> There are just too many checks for |auto_confirm_for_test| for me to really
have
> much confidence in those unit tests being useful. Bah. I have some suggestions
> below.

Done.

https://codereview.chromium.org/948413005/diff/20001/extensions/browser/api/m...
extensions/browser/api/management/management_api.cc:542: } else {  // No confirm
dialog.
On 2015/02/25 21:37:58, kalman wrote:
> This is (at least, allowing me to not squint at this code too much) a
behavioral
> change? Maybe it's still correct, but it's too cute for me to tell without
> squinting.
> 
> I'd rather keep at least the old structure... sort of. A case for showing a
> confirm dialogue, a case otherwise. But better. Importantly, scope reading
> |auto_confirm_for_test| to as minimal as possible.
> 
> Could you write it more like:
> 
> 
> // The confirm dialogue needs to be shown for any extension trying
> // to uninstall another extension.
> // Note: null extension means it's WebUI.
> show_confirm_dialog |= extension.get() &&
>                        extension->id() != target_extension_id_;
> 
> if (show_confirm_dialog) {
>   if (auto_confirm_for_test != DO_NOT_SKIP) {
>     // Just kidding. If a test wants to skip it, so be it.
>     base::MessageLoop::current()->PostTask(FROM_HERE, base::Bind(
>         &Finish, this, auto_confirm_for_test == PROCEED));
>   } else {
>     AddRef();  // Balanced somewhere.
>     uninstall_dialog_ = ...;
>   }
> } else {
>   base::MessageLoop::current()->PostTask(FROM_HERE, base::Bind(
>       &Finish, this, true));
> }
> return RespondLater();
> 
> 
> Which happy means moving that bit of logic out of the delegate.

Duplicating a post task is particularly ugly, IMO.  But, whatevs.  And that code
doesn't do anything to move the logic out of the delegate.  Added some that can
help a bit.

https://codereview.chromium.org/948413005/diff/20001/extensions/browser/api/m...
extensions/browser/api/management/management_api.cc:553: should_uninstall));  //
This bind adds a ref.
On 2015/02/25 21:37:58, kalman wrote:
> Seems like an obvious thing to comment?

Done.

https://codereview.chromium.org/948413005/diff/20001/extensions/browser/api/m...
extensions/browser/api/management/management_api.cc:592:
Respond(response.Pass());
On 2015/02/25 21:37:57, kalman wrote:
> Inline |response|?

Done.

https://codereview.chromium.org/948413005/diff/20001/extensions/browser/api/m...
extensions/browser/api/management/management_api.cc:622:
*params->options->show_confirm_dialog;
On 2015/02/25 21:37:57, kalman wrote:
> The Base already does this checking logic, no need to do it twice. This whole
> function can be 1 (long) line.

Done, ish.  Too messy for one line.

https://codereview.chromium.org/948413005/diff/20001/extensions/browser/api/m...
extensions/browser/api/management/management_api.cc:624: if (show_confirm_dialog
&& !user_gesture())
On 2015/02/25 21:37:58, kalman wrote:
> The user gesture logic should be moved into the base function.

Done.

[not at google - send to devlin, 2015-02-25 23:55:58]

lgtm

https://codereview.chromium.org/948413005/diff/20001/chrome/browser/resources...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/948413005/diff/20001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_list.js:301: this.uninstallShown_
= true;
On 2015/02/25 23:12:47, Devlin wrote:
> On 2015/02/25 21:37:57, kalman wrote:
> > uninstallIsShowing would be better, Shown could mean it was shown in the
past.
> 
> Called it uninstallShown to match, e.g., optionsShown, but I'm not picky.
Done.

Should be optionsIsShowing :P

https://codereview.chromium.org/948413005/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/management/chrome_management_api_delegate.cc
(right):

https://codereview.chromium.org/948413005/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/management/chrome_management_api_delegate.cc:77:
const extensions::Extension* target_extension,
Well, technically you can read the target extension from the function, so
passing them both in seems arbitrary. In fact if you added a
GetTargetExtension() function, you'd cut down on some of that other boilerplate.

However, a whole ExtensionFunction is a big bundle of state, it'd be nice if the
dependencies were tighter - just browser context, source, target, and - if you
want to be especially thorough - the bool, too (as opposed to just checking
whether the source is null).

Up to you.

https://codereview.chromium.org/948413005/diff/40001/extensions/browser/api/m...
File extensions/browser/api/management/management_api.cc (right):

https://codereview.chromium.org/948413005/diff/40001/extensions/browser/api/m...
extensions/browser/api/management/management_api.cc:589: const Extension*
extension =
target_extension

[Devlin, 2015-02-26 00:19:54]

https://codereview.chromium.org/948413005/diff/20001/chrome/browser/resources...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/948413005/diff/20001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_list.js:301: this.uninstallShown_
= true;
On 2015/02/25 23:55:58, kalman wrote:
> On 2015/02/25 23:12:47, Devlin wrote:
> > On 2015/02/25 21:37:57, kalman wrote:
> > > uninstallIsShowing would be better, Shown could mean it was shown in the
> past.
> > 
> > Called it uninstallShown to match, e.g., optionsShown, but I'm not picky.
> Done.
> 
> Should be optionsIsShowing :P

haha yeah, it should.

https://codereview.chromium.org/948413005/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/management/chrome_management_api_delegate.cc
(right):

https://codereview.chromium.org/948413005/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/management/chrome_management_api_delegate.cc:77:
const extensions::Extension* target_extension,
On 2015/02/25 23:55:58, kalman wrote:
> Well, technically you can read the target extension from the function, so
> passing them both in seems arbitrary. In fact if you added a
> GetTargetExtension() function, you'd cut down on some of that other
boilerplate.
> 
> However, a whole ExtensionFunction is a big bundle of state, it'd be nice if
the
> dependencies were tighter - just browser context, source, target, and - if you
> want to be especially thorough - the bool, too (as opposed to just checking
> whether the source is null).
> 
> Up to you.

Agreed, the function is a lot of state.  But we need it, since we forward the
results of the uninstall dialog to it in ExtensionUninstallCanceled/Accepted()
(I know it's ugly, but that's not this patch's fault ;)).  We also can't just
pass in the bool, since it's not enough to know if the source extension is null
(since we show a regular uninstall for self-uninstalls).  So, we need the
function and the bool.  We could expose a GetTargetExtension() method on the
function, but that's just putting a greater dependency on passing in the
function, so I'd like to avoid it.

https://codereview.chromium.org/948413005/diff/40001/extensions/browser/api/m...
File extensions/browser/api/management/management_api.cc (right):

https://codereview.chromium.org/948413005/diff/40001/extensions/browser/api/m...
extensions/browser/api/management/management_api.cc:589: const Extension*
extension =
On 2015/02/25 23:55:58, kalman wrote:
> target_extension

Done.

[Devlin, 2015-02-26 21:22:56]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2015-02-26 21:22:57]

The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/948413005/#ps80001 (title:
"Latest master")

[commit-bot: I haz the power, 2015-02-26 21:24:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/948413005/80001

[commit-bot: I haz the power, 2015-02-26 21:27:41]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-02-26 21:32:08]

Patchset 4 (id:??) landed as
https://crrev.com/529ad1a90fdcd3b97aaa64d3c26081ddd9871f21
Cr-Commit-Position: refs/heads/master@{#318317}

[Dan Beam, 2015-03-03 00:12:58]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2015-03-03 00:12:59]

fixing here: https://codereview.chromium.org/971993002/ (haven't tried yet,
though)

https://codereview.chromium.org/948413005/diff/80001/chrome/browser/resources...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/948413005/diff/80001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_list.js:903:
ExtensionList.uninstallCancel = function() {
dead code now :(

https://codereview.chromium.org/948413005/diff/80001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (left):

https://codereview.chromium.org/948413005/diff/80001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:844:
web_ui()->CallJavascriptFunction("extensions.ExtensionList.uninstallCancel");
this functionality was dropped and code left :(