webstore_private API implementations now uses JSON compiler generated code

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 f10848022e39bdb6fa91348974196f5693aedaf0..8a8385a12b0cb1f8a39b4ab0d36e484a5abf0fe6 100644
--- a/chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
+++ b/chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
@@ -47,6 +47,17 @@ 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.
@@ -142,15 +153,6 @@ static base::LazyInstance<PendingApprovals> g_pending_approvals =
 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";
@@ -164,16 +166,6 @@ 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
@@ -210,11 +202,12 @@ 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);
@@ -226,20 +219,13 @@ bool WebstorePrivateInstallBundleFunction::RunImpl() {
 }
 
 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);
   }
 
@@ -278,33 +264,33 @@ 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;
@@ -312,17 +298,14 @@ bool WebstorePrivateBeginInstallWithManifest3Function::RunImpl() {
     }
   }
 
-  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();
@@ -357,34 +340,36 @@ 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.

@@ -551,17 +536,19 @@ 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;
   }
 
@@ -583,7 +570,7 @@ bool WebstorePrivateCompleteInstallFunction::RunImpl() {
   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;
@@ -633,19 +620,23 @@ bool WebstorePrivateEnableAppLauncherFunction::RunImpl() {
 }
 
 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;
 }
 
@@ -660,8 +651,8 @@ 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() {
@@ -676,14 +667,15 @@ void WebstorePrivateGetWebGLStatusFunction::
 }
 
 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;
 }