WebstorePrivate extension API cleanup, part 1

chrome/browser/extensions/api/webstore_private/webstore_private_api.cc

Index: chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
diff --git a/chrome/browser/extensions/api/webstore_private/webstore_private_api.cc b/chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
index 52aa1b88a57fc79a9d2e9ed1e71c07f1ea9f2f0a..528219741fe9db6680a40656784e5bede877ecdc 100644
--- a/chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
+++ b/chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
@@ -165,38 +165,30 @@ WebstorePrivateApi::PopApprovalForTesting(
 }
 
 WebstorePrivateBeginInstallWithManifest3Function::
-    WebstorePrivateBeginInstallWithManifest3Function() {
+    WebstorePrivateBeginInstallWithManifest3Function() : chrome_details_(this) {
 }
 
 WebstorePrivateBeginInstallWithManifest3Function::
     ~WebstorePrivateBeginInstallWithManifest3Function() {
 }
 
-bool WebstorePrivateBeginInstallWithManifest3Function::RunAsync() {
+ExtensionFunction::ResponseAction
+WebstorePrivateBeginInstallWithManifest3Function::Run() {
   params_ = BeginInstallWithManifest3::Params::Create(*args_);
   EXTENSION_FUNCTION_VALIDATE(params_);
 
-  if (!crx_file::id_util::IdIsValid(params_->details.id)) {
-    SetResultCode(INVALID_ID);
-    error_ = kInvalidIdError;
-    return false;
-  }
+  if (!crx_file::id_util::IdIsValid(params_->details.id))
+    return RespondNowWithError(INVALID_ID, kInvalidIdError);
 
-  if (params_->details.icon_data && params_->details.icon_url) {
-    SetResultCode(ICON_ERROR);
-    error_ = kCannotSpecifyIconDataAndUrlError;
-    return false;
-  }
+  if (params_->details.icon_data && params_->details.icon_url)
+    return RespondNowWithError(ICON_ERROR, kCannotSpecifyIconDataAndUrlError);
 
   GURL icon_url;
   if (params_->details.icon_url) {
     std::string tmp_url;
     icon_url = source_url().Resolve(*params_->details.icon_url);
-    if (!icon_url.is_valid()) {
-      SetResultCode(INVALID_ICON_URL);
-      error_ = kInvalidIconUrlError;
-      return false;
-    }
+    if (!icon_url.is_valid())
+      return RespondNowWithError(INVALID_ICON_URL, kInvalidIconUrlError);
   }
 
   if (params_->details.authuser) {
@@ -206,21 +198,19 @@ bool WebstorePrivateBeginInstallWithManifest3Function::RunAsync() {
   std::string icon_data = params_->details.icon_data ?
       *params_->details.icon_data : std::string();
 
-  Profile* profile = GetProfile();
-  InstallTracker* tracker = InstallTracker::Get(profile);
+  InstallTracker* tracker = InstallTracker::Get(browser_context());
   DCHECK(tracker);
-  if (util::IsExtensionInstalledPermanently(params_->details.id, profile) ||
+  if (util::IsExtensionInstalledPermanently(params_->details.id,
+                                            browser_context()) ||
       tracker->GetActiveInstall(params_->details.id)) {
-    SetResultCode(ALREADY_INSTALLED);
-    error_ = kAlreadyInstalledError;
-    return false;
+    return RespondNowWithError(ALREADY_INSTALLED, kAlreadyInstalledError);
   }
   ActiveInstallData install_data(params_->details.id);
   scoped_active_install_.reset(new ScopedActiveInstall(tracker, install_data));
 
   net::URLRequestContextGetter* context_getter = NULL;
   if (!icon_url.is_empty())
-    context_getter = GetProfile()->GetRequestContext();
+    context_getter = browser_context()->GetRequestContext();
 
   scoped_refptr<WebstoreInstallHelper> helper = new WebstoreInstallHelper(
       this, params_->details.id, params_->details.manifest, icon_data, icon_url,
@@ -235,39 +225,7 @@ bool WebstorePrivateBeginInstallWithManifest3Function::RunAsync() {
 
   // The response is sent asynchronously in OnWebstoreParseSuccess/
   // OnWebstoreParseFailure.
-  return true;
-}
-
-const char* WebstorePrivateBeginInstallWithManifest3Function::
-    ResultCodeToString(ResultCode code) {
-  switch (code) {
-    case ERROR_NONE:
-      return "";
-    case UNKNOWN_ERROR:
-      return "unknown_error";
-    case USER_CANCELLED:
-      return "user_cancelled";
-    case MANIFEST_ERROR:
-      return "manifest_error";
-    case ICON_ERROR:
-      return "icon_error";
-    case INVALID_ID:
-      return "invalid_id";
-    case PERMISSION_DENIED:
-      return "permission_denied";
-    case INVALID_ICON_URL:
-      return "invalid_icon_url";
-    case ALREADY_INSTALLED:
-      return "already_installed";
-  }
-  NOTREACHED();
-  return "";
-}
-
-void WebstorePrivateBeginInstallWithManifest3Function::SetResultCode(
-    ResultCode code) {
-  results_ = BeginInstallWithManifest3::Results::Create(
-      ResultCodeToString(code));
+  return RespondLater();
 }
 
 void WebstorePrivateBeginInstallWithManifest3Function::OnWebstoreParseSuccess(
@@ -317,21 +275,21 @@ void WebstorePrivateBeginInstallWithManifest3Function::OnWebstoreParseFailure(
   CHECK_EQ(params_->details.id, id);
 
   // Map from WebstoreInstallHelper's result codes to ours.
+  ResultCode code;
   switch (result_code) {
     case WebstoreInstallHelper::Delegate::UNKNOWN_ERROR:
-      SetResultCode(UNKNOWN_ERROR);
+      code = UNKNOWN_ERROR;
       break;
     case WebstoreInstallHelper::Delegate::ICON_ERROR:
-      SetResultCode(ICON_ERROR);
+      code = ICON_ERROR;
       break;
     case WebstoreInstallHelper::Delegate::MANIFEST_ERROR:
-      SetResultCode(MANIFEST_ERROR);
+      code = MANIFEST_ERROR;
       break;
     default:

[not at google - send to devlin, 2015/02/05 17:16:17]

It's better to leave out the default case so that leaving out any unclaimed
cases is a compile error. You could initialize |code| to ERROR_NONE then
[D]CHECK_NE(ERROR_NONE) before calling RespondWithError.

[Marc Treib, 2015/02/06 10:26:15]

Done.

       CHECK(false);
   }
-  error_ = error_message;
-  SendResponse(false);
+  RespondWithError(code, error_message);
 
   // Matches the AddRef in RunAsync().
   Release();
@@ -343,7 +301,10 @@ void WebstorePrivateBeginInstallWithManifest3Function::InstallUIProceed() {
   // entry is only valid for some number of minutes.
   scoped_ptr<WebstoreInstaller::Approval> approval(
       WebstoreInstaller::Approval::CreateWithNoInstallPrompt(
-          GetProfile(), params_->details.id, parsed_manifest_.Pass(), false));
+          chrome_details_.GetProfile(),
+          params_->details.id,
+          parsed_manifest_.Pass(),
+          false));
   approval->use_app_installed_bubble = params_->details.app_install_bubble;
   approval->enable_launcher = params_->details.enable_launcher;
   // If we are enabling the launcher, we should not show the app list in order
@@ -357,8 +318,7 @@ void WebstorePrivateBeginInstallWithManifest3Function::InstallUIProceed() {
   DCHECK(scoped_active_install_.get());
   scoped_active_install_->CancelDeregister();
 
-  SetResultCode(ERROR_NONE);
-  SendResponse(true);
+  RespondWithSuccess();
 
   // The Permissions_Install histogram is recorded from the ExtensionService
   // for all extension installs, so we only need to record the web store
@@ -372,9 +332,7 @@ void WebstorePrivateBeginInstallWithManifest3Function::InstallUIProceed() {
 
 void WebstorePrivateBeginInstallWithManifest3Function::InstallUIAbort(
     bool user_initiated) {
-  error_ = kUserCancelledError;
-  SetResultCode(USER_CANCELLED);
-  SendResponse(false);
+  RespondWithError(USER_CANCELLED, kUserCancelledError);
 
   // The web store install histograms are a subset of the install histograms.
   // We need to record both histograms here since CrxInstaller::InstallUIAbort
@@ -394,6 +352,62 @@ void WebstorePrivateBeginInstallWithManifest3Function::InstallUIAbort(
   Release();
 }
 
+const char* WebstorePrivateBeginInstallWithManifest3Function::
+    ResultCodeToString(ResultCode code) const {
+  switch (code) {
+    case ERROR_NONE:
+      return "";
+    case UNKNOWN_ERROR:
+      return "unknown_error";
+    case USER_CANCELLED:
+      return "user_cancelled";
+    case MANIFEST_ERROR:
+      return "manifest_error";
+    case ICON_ERROR:
+      return "icon_error";
+    case INVALID_ID:
+      return "invalid_id";
+    case PERMISSION_DENIED:
+      return "permission_denied";
+    case INVALID_ICON_URL:
+      return "invalid_icon_url";
+    case ALREADY_INSTALLED:
+      return "already_installed";
+  }
+  NOTREACHED();
+  return "";
+}
+
+ExtensionFunction::ResponseValue
+WebstorePrivateBeginInstallWithManifest3Function::BuildResponseForSuccess() {
+  return ArgumentList(
+      BeginInstallWithManifest3::Results::Create(
+          ResultCodeToString(ERROR_NONE)));
+}
+
+ExtensionFunction::ResponseValue
+WebstorePrivateBeginInstallWithManifest3Function::BuildResponseForError(
+    ResultCode code, const std::string& error) {
+  return ArgumentListWithError(
+      BeginInstallWithManifest3::Results::Create(ResultCodeToString(code)),
+      error);
+}
+
+ExtensionFunction::ResponseAction
+WebstorePrivateBeginInstallWithManifest3Function::RespondNowWithError(

[not at google - send to devlin, 2015/02/05 17:16:17]

IMO - suggestion - these 3 functions below don't save a whole lot.

RespondNowWithError(code, message)
vs
RespondNow(BuildResponseForError(code, message))

isn't that different, particularly if you call BuildResponseForError less wordy,
like "WebstoreError" (or similar). Same deal with BuildResponseForSuccess. The
advantage is fewer function calls and better composition with
Respond/RespondNow.

I make this comment because it was a deliberate decision to not supply helper
methods on ExtensionFunction like RespondError(err) and force callers to specify
Respond(Error(err)) to minimise API surface.

[Marc Treib, 2015/02/06 10:26:15]

I've removed the convenience functions. You're right, it's probably
cleaner/easier to read without them.
BuildResponseForError isn't just used for Webstore errors though, so I've left
the name as it is. I can live with the wordiness.

+    ResultCode code, const std::string& error) {
+  return RespondNow(BuildResponseForError(code, error));
+}
+
+void WebstorePrivateBeginInstallWithManifest3Function::RespondWithSuccess() {
+  Respond(BuildResponseForSuccess());
+}
+
+void WebstorePrivateBeginInstallWithManifest3Function::RespondWithError(
+    ResultCode code, const std::string& error) {
+  Respond(BuildResponseForError(code, error));
+}
+
 WebstorePrivateCompleteInstallFunction::
     WebstorePrivateCompleteInstallFunction() {}