Use JSON schema compiler in chrome.management code

chrome/browser/extensions/api/management/management_api.cc

Index: chrome/browser/extensions/api/management/management_api.cc
diff --git a/chrome/browser/extensions/api/management/management_api.cc b/chrome/browser/extensions/api/management/management_api.cc
index 0af965c53154e2d883333c6ca9ad0991a3c6102a..d39b82e642287fea88c3b7e7c3ed35bd16e43679 100644
--- a/chrome/browser/extensions/api/management/management_api.cc
+++ b/chrome/browser/extensions/api/management/management_api.cc
@@ -6,13 +6,17 @@
 
 #include <map>
 #include <string>
+#include <vector>
 
 #include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/json/json_writer.h"
+#include "base/memory/linked_ptr.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/metrics/histogram.h"
 #include "base/string_number_conversions.h"
 #include "base/string_util.h"
+#include "base/utf_string_conversions.h"
 #include "chrome/browser/extensions/api/management/management_api_constants.h"
 #include "chrome/browser/extensions/event_names.h"
 #include "chrome/browser/extensions/event_router.h"
@@ -25,6 +29,7 @@
 #include "chrome/browser/ui/webui/extensions/extension_icon_source.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_utility_messages.h"
+#include "chrome/common/extensions/api/management.h"
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/extensions/extension_constants.h"
 #include "chrome/common/extensions/extension_error_utils.h"
@@ -44,14 +49,34 @@ using base::IntToString;
 using content::BrowserThread;
 using content::UtilityProcessHost;
 using content::UtilityProcessHostClient;
+using extensions::api::management::ExtensionInfo;
+using extensions::api::management::IconInfo;
 using extensions::Extension;
+using extensions::ExtensionSystem;
 using extensions::PermissionMessages;
 
+namespace Get = extensions::api::management::Get;

[Aaron Boodman, 2012/08/09 13:17:19]

I think it would be more clear to just do:

using extensions::Management.

Then we don't have to list these all individually, and it will also be more
obvious where things like GetAll come from.

[mitchellwrosen, 2012/08/09 18:15:41]

Do you mean using namespace extensions::api::management?

[Aaron Boodman, 2012/08/10 06:12:36]

I meant:

using extensions::management.

So you'd have to write, e.g., management::GetAll::<whatever>.

I know this is different than what we've done in other places, but it has the
advantages:

- one line.
- easy to see where symbols come from.
- namespaces in Chrome are typically lower case.

[mitchellwrosen, 2012/08/10 18:10:13]

Oh, I gotcha. namespace management = extensions::api::management, just like
brettw wanted in bookmarks code. I agree that it's much better looking than the
old style

+namespace GetAll = extensions::api::management::GetAll;
+namespace GetPermissionWarningsById =
+    extensions::api::management::GetPermissionWarningsById;
+namespace GetPermissionWarningsByManifest =
+    extensions::api::management::GetPermissionWarningsByManifest;
+namespace LaunchApp = extensions::api::management::LaunchApp;
+namespace OnDisabled = extensions::api::management::OnDisabled;
+namespace OnEnabled = extensions::api::management::OnEnabled;
+namespace OnInstalled = extensions::api::management::OnInstalled;
+namespace OnUninstalled = extensions::api::management::OnUninstalled;
+namespace SetEnabled = extensions::api::management::SetEnabled;
+namespace Uninstall = extensions::api::management::Uninstall;
+
 namespace events = extensions::event_names;
 namespace keys = extension_management_api_constants;
 
 namespace {
 
+typedef std::vector<linked_ptr<ExtensionInfo> > ExtensionInfoList;

[Aaron Boodman, 2012/08/09 13:17:19]

ScopedVector<T> is now preferred over std::vector<linked_ptr<T> >. It does
essentially the same thing.

[mitchellwrosen, 2012/08/09 18:15:41]

Hm. For now, the JSC uses std::vector<std::linked_ptr>>. Should that be changed?

[Aaron Boodman, 2012/08/10 06:12:36]

Ah, makes sense. Nevermind.

+typedef std::vector<linked_ptr<IconInfo> > IconInfoList;
+
 enum AutoConfirmForTest {
   DO_NOT_SKIP = 0,
   PROCEED,
@@ -60,6 +85,15 @@ enum AutoConfirmForTest {
 
 AutoConfirmForTest auto_confirm_for_test = DO_NOT_SKIP;
 
+void CreateWarningsList(const Extension* extension,

[Aaron Boodman, 2012/08/09 13:17:19]

You can just return std::vector<std::string>. It's easier to read, the copy
doesn't matter here, and is usually optimized away by the compiler anyway.

[mitchellwrosen, 2012/08/09 18:15:41]

Done.

+                        std::vector<std::string>* warnings_list) {
+  PermissionMessages warnings = extension->GetPermissionMessages();
+  for (PermissionMessages::const_iterator iter = warnings.begin();
+       iter != warnings.end(); ++iter) {
+    warnings_list->push_back(UTF16ToUTF8(iter->message()));
+  }
+}
+
 } // namespace
 
 ExtensionService* ExtensionManagementFunction::service() {
@@ -70,160 +104,164 @@ ExtensionService* AsyncExtensionManagementFunction::service() {
   return profile()->GetExtensionService();
 }
 
-static DictionaryValue* CreateExtensionInfo(const Extension& extension,
-                                            ExtensionService* service) {
-  DictionaryValue* info = new DictionaryValue();
-  bool enabled = service->IsExtensionEnabled(extension.id());
-  extension.GetBasicInfo(enabled, info);
-
-  const extensions::ManagementPolicy* policy = extensions::ExtensionSystem::Get(
-      service->profile())->management_policy();
-  info->SetBoolean(keys::kMayDisableKey,
-                   policy->UserMayModifySettings(&extension, NULL));
-
-  info->SetBoolean(keys::kIsAppKey, extension.is_app());
-
-  if (!enabled) {
+static scoped_ptr<ExtensionInfo> CreateExtensionInfo(

[Aaron Boodman, 2012/08/09 13:17:19]

Can you move this into the anonymous namespace? We prefer that over static
global functions.

[mitchellwrosen, 2012/08/09 18:15:41]

Done.

+    const Extension& extension,
+    ExtensionService* service) {
+  scoped_ptr<ExtensionInfo> info(new ExtensionInfo());
+
+  info->id = extension.id();
+  info->name = extension.name();
+  info->enabled = service->IsExtensionEnabled(info->id);
+  info->offline_enabled = extension.offline_enabled();
+  info->version = extension.VersionString();
+  info->description = extension.description();
+  info->options_url = extension.options_url().possibly_invalid_spec();

[Aaron Boodman, 2012/08/09 13:17:19]

I think just spec() will do? Same thing below.

[mitchellwrosen, 2012/08/09 18:15:41]

Done.

+  info->homepage_url.reset(new std::string(
+      extension.GetHomepageURL().possibly_invalid_spec()));
+  info->may_disable = ExtensionSystem::Get(service->profile())->

[Aaron Boodman, 2012/08/09 13:17:19]

Pass ExtensionSystem* instead into this function. You can get ExtensionService*
from it.

[mitchellwrosen, 2012/08/09 18:15:41]

Done.

+      management_policy()->UserMayModifySettings(&extension, NULL);
+  info->is_app = extension.is_app();
+
+  if (!info->enabled) {

[Aaron Boodman, 2012/08/09 13:17:19]

Flip this check to put the positive branch first.

[mitchellwrosen, 2012/08/09 18:15:41]

Done.

     extensions::ExtensionPrefs* prefs = service->extension_prefs();
-    bool permissions_escalated =
-        prefs->DidExtensionEscalatePermissions(extension.id());
-    const char* reason = permissions_escalated ?
-        keys::kDisabledReasonPermissionsIncrease : keys::kDisabledReasonUnknown;
-    info->SetString(keys::kDisabledReasonKey, reason);
+    if (prefs->DidExtensionEscalatePermissions(extension.id())) {
+      info->disabled_reason =
+          ExtensionInfo::DISABLED_REASON_PERMISSIONS_INCREASE;
+    } else {
+      info->disabled_reason = ExtensionInfo::DISABLED_REASON_UNKNOWN;
+    }
+  } else {
+    info->disabled_reason = ExtensionInfo::DISABLED_REASON_NONE;
   }
 
-  if (!extension.update_url().is_empty())
-    info->SetString(keys::kUpdateUrlKey,
-                    extension.update_url().possibly_invalid_spec());
-  if (extension.is_app())
-    info->SetString(keys::kAppLaunchUrlKey,
-                    extension.GetFullLaunchURL().possibly_invalid_spec());
+  if (!extension.update_url().is_empty()) {
+    info->update_url.reset(new std::string(
+        extension.update_url().possibly_invalid_spec()));
+  }
+
+  if (extension.is_app()) {
+    info->app_launch_url.reset(new std::string(
+        extension.GetFullLaunchURL().possibly_invalid_spec()));
+  }
 
   const ExtensionIconSet::IconMap& icons = extension.icons().map();
   if (!icons.empty()) {
-    ListValue* icon_list = new ListValue();
+    info->icons.reset(new IconInfoList());
     std::map<ExtensionIconSet::Icons, std::string>::const_iterator icon_iter;
     for (icon_iter = icons.begin(); icon_iter != icons.end(); ++icon_iter) {
-      DictionaryValue* icon_info = new DictionaryValue();
-      ExtensionIconSet::Icons size = icon_iter->first;
+      IconInfo* icon_info = new IconInfo();
+      icon_info->size = icon_iter->first;
       GURL url = ExtensionIconSource::GetIconURL(
-          &extension, size, ExtensionIconSet::MATCH_EXACTLY, false, NULL);
-      icon_info->SetInteger(keys::kSizeKey, icon_iter->first);
-      icon_info->SetString(keys::kUrlKey, url.spec());
-      icon_list->Append(icon_info);
+          &extension,

[Aaron Boodman, 2012/08/09 13:17:19]

Put multiple arguments on one line, so long as the first arg of each line
aligns.

[mitchellwrosen, 2012/08/09 18:15:41]

Agh, that's how I did it, but was told by another reviewer that it's either all
on one line or one per line.

[Aaron Boodman, 2012/08/10 06:12:36]

Don't think so:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

+          icon_info->size,
+          ExtensionIconSet::MATCH_EXACTLY,
+          false,
+          NULL);
+      icon_info->url = url.spec();
+      info->icons->push_back(make_linked_ptr<IconInfo>(icon_info));
     }
-    info->Set(keys::kIconsKey, icon_list);
   }
 
   const std::set<std::string> perms =
       extension.GetActivePermissions()->GetAPIsAsStrings();
-  ListValue* permission_list = new ListValue();
   if (!perms.empty()) {
     std::set<std::string>::const_iterator perms_iter;
-    for (perms_iter = perms.begin(); perms_iter != perms.end(); ++perms_iter) {
-      StringValue* permission_name = new StringValue(*perms_iter);
-      permission_list->Append(permission_name);
-    }
+    for (perms_iter = perms.begin(); perms_iter != perms.end(); ++perms_iter)
+      info->permissions.push_back(*perms_iter);
   }
-  info->Set(keys::kPermissionsKey, permission_list);
 
-  ListValue* host_permission_list = new ListValue();
   if (!extension.is_hosted_app()) {
     // Skip host permissions for hosted apps.
     const URLPatternSet host_perms =
         extension.GetActivePermissions()->explicit_hosts();
     if (!host_perms.is_empty()) {
-      URLPatternSet::const_iterator host_perms_iter;
-      for (host_perms_iter = host_perms.begin();
-           host_perms_iter != host_perms.end();
-           ++host_perms_iter) {
-        StringValue* name = new StringValue(host_perms_iter->GetAsString());
-        host_permission_list->Append(name);
+      for (URLPatternSet::const_iterator iter = host_perms.begin();
+           iter != host_perms.end(); ++iter) {
+        info->host_permissions.push_back(iter->GetAsString());
       }
     }
   }
-  info->Set(keys::kHostPermissionsKey, host_permission_list);
 
-  std::string install_type = keys::kInstallTypeOther;
   switch (extension.location()) {
     case Extension::INTERNAL:
-      install_type = keys::kInstallTypeNormal;
+      info->install_type = ExtensionInfo::INSTALL_TYPE_NORMAL;
       break;
     case Extension::LOAD:
-      install_type = keys::kInstallTypeDevelopment;
+      info->install_type = ExtensionInfo::INSTALL_TYPE_DEVELOPMENT;
       break;
     case Extension::EXTERNAL_PREF:
     case Extension::EXTERNAL_REGISTRY:
     case Extension::EXTERNAL_PREF_DOWNLOAD:
-      install_type = keys::kInstallTypeSideload;
+      info->install_type = ExtensionInfo::INSTALL_TYPE_SIDELOAD;
       break;
     case Extension::EXTERNAL_POLICY_DOWNLOAD:
-      install_type = keys::kInstallTypeAdmin;
+      info->install_type = ExtensionInfo::INSTALL_TYPE_ADMIN;
       break;
     default:
-      install_type = keys::kInstallTypeOther;
+      info->install_type = ExtensionInfo::INSTALL_TYPE_OTHER;
+      break;
   }
-  info->SetString(keys::kInstallTypeKey, install_type);
 
-  return info;
+  return info.Pass();
 }
 
-static void AddExtensionInfo(ListValue* list,
-                             const ExtensionSet& extensions,
-                             ExtensionService* service) {
-  for (ExtensionSet::const_iterator i = extensions.begin();
-       i != extensions.end(); ++i) {
-    const Extension& extension = **i;
+static void AddExtensionInfo(const ExtensionSet& extensions,

[Aaron Boodman, 2012/08/09 13:17:19]

Same comment as above about anon namespace.

[mitchellwrosen, 2012/08/09 18:15:41]

Done.

+                             ExtensionService* service,
+                             ExtensionInfoList* extension_list) {
+  for (ExtensionSet::const_iterator iter = extensions.begin();
+       iter != extensions.end(); ++iter) {
+    const Extension& extension = **iter;
 
     if (extension.location() == Extension::COMPONENT)
       continue;  // Skip built-in extensions.
 
-    list->Append(CreateExtensionInfo(extension, service));
+    extension_list->push_back(make_linked_ptr<ExtensionInfo>(
+        CreateExtensionInfo(extension, service).release()));
   }
 }
 
 bool GetAllExtensionsFunction::RunImpl() {
-  ListValue* result = new ListValue();
-  SetResult(result);
+  ExtensionInfoList extensions;
 
-  AddExtensionInfo(result, *service()->extensions(), service());
-  AddExtensionInfo(result, *service()->disabled_extensions(), service());
+  AddExtensionInfo(*service()->extensions(), service(), &extensions);
+  AddExtensionInfo(*service()->disabled_extensions(), service(), &extensions);
 
+  results_ = GetAll::Results::Create(extensions);
   return true;
 }
 
 bool GetExtensionByIdFunction::RunImpl() {
-  std::string extension_id;
-  EXTENSION_FUNCTION_VALIDATE(args_->GetString(0, &extension_id));
-  const Extension* extension = service()->GetExtensionById(extension_id, true);
+  scoped_ptr<Get::Params> params(Get::Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params.get());
+
+  const Extension* extension = service()->GetExtensionById(params->id, true);
   if (!extension) {
     error_ = ExtensionErrorUtils::FormatErrorMessage(keys::kNoExtensionError,
-                                                     extension_id);
+                                                     params->id);
     return false;
   }
-  DictionaryValue* result = CreateExtensionInfo(*extension, service());
-  SetResult(result);
+
+  scoped_ptr<ExtensionInfo> info = CreateExtensionInfo(*extension, service());
+  results_ = Get::Results::Create(*info);
 
   return true;
 }
 
 bool GetPermissionWarningsByIdFunction::RunImpl() {
-  std::string ext_id;
-  EXTENSION_FUNCTION_VALIDATE(args_->GetString(0, &ext_id));
+  scoped_ptr<GetPermissionWarningsById::Params> params(
+      GetPermissionWarningsById::Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params.get());
 
-  const Extension* extension = service()->GetExtensionById(ext_id, true);
+  const Extension* extension = service()->GetExtensionById(params->id, true);
   if (!extension) {
     error_ = ExtensionErrorUtils::FormatErrorMessage(keys::kNoExtensionError,
-                                                     ext_id);
+                                                     params->id);
     return false;
   }
 
-  PermissionMessages warnings = extension->GetPermissionMessages();
-  ListValue* result = new ListValue();
-  for (PermissionMessages::const_iterator i = warnings.begin();
-       i < warnings.end(); ++i)
-    result->Append(Value::CreateStringValue(i->message()));
-  SetResult(result);
+  std::vector<std::string> warnings;
+  CreateWarningsList(extension, &warnings);
+  results_ = GetPermissionWarningsById::Results::Create(warnings);
   return true;
 }
 
@@ -317,11 +355,12 @@ class SafeManifestJSONParser : public UtilityProcessHostClient {
 }  // namespace
 
 bool GetPermissionWarningsByManifestFunction::RunImpl() {
-  std::string manifest_str;
-  EXTENSION_FUNCTION_VALIDATE(args_->GetString(0, &manifest_str));
+  scoped_ptr<GetPermissionWarningsByManifest::Params> params(
+      GetPermissionWarningsByManifest::Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params.get());
 
   scoped_refptr<SafeManifestJSONParser> parser =
-      new SafeManifestJSONParser(this, manifest_str);
+      new SafeManifestJSONParser(this, params->manifest_str);
   parser->Start();
 
   // Matched with a Release() in OnParseSuccess/Failure().
@@ -343,12 +382,9 @@ void GetPermissionWarningsByManifestFunction::OnParseSuccess(
     return;
   }
 
-  PermissionMessages warnings = extension->GetPermissionMessages();
-  ListValue* result = new ListValue();
-  for (PermissionMessages::const_iterator i = warnings.begin();
-       i < warnings.end(); ++i)
-    result->Append(Value::CreateStringValue(i->message()));
-  SetResult(result);
+  std::vector<std::string> warnings;
+  CreateWarningsList(extension, &warnings);
+  results_ = GetPermissionWarningsByManifest::Results::Create(warnings);
   SendResponse(true);
 
   // Matched with AddRef() in RunImpl().
@@ -365,17 +401,17 @@ void GetPermissionWarningsByManifestFunction::OnParseFailure(
 }
 
 bool LaunchAppFunction::RunImpl() {
-  std::string extension_id;
-  EXTENSION_FUNCTION_VALIDATE(args_->GetString(0, &extension_id));
-  const Extension* extension = service()->GetExtensionById(extension_id, true);
+  scoped_ptr<LaunchApp::Params> params(LaunchApp::Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params.get());
+  const Extension* extension = service()->GetExtensionById(params->id, true);
   if (!extension) {
     error_ = ExtensionErrorUtils::FormatErrorMessage(keys::kNoExtensionError,
-                                                     extension_id);
+                                                     params->id);
     return false;
   }
   if (!extension->is_app()) {
     error_ = ExtensionErrorUtils::FormatErrorMessage(keys::kNotAnAppError,
-                                                     extension_id);
+                                                     params->id);
     return false;
   }
 
@@ -402,9 +438,10 @@ SetEnabledFunction::~SetEnabledFunction() {
 }
 
 bool SetEnabledFunction::RunImpl() {
-  bool enable;
-  EXTENSION_FUNCTION_VALIDATE(args_->GetString(0, &extension_id_));
-  EXTENSION_FUNCTION_VALIDATE(args_->GetBoolean(1, &enable));
+  scoped_ptr<SetEnabled::Params> params(SetEnabled::Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params.get());
+
+  extension_id_ = params->id;
 
   const Extension* extension = service()->GetExtensionById(extension_id_, true);
   if (!extension) {
@@ -423,7 +460,7 @@ bool SetEnabledFunction::RunImpl() {
 
   bool currently_enabled = service()->IsExtensionEnabled(extension_id_);
 
-  if (!currently_enabled && enable) {
+  if (!currently_enabled && params->enabled) {
     extensions::ExtensionPrefs* prefs = service()->extension_prefs();
     if (prefs->DidExtensionEscalatePermissions(extension_id_)) {
       if (!user_gesture()) {
@@ -437,7 +474,7 @@ bool SetEnabledFunction::RunImpl() {
       return true;
     }
     service()->EnableExtension(extension_id_);
-  } else if (currently_enabled && !enable) {
+  } else if (currently_enabled && !params->enabled) {
     service()->DisableExtension(extension_id_, Extension::DISABLE_USER_ACTION);
   }
 
@@ -468,19 +505,14 @@ UninstallFunction::~UninstallFunction() {
 }
 
 bool UninstallFunction::RunImpl() {
-  bool show_confirm_dialog = false;
+  scoped_ptr<Uninstall::Params> params(Uninstall::Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params.get());
 
-  EXTENSION_FUNCTION_VALIDATE(args_->GetString(0, &extension_id_));
+  extension_id_ = params->id;
 
-  if (HasOptionalArgument(1)) {
-    DictionaryValue* options = NULL;
-    EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(1, &options));
-
-    if (options->HasKey(keys::kShowConfirmDialogKey)) {
-      EXTENSION_FUNCTION_VALIDATE(options->GetBoolean(
-          keys::kShowConfirmDialogKey, &show_confirm_dialog));
-    }
-  }
+  bool show_confirm_dialog = false;
+  if (params->options.get() && params->options->show_confirm_dialog.get())
+    show_confirm_dialog = *params->options->show_confirm_dialog;
 
   const Extension* extension = service()->GetExtensionById(extension_id_, true);
   if (!extension) {
@@ -592,10 +624,10 @@ void ExtensionManagementEventRouter::Observe(
       return;
   }
 
-  ListValue args;
+  std::string json = "";

[Aaron Boodman, 2012/08/09 13:17:19]

No need to initialize non-primitives.

[mitchellwrosen, 2012/08/09 18:15:41]

Done.

   if (event_name == events::kOnExtensionUninstalled) {
-    args.Append(Value::CreateStringValue(
-        content::Details<const extensions::Extension>(details).ptr()->id()));
+    json = OnUninstalled::ToJson(
+        content::Details<const Extension>(details).ptr()->id());
   } else {
     const Extension* extension = NULL;
     if (event_name == events::kOnExtensionDisabled) {
@@ -606,12 +638,12 @@ void ExtensionManagementEventRouter::Observe(
     }
     CHECK(extension);
     ExtensionService* service = profile->GetExtensionService();
-    args.Append(CreateExtensionInfo(*extension, service));
+    scoped_ptr<ExtensionInfo> info = CreateExtensionInfo(*extension, service);
+    // OnInstalled::ToJson, OnEnabled::ToJson, and OnDisabled::ToJson are all

[Aaron Boodman, 2012/08/09 13:17:19]

This is brittle - if one of these changed to be non-equal, the person writing
the change would not know about this dependency.

I prefer the extra code and just having another branch (or you can get fancy and
use a function pointer to call the right version of ToJson).

[mitchellwrosen, 2012/08/09 18:15:41]

Good point. Done.

+    // extensionally and intensionally equal, so pick one.
+    json = OnInstalled::ToJson(*info);
   }
 
-  std::string args_json;
-  base::JSONWriter::Write(&args, &args_json);
-
   profile->GetExtensionEventRouter()->DispatchEventToRenderers(
-      event_name, args_json, NULL, GURL(), extensions::EventFilteringInfo());
+      event_name, json, NULL, GURL(), extensions::EventFilteringInfo());
 }