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

extensions/browser/api/management/management_api.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "extensions/browser/api/management/management_api.h"
 
 #include <string>
 #include <vector>
 
 #include "base/basictypes.h"
@@ skipping 493 matching lines @@
                   JoinString(requirements_errors, ' ')));
   }
 }
 
 ManagementUninstallFunctionBase::ManagementUninstallFunctionBase() {
 }
 
 ManagementUninstallFunctionBase::~ManagementUninstallFunctionBase() {
 }
 
-bool ManagementUninstallFunctionBase::Uninstall(
+ExtensionFunction::ResponseAction ManagementUninstallFunctionBase::Uninstall(
     const std::string& target_extension_id,
     bool show_confirm_dialog) {
   const ManagementAPIDelegate* delegate = ManagementAPI::GetFactoryInstance()
                                               ->Get(browser_context())
                                               ->GetDelegate();
   extension_id_ = target_extension_id;

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

ugh, can we call |extension_id_| actually |target_extension_id_|?

[Devlin, 2015/02/25 23:12:47]

Done.

   const Extension* target_extension =
       extensions::ExtensionRegistry::Get(browser_context())
           ->GetExtensionById(extension_id_, ExtensionRegistry::EVERYTHING);
   if (!target_extension ||
       ShouldNotBeVisible(target_extension, browser_context())) {
-    error_ =
-        ErrorUtils::FormatErrorMessage(keys::kNoExtensionError, extension_id_);
-    return false;
+    return RespondNow(Error(keys::kNoExtensionError, extension_id_));
   }
 
   ManagementPolicy* policy =
       ExtensionSystem::Get(browser_context())->management_policy();
   if (!policy->UserMayModifySettings(target_extension, nullptr) ||
       policy->MustRemainInstalled(target_extension, nullptr)) {
-    error_ = ErrorUtils::FormatErrorMessage(keys::kUserCantModifyError,
-                                            extension_id_);
-    return false;
+    return RespondNow(Error(keys::kUserCantModifyError, extension_id_));
   }
 
-  if (auto_confirm_for_test == DO_NOT_SKIP) {
-    if (show_confirm_dialog) {
-      AddRef();  // Balanced in ExtensionUninstallAccepted/Canceled
-      uninstall_dialog_ =
-          delegate->UninstallFunctionDelegate(this, target_extension_id);
-    } else {
-      Finish(true);
-    }
-  } else {
-    Finish(auto_confirm_for_test == PROCEED);
+  if (show_confirm_dialog && auto_confirm_for_test == DO_NOT_SKIP) {

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

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.

[Devlin, 2015/02/25 23:12:47]

Done.

+    AddRef();  // Balanced in ExtensionUninstallAccepted/Canceled
+    // TODO(devlin): A method called "UninstallFunctionDelegate" does not in

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

Indeed!

+    // any way imply that this actually creates a dialog and runs it.
+    uninstall_dialog_ =
+        delegate->UninstallFunctionDelegate(this, target_extension_id);
+  } else {  // No confirm dialog.

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

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.

[Devlin, 2015/02/25 23:12:47]

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.

+    // Uninstall should succeed, unless we would normally show the confirm
+    // dialog but are taking a shortcut to the cancel for a test.
+    bool should_uninstall =
+        !(auto_confirm_for_test == ABORT && show_confirm_dialog);
+    // We do this asynchronously because Finish() Respond()s, which assumes that
+    // this function doesn't.
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(&ManagementUninstallFunctionBase::Finish,
+                   this,
+                   should_uninstall));  // This bind adds a ref.

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

Seems like an obvious thing to comment?

[Devlin, 2015/02/25 23:12:47]

Done.

   }
 
-  return true;
+  return RespondLater();
 }
 
 // static
 void ManagementUninstallFunctionBase::SetAutoConfirmForTest(
     bool should_proceed) {
   auto_confirm_for_test = should_proceed ? PROCEED : ABORT;
 }
 
 void ManagementUninstallFunctionBase::Finish(bool should_uninstall) {
-  if (should_uninstall) {
-    // The extension can be uninstalled in another window while the UI was
-    // showing. Do nothing in that case.
-    ExtensionRegistry* registry = ExtensionRegistry::Get(browser_context());
-    const Extension* extension = registry->GetExtensionById(
-        extension_id_, ExtensionRegistry::EVERYTHING);
-    if (!extension) {
-      error_ = ErrorUtils::FormatErrorMessage(keys::kNoExtensionError,
-                                              extension_id_);
-      SendResponse(false);
-    } else {
-      const ManagementAPIDelegate* delegate =
-          ManagementAPI::GetFactoryInstance()
-              ->Get(browser_context())
-              ->GetDelegate();
-      bool success = delegate->UninstallExtension(
-          browser_context(), extension_id_,
-          extensions::UNINSTALL_REASON_MANAGEMENT_API,
-          base::Bind(&base::DoNothing), NULL);
+  if (!should_uninstall) {
+    Respond(Error(keys::kUninstallCanceledError, extension_id_));
+    return;
+  }
 
-      // TODO set error_ if !success
-      SendResponse(success);
-    }
-  } else {
-    error_ = ErrorUtils::FormatErrorMessage(keys::kUninstallCanceledError,
-                                            extension_id_);
-    SendResponse(false);
+  // The extension can be uninstalled in another window while the UI was
+  // showing. Do nothing in that case.
+  const Extension* extension =
+      extensions::ExtensionRegistry::Get(browser_context())
+          ->GetExtensionById(extension_id_, ExtensionRegistry::EVERYTHING);
+  if (!extension) {
+    Respond(Error(keys::kNoExtensionError, extension_id_));
+    return;
   }
+
+  const ManagementAPIDelegate* delegate =
+      ManagementAPI::GetFactoryInstance()
+          ->Get(browser_context())
+          ->GetDelegate();
+  base::string16 error;
+  bool success = delegate->UninstallExtension(
+      browser_context(), extension_id_,
+      extensions::UNINSTALL_REASON_MANAGEMENT_API,
+      base::Bind(&base::DoNothing), &error);
+  ResponseValue response =
+      success ? NoArguments() : Error(base::UTF16ToUTF8(error));
+  Respond(response.Pass());

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

Inline |response|?

[Devlin, 2015/02/25 23:12:47]

Done.

 }
 
 void ManagementUninstallFunctionBase::ExtensionUninstallAccepted() {
   Finish(true);
   Release();
 }
 
 void ManagementUninstallFunctionBase::ExtensionUninstallCanceled() {
   Finish(false);
   Release();
 }
 
 ManagementUninstallFunction::ManagementUninstallFunction() {
 }
 
 ManagementUninstallFunction::~ManagementUninstallFunction() {
 }
 
-bool ManagementUninstallFunction::RunAsync() {
+ExtensionFunction::ResponseAction ManagementUninstallFunction::Run() {
   scoped_ptr<management::Uninstall::Params> params(
       management::Uninstall::Params::Create(*args_));
-  EXTENSION_FUNCTION_VALIDATE(extension_.get());
   EXTENSION_FUNCTION_VALIDATE(params.get());
 
   bool show_confirm_dialog = true;
   // By default confirmation dialog isn't shown when uninstalling self, but this
   // can be overridden with showConfirmDialog.
-  if (params->id == extension_->id()) {
+  if (extension_.get() && params->id == extension_->id()) {
     show_confirm_dialog = params->options.get() &&
                           params->options->show_confirm_dialog.get() &&
                           *params->options->show_confirm_dialog;

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

The Base already does this checking logic, no need to do it twice. This whole
function can be 1 (long) line.

[Devlin, 2015/02/25 23:12:47]

Done, ish.  Too messy for one line.

   }
-  if (show_confirm_dialog && !user_gesture()) {
-    error_ = keys::kGestureNeededForUninstallError;
-    return false;
-  }
+  if (show_confirm_dialog && !user_gesture())

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

The user gesture logic should be moved into the base function.

[Devlin, 2015/02/25 23:12:47]

Done.

+    return RespondNow(Error(keys::kGestureNeededForUninstallError));
+
   return Uninstall(params->id, show_confirm_dialog);
 }
 
 ManagementUninstallSelfFunction::ManagementUninstallSelfFunction() {
 }
 
 ManagementUninstallSelfFunction::~ManagementUninstallSelfFunction() {
 }
 
-bool ManagementUninstallSelfFunction::RunAsync() {
+ExtensionFunction::ResponseAction ManagementUninstallSelfFunction::Run() {
   scoped_ptr<management::UninstallSelf::Params> params(
       management::UninstallSelf::Params::Create(*args_));
   EXTENSION_FUNCTION_VALIDATE(params.get());
+  EXTENSION_FUNCTION_VALIDATE(extension_.get());
 
-  bool show_confirm_dialog = false;
-  if (params->options.get() && params->options->show_confirm_dialog.get())
-    show_confirm_dialog = *params->options->show_confirm_dialog;
+  bool show_confirm_dialog = params->options.get() &&
+                             params->options->show_confirm_dialog.get() &&
+                             *params->options->show_confirm_dialog;
   return Uninstall(extension_->id(), show_confirm_dialog);
 }
 
 ManagementCreateAppShortcutFunction::ManagementCreateAppShortcutFunction() {
 }
 
 ManagementCreateAppShortcutFunction::~ManagementCreateAppShortcutFunction() {
 }
 
 // static
@@ skipping 255 matching lines @@
 ManagementAPI::GetFactoryInstance() {
   return g_factory.Pointer();
 }
 
 void ManagementAPI::OnListenerAdded(const EventListenerInfo& details) {
   management_event_router_.reset(new ManagementEventRouter(browser_context_));
   EventRouter::Get(browser_context_)->UnregisterObserver(this);
 }
 
 }  // namespace extensions