webstore_private API implementations now uses JSON compiler generated code

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 "apps/app_launcher.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/lazy_instance.h"
@@ skipping 29 matching lines @@
 #include "content/public/browser/web_contents.h"
 #include "extensions/common/error_utils.h"
 #include "grit/chromium_strings.h"
 #include "grit/generated_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 
 using content::GpuDataManager;
 
 namespace extensions {
 
+namespace BeginInstallWithManifest3 =
+    api::webstore_private::BeginInstallWithManifest3;
+namespace CompleteInstall = api::webstore_private::CompleteInstall;
+namespace GetBrowserLogin = api::webstore_private::GetBrowserLogin;
+namespace GetIsLauncherEnabled = api::webstore_private::GetIsLauncherEnabled;
+namespace GetStoreLogin = api::webstore_private::GetStoreLogin;
+namespace GetWebGLStatus = api::webstore_private::GetWebGLStatus;
+namespace InstallBundle = api::webstore_private::InstallBundle;
+namespace IsInIncognitoMode = api::webstore_private::IsInIncognitoMode;
+namespace SetStoreLogin = api::webstore_private::SetStoreLogin;
+
 namespace {
 
 // Holds the Approvals between the time we prompt and start the installs.
 class PendingApprovals {
  public:
   PendingApprovals();
   ~PendingApprovals();
 
   void PushApproval(scoped_ptr<WebstoreInstaller::Approval> approval);
   scoped_ptr<WebstoreInstaller::Approval> PopApproval(
@@ skipping 75 matching lines @@
       return (installs_.begin() + i);
   }
   return installs_.end();
 }
 
 static base::LazyInstance<PendingApprovals> g_pending_approvals =
     LAZY_INSTANCE_INITIALIZER;
 static base::LazyInstance<PendingInstalls> g_pending_installs =
     LAZY_INSTANCE_INITIALIZER;
 
-const char kAppInstallBubbleKey[] = "appInstallBubble";
-const char kEnableLauncherKey[] = "enableLauncher";
-const char kIconDataKey[] = "iconData";
-const char kIconUrlKey[] = "iconUrl";
-const char kIdKey[] = "id";
-const char kLocalizedNameKey[] = "localizedName";
-const char kLoginKey[] = "login";
-const char kManifestKey[] = "manifest";
-
 // A preference set by the web store to indicate login information for
 // purchased apps.
 const char kWebstoreLogin[] = "extensions.webstore_login";
 const char kAlreadyInstalledError[] = "This item is already installed";
 const char kCannotSpecifyIconDataAndUrlError[] =
     "You cannot specify both icon data and an icon url";
 const char kInvalidIconUrlError[] = "Invalid icon url";
 const char kInvalidIdError[] = "Invalid id";
 const char kInvalidManifestError[] = "Invalid manifest";
 const char kNoPreviousBeginInstallWithManifestError[] =
     "* does not match a previous call to beginInstallWithManifest3";
 const char kUserCancelledError[] = "User cancelled install";
 
-// Helper to create a dictionary with login properties set from the appropriate
-// values in the passed-in |profile|.
-base::DictionaryValue* CreateLoginResult(Profile* profile) {
-  base::DictionaryValue* dictionary = new base::DictionaryValue();
-  std::string username = profile->GetPrefs()->GetString(
-      prefs::kGoogleServicesUsername);
-  dictionary->SetString(kLoginKey, username);
-  return dictionary;
-}
-
 WebstoreInstaller::Delegate* test_webstore_installer_delegate = NULL;
 
 // We allow the web store to set a string containing login information when a
 // purchase is made, so that when a user logs into sync with a different
 // account we can recognize the situation. The Get function returns the login if
 // there was previously stored data, or an empty string otherwise. The Set will
 // overwrite any previous login.
 std::string GetWebstoreLogin(Profile* profile) {
   if (profile->GetPrefs()->HasPrefPath(kWebstoreLogin))
     return profile->GetPrefs()->GetString(kWebstoreLogin);
@@ skipping 16 matching lines @@
 scoped_ptr<WebstoreInstaller::Approval>
 WebstorePrivateApi::PopApprovalForTesting(
     Profile* profile, const std::string& extension_id) {
   return g_pending_approvals.Get().PopApproval(profile, extension_id);
 }
 
 WebstorePrivateInstallBundleFunction::WebstorePrivateInstallBundleFunction() {}
 WebstorePrivateInstallBundleFunction::~WebstorePrivateInstallBundleFunction() {}
 
 bool WebstorePrivateInstallBundleFunction::RunImpl() {
-  base::ListValue* extensions = NULL;
-  EXTENSION_FUNCTION_VALIDATE(args_->GetList(0, &extensions));
+  scoped_ptr<InstallBundle::Params> params(
+      InstallBundle::Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params.get());

[not at google - send to devlin, 2013/08/12 20:48:38]

VALIDATE(params) is enough

[pals, 2013/08/14 09:12:31]

Done.

 
   BundleInstaller::ItemList items;
-  if (!ReadBundleInfo(extensions, &items))
+  if (!ReadBundleInfo(params.get(), &items))
     return false;
 
   bundle_ = new BundleInstaller(GetCurrentBrowser(), items);
 
   AddRef();  // Balanced in OnBundleInstallCompleted / OnBundleInstallCanceled.
 
   bundle_->PromptForApproval(this);
   return true;
 }
 
 bool WebstorePrivateInstallBundleFunction::
-    ReadBundleInfo(base::ListValue* extensions,
+    ReadBundleInfo(InstallBundle::Params* params,

[not at google - send to devlin, 2013/08/12 20:48:38]

make this a const ref?

[pals, 2013/08/14 09:12:31]

Done.

     BundleInstaller::ItemList* items) {
-  for (size_t i = 0; i < extensions->GetSize(); ++i) {
-    base::DictionaryValue* details = NULL;
-    EXTENSION_FUNCTION_VALIDATE(extensions->GetDictionary(i, &details));
-
+  for (size_t i = 0; i < params->details.size(); ++i) {
     BundleInstaller::Item item;
-    EXTENSION_FUNCTION_VALIDATE(details->GetString(
-        kIdKey, &item.id));
-    EXTENSION_FUNCTION_VALIDATE(details->GetString(
-        kManifestKey, &item.manifest));
-    EXTENSION_FUNCTION_VALIDATE(details->GetString(
-        kLocalizedNameKey, &item.localized_name));
-
+    item.id = params->details[i]->id;
+    item.manifest = params->details[i]->manifest;
+    item.localized_name = params->details[i]->localized_name;
     items->push_back(item);
   }
 
   return true;
 }
 
 void WebstorePrivateInstallBundleFunction::OnBundleInstallApproved() {
   bundle_->CompleteInstall(
       &(dispatcher()->delegate()->GetAssociatedWebContents()->GetController()),
       this);
@@ skipping 18 matching lines @@
 }
 
 WebstorePrivateBeginInstallWithManifest3Function::
     WebstorePrivateBeginInstallWithManifest3Function()
     : use_app_installed_bubble_(false), enable_launcher_(false) {}
 
 WebstorePrivateBeginInstallWithManifest3Function::
     ~WebstorePrivateBeginInstallWithManifest3Function() {}
 
 bool WebstorePrivateBeginInstallWithManifest3Function::RunImpl() {
-  base::DictionaryValue* details = NULL;
-  EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(0, &details));
-  CHECK(details);
+  scoped_ptr<BeginInstallWithManifest3::Params> params(
+      BeginInstallWithManifest3::Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params.get());
+  CHECK(params.get());

[not at google - send to devlin, 2013/08/12 20:48:38]

270 is redundant, EFV(params) will return if it fails.

[pals, 2013/08/14 09:12:31]

Done.

 
-  EXTENSION_FUNCTION_VALIDATE(details->GetString(kIdKey, &id_));
+  id_ = params->details.id;
   if (!extensions::Extension::IdIsValid(id_)) {
     SetResultCode(INVALID_ID);
     error_ = kInvalidIdError;
     return false;
   }
 
-  EXTENSION_FUNCTION_VALIDATE(details->GetString(kManifestKey, &manifest_));
+  manifest_ = params->details.manifest;
 
-  if (details->HasKey(kIconDataKey) && details->HasKey(kIconUrlKey)) {
+  if (params->details.icon_data.get() && params->details.icon_url.get()) {

[not at google - send to devlin, 2013/08/12 20:48:38]

I won't make this comment on every instance of this since it'll be too noisy;
but scoped_ptrs know how to be tested in boolean operations, so you just need

params->details.icon_data && params->details.icon_url

no .get()s.

[pals, 2013/08/14 09:12:31]

Done.

     SetResultCode(ICON_ERROR);
     error_ = kCannotSpecifyIconDataAndUrlError;
     return false;
   }
 
-  if (details->HasKey(kIconDataKey))
-    EXTENSION_FUNCTION_VALIDATE(details->GetString(kIconDataKey, &icon_data_));
+  if(params->details.icon_data.get())

[not at google - send to devlin, 2013/08/12 20:48:38]

space between if and (, ditto elsewhere.

[pals, 2013/08/14 09:12:31]

Done.

+    icon_data_ = *params->details.icon_data;
 
   GURL icon_url;
-  if (details->HasKey(kIconUrlKey)) {
+  if(params->details.icon_url.get()) {
     std::string tmp_url;
-    EXTENSION_FUNCTION_VALIDATE(details->GetString(kIconUrlKey, &tmp_url));
-    icon_url = source_url().Resolve(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 (details->HasKey(kLocalizedNameKey))
-    EXTENSION_FUNCTION_VALIDATE(details->GetString(kLocalizedNameKey,
-                                                   &localized_name_));
+  if(params->details.localized_name.get())
+    localized_name_ = *params->details.localized_name;
 
-  if (details->HasKey(kAppInstallBubbleKey))
-    EXTENSION_FUNCTION_VALIDATE(details->GetBoolean(
-        kAppInstallBubbleKey, &use_app_installed_bubble_));
+  if(params->details.app_install_bubble.get())
+    use_app_installed_bubble_ = *params->details.app_install_bubble;
 
-  if (details->HasKey(kEnableLauncherKey))
-    EXTENSION_FUNCTION_VALIDATE(details->GetBoolean(
-        kEnableLauncherKey, &enable_launcher_));
+  if(params->details.enable_launcher.get())
+    enable_launcher_ = *params->details.enable_launcher;

[not at google - send to devlin, 2013/08/12 20:48:38]

all of this assigning into variables makes me thing we should just hold onto the
params as a member variable directly rather than expanding it out like this.

[pals, 2013/08/14 09:12:31]

Done.

 
   ExtensionService* service =
     extensions::ExtensionSystem::Get(profile_)->extension_service();
   if (service->GetInstalledExtension(id_) ||
       !g_pending_installs.Get().InsertInstall(profile_, id_)) {
     SetResultCode(ALREADY_INSTALLED);
     error_ = kAlreadyInstalledError;
     return false;
   }
 
@@ skipping 14 matching lines @@
   // The response is sent asynchronously in OnWebstoreParseSuccess/
   // OnWebstoreParseFailure.
   return true;
 }
 
 
 void WebstorePrivateBeginInstallWithManifest3Function::SetResultCode(
     ResultCode code) {
   switch (code) {
     case ERROR_NONE:
-      SetResult(Value::CreateStringValue(std::string()));
+      results_ = BeginInstallWithManifest3::Results::Create(std::string());
       break;
     case UNKNOWN_ERROR:
-      SetResult(Value::CreateStringValue("unknown_error"));
+      results_ = BeginInstallWithManifest3::Results::Create("unknown_error");
       break;
     case USER_CANCELLED:
-      SetResult(Value::CreateStringValue("user_cancelled"));
+      results_ = BeginInstallWithManifest3::Results::Create("user_cancelled");
       break;
     case MANIFEST_ERROR:
-      SetResult(Value::CreateStringValue("manifest_error"));
+      results_ = BeginInstallWithManifest3::Results::Create("manifest_error");
       break;
     case ICON_ERROR:
-      SetResult(Value::CreateStringValue("icon_error"));
+      results_ = BeginInstallWithManifest3::Results::Create("icon_error");
       break;
     case INVALID_ID:
-      SetResult(Value::CreateStringValue("invalid_id"));
+      results_ = BeginInstallWithManifest3::Results::Create("invalid_id");
       break;
     case PERMISSION_DENIED:
-      SetResult(Value::CreateStringValue("permission_denied"));
+      results_ = BeginInstallWithManifest3::Results::Create(
+          "permission_denied");
       break;
     case INVALID_ICON_URL:
-      SetResult(Value::CreateStringValue("invalid_icon_url"));
+      results_ = BeginInstallWithManifest3::Results::Create("invalid_icon_url");
       break;
     case SIGNIN_FAILED:
-      SetResult(Value::CreateStringValue("signin_failed"));
+      results_ = BeginInstallWithManifest3::Results::Create("signin_failed");
       break;
     case ALREADY_INSTALLED:
-      SetResult(Value::CreateStringValue("already_installed"));
+      results_ = BeginInstallWithManifest3::Results::Create(
+          "already_installed");
       break;
     default:
       CHECK(false);

[not at google - send to devlin, 2013/08/12 20:48:38]

pull this switch into a separate function which returns the appropriate string.
It'll be more concise.

SetResultCode(code) {
  results_ = BIWM3::Results::Create(ResultCodeToString(code));
}

const char* ResultCodeToString(code) {
  switch (code) {
    case ERROR_NONE:
      return "";
  }
}

etc.

then, don't have a default case (so that compiler will error if an enum case is
missing) then NOTREACHED if it fails to return. And return "" I guess.

[pals, 2013/08/14 09:12:31]

Done.

   }
 }
 
 void WebstorePrivateBeginInstallWithManifest3Function::OnWebstoreParseSuccess(
     const std::string& id,
     const SkBitmap& icon,
     base::DictionaryValue* parsed_manifest) {
   CHECK_EQ(id_, id);
   CHECK(parsed_manifest);
   icon_ = icon;
@@ skipping 143 matching lines @@
   Release();
 }
 
 WebstorePrivateCompleteInstallFunction::
     WebstorePrivateCompleteInstallFunction() {}
 
 WebstorePrivateCompleteInstallFunction::
     ~WebstorePrivateCompleteInstallFunction() {}
 
 bool WebstorePrivateCompleteInstallFunction::RunImpl() {
-  std::string id;
-  EXTENSION_FUNCTION_VALIDATE(args_->GetString(0, &id));
-  if (!extensions::Extension::IdIsValid(id)) {
+  scoped_ptr<CompleteInstall::Params> params(
+      CompleteInstall::Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params.get());
+  if (!extensions::Extension::IdIsValid(params->expected_id)) {
     error_ = kInvalidIdError;
     return false;
   }
 
-  approval_ = g_pending_approvals.Get().PopApproval(profile(), id).Pass();
+  approval_ = g_pending_approvals.Get().PopApproval(profile(),
+      params->expected_id).Pass();
   if (!approval_) {
     error_ = ErrorUtils::FormatErrorMessage(
-        kNoPreviousBeginInstallWithManifestError, id);
+        kNoPreviousBeginInstallWithManifestError, params->expected_id);
     return false;
   }
 
   // Balanced in OnExtensionInstallSuccess() or OnExtensionInstallFailure().
   AddRef();
 
   if (approval_->enable_launcher)
     AppListService::Get()->EnableAppList(profile());
 
   if (apps::IsAppLauncherEnabled()) {
     // Show the app list to show download is progressing. Don't show the app
     // list on first app install so users can be trained to open it themselves.
     if (approval_->manifest->is_app() && !approval_->enable_launcher)
       AppListService::Get()->ShowForProfile(profile());
   }
 
   // The extension will install through the normal extension install flow, but
   // the whitelist entry will bypass the normal permissions install dialog.
   scoped_refptr<WebstoreInstaller> installer = new WebstoreInstaller(
       profile(), this,
       &(dispatcher()->delegate()->GetAssociatedWebContents()->GetController()),
-      id, approval_.Pass(), WebstoreInstaller::FLAG_NONE);
+      params->expected_id, approval_.Pass(), WebstoreInstaller::FLAG_NONE);
   installer->Start();
 
   return true;
 }
 
 void WebstorePrivateCompleteInstallFunction::OnExtensionInstallSuccess(
     const std::string& id) {
   if (test_webstore_installer_delegate)
     test_webstore_installer_delegate->OnExtensionInstallSuccess(id);
 
@@ skipping 29 matching lines @@
 WebstorePrivateEnableAppLauncherFunction::
     ~WebstorePrivateEnableAppLauncherFunction() {}
 
 bool WebstorePrivateEnableAppLauncherFunction::RunImpl() {
   AppListService::Get()->EnableAppList(profile());
   SendResponse(true);
   return true;
 }
 
 bool WebstorePrivateGetBrowserLoginFunction::RunImpl() {
-  SetResult(CreateLoginResult(profile_->GetOriginalProfile()));
+  GetBrowserLogin::Results::Info info;
+  info.login = profile_->GetOriginalProfile()->GetPrefs()->GetString(
+      prefs::kGoogleServicesUsername);
+  results_ = GetBrowserLogin::Results::Create(info);
   return true;
 }
 
 bool WebstorePrivateGetStoreLoginFunction::RunImpl() {
-  SetResult(Value::CreateStringValue(GetWebstoreLogin(profile_)));
+  results_ = GetStoreLogin::Results::Create(GetWebstoreLogin(profile_));
   return true;
 }
 
 bool WebstorePrivateSetStoreLoginFunction::RunImpl() {
-  std::string login;
-  EXTENSION_FUNCTION_VALIDATE(args_->GetString(0, &login));
-  SetWebstoreLogin(profile_, login);
+  scoped_ptr<SetStoreLogin::Params> params(
+      SetStoreLogin::Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params.get());
+  SetWebstoreLogin(profile_, params->login);
   return true;
 }
 
 WebstorePrivateGetWebGLStatusFunction::WebstorePrivateGetWebGLStatusFunction() {
   feature_checker_ = new GPUFeatureChecker(
       gpu::GPU_FEATURE_TYPE_WEBGL,
       base::Bind(&WebstorePrivateGetWebGLStatusFunction::OnFeatureCheck,
           base::Unretained(this)));
 }
 
 WebstorePrivateGetWebGLStatusFunction::
     ~WebstorePrivateGetWebGLStatusFunction() {}
 
 void WebstorePrivateGetWebGLStatusFunction::CreateResult(bool webgl_allowed) {
-  SetResult(Value::CreateStringValue(
-      webgl_allowed ? "webgl_allowed" : "webgl_blocked"));
+  results_ = GetWebGLStatus::Results::Create(GetWebGLStatus::Results::
+      ParseWebgl_status(webgl_allowed ? "webgl_allowed" : "webgl_blocked"));
 }
 
 bool WebstorePrivateGetWebGLStatusFunction::RunImpl() {
   feature_checker_->CheckGPUFeatureAvailability();
   return true;
 }
 
 void WebstorePrivateGetWebGLStatusFunction::
     OnFeatureCheck(bool feature_allowed) {
   CreateResult(feature_allowed);
   SendResponse(true);
 }
 
 bool WebstorePrivateGetIsLauncherEnabledFunction::RunImpl() {
-  SetResult(Value::CreateBooleanValue(apps::IsAppLauncherEnabled()));
+  results_ = GetIsLauncherEnabled::Results::Create(
+      apps::IsAppLauncherEnabled());
   SendResponse(true);
   return true;
 }
 
 bool WebstorePrivateIsInIncognitoModeFunction::RunImpl() {
-  SetResult(
-      Value::CreateBooleanValue(profile_ != profile_->GetOriginalProfile()));
+  results_ = IsInIncognitoMode::Results::Create(
+      profile_ != profile_->GetOriginalProfile());
   SendResponse(true);

[not at google - send to devlin, 2013/08/12 20:48:38]

Make these SyncExtensionFunctions then you won't need the SendResponse(true),
return true is enough.

You actually might not even need it either way, check the implementation of
AsyncExtensionFunction.

[pals, 2013/08/14 09:12:31]

Done.

   return true;
 }
 
 }  // namespace extensions