WebstorePrivate extension API cleanup, part 1

chrome/browser/extensions/api/webstore_private/webstore_private_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 "chrome/browser/extensions/api/webstore_private/webstore_private_api.h"
 
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/lazy_instance.h"
 #include "base/memory/scoped_vector.h"
@@ skipping 147 matching lines @@
 }
 
 // static
 scoped_ptr<WebstoreInstaller::Approval>
 WebstorePrivateApi::PopApprovalForTesting(
     Profile* profile, const std::string& extension_id) {
   return g_pending_approvals.Get().PopApproval(profile, extension_id);
 }
 
 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) {
     authuser_ = *params_->details.authuser;
   }
 
   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,
           context_getter);
 
   // The helper will call us back via OnWebstoreParseSuccess or
   // OnWebstoreParseFailure.
   helper->Start();
 
   // Matched with a Release in OnWebstoreParseSuccess/OnWebstoreParseFailure.
   AddRef();
 
   // 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(
     const std::string& id,
     const SkBitmap& icon,
     base::DictionaryValue* parsed_manifest) {
   CHECK_EQ(params_->details.id, id);
   CHECK(parsed_manifest);
   icon_ = icon;
   parsed_manifest_.reset(parsed_manifest);
@@ skipping 29 matching lines @@
   // Control flow finishes up in InstallUIProceed or InstallUIAbort.
 }
 
 void WebstorePrivateBeginInstallWithManifest3Function::OnWebstoreParseFailure(
     const std::string& id,
     WebstoreInstallHelper::Delegate::InstallHelperResultCode result_code,
     const std::string& error_message) {
   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();
 }
 
 void WebstorePrivateBeginInstallWithManifest3Function::InstallUIProceed() {
   // This gets cleared in CrxInstaller::ConfirmInstall(). TODO(asargent) - in
   // the future we may also want to add time-based expiration, where a whitelist
   // 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
   // to train the user to open it themselves at least once.
   approval->skip_post_install_ui = params_->details.enable_launcher;
   approval->dummy_extension = dummy_extension_;
   approval->installing_icon = gfx::ImageSkia::CreateFrom1xBitmap(icon_);
   approval->authuser = authuser_;
   g_pending_approvals.Get().PushApproval(approval.Pass());
 
   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
   // specific histogram here.
   ExtensionService::RecordPermissionMessagesHistogram(
       dummy_extension_.get(), "Extensions.Permissions_WebStoreInstall2");
 
   // Matches the AddRef in RunAsync().
   Release();
 }
 
 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
   // is never called for web store install cancellations.
   std::string histogram_name =
       user_initiated ? "Extensions.Permissions_WebStoreInstallCancel2"
                      : "Extensions.Permissions_WebStoreInstallAbort2";
   ExtensionService::RecordPermissionMessagesHistogram(dummy_extension_.get(),
                                                       histogram_name.c_str());
 
   histogram_name = user_initiated ? "Extensions.Permissions_InstallCancel2"
                                   : "Extensions.Permissions_InstallAbort2";
   ExtensionService::RecordPermissionMessagesHistogram(dummy_extension_.get(),
                                                       histogram_name.c_str());
 
   // Matches the AddRef in RunAsync().
   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() {}
 
 WebstorePrivateCompleteInstallFunction::
     ~WebstorePrivateCompleteInstallFunction() {}
 
 bool WebstorePrivateCompleteInstallFunction::RunAsync() {
   scoped_ptr<CompleteInstall::Params> params(
       CompleteInstall::Params::Create(*args_));
   EXTENSION_FUNCTION_VALIDATE(params);
@@ skipping 294 matching lines @@
 WebstorePrivateGetEphemeralAppsEnabledFunction::
     ~WebstorePrivateGetEphemeralAppsEnabledFunction() {}
 
 bool WebstorePrivateGetEphemeralAppsEnabledFunction::RunSync() {
   results_ = GetEphemeralAppsEnabled::Results::Create(
       EphemeralAppLauncher::IsFeatureEnabledInWebstore());
   return true;
 }
 
 }  // namespace extensions