Propagate more information about ValueStore errors to callers, notably an

chrome/browser/extensions/api/storage/storage_api.cc

Index: chrome/browser/extensions/api/storage/storage_api.cc
diff --git a/chrome/browser/extensions/api/storage/storage_api.cc b/chrome/browser/extensions/api/storage/storage_api.cc
index f9c683106fbd119022b4e2211b4222ef665a15e9..3af4a346d87a29be190400816d134dd89cfbd6ef 100644
--- a/chrome/browser/extensions/api/storage/storage_api.cc
+++ b/chrome/browser/extensions/api/storage/storage_api.cc
@@ -21,15 +21,6 @@ namespace extensions {
 
 using content::BrowserThread;
 
-namespace {
-const char kUnsupportedArgumentType[] = "Unsupported argument type";
-const char kInvalidNamespaceErrorMessage[] =
-    "\"%s\" is not available in this instance of Chrome";
-const char kManagedNamespaceDisabledErrorMessage[] =
-    "\"managed\" is disabled. Use \"--%s\" to enable it.";
-const char kStorageErrorMessage[] = "Storage error";
-}  // namespace
-
 // SettingsFunction
 
 SettingsFunction::SettingsFunction()
@@ -41,8 +32,8 @@ bool SettingsFunction::ShouldSkipQuotaLimiting() const {
   // Only apply quota if this is for sync storage.
   std::string settings_namespace_string;
   if (!args_->GetString(0, &settings_namespace_string)) {
-    // This is an error but it will be caught in RunImpl(), there is no
-    // mechanism to signify an error from this function.
+    // This should be EXTENSION_FUNCTION_VALIDATE(false) but there is no way
+    // to signify that from this function. It will be caught in RunImpl().
     return false;
   }
   return settings_namespace_string != "sync";
@@ -60,8 +51,9 @@ bool SettingsFunction::RunImpl() {
   SettingsFrontend* frontend =
       profile()->GetExtensionService()->settings_frontend();
   if (!frontend->IsStorageEnabled(settings_namespace_)) {
-    error_ = base::StringPrintf(kInvalidNamespaceErrorMessage,
-                                settings_namespace_string.c_str());
+    error_ = base::StringPrintf(
+        "\"%s\" is not available in this instance of Chrome",
+        settings_namespace_string.c_str());
     return false;
   }
 
@@ -81,19 +73,21 @@ void SettingsFunction::AsyncRunWithStorage(ValueStore* storage) {
       base::Bind(&SettingsFunction::SendResponse, this, success));
 }
 
-bool SettingsFunction::UseReadResult(ValueStore::ReadResult result) {
-  if (result->HasError()) {
-    error_ = result->error();
+bool SettingsFunction::UseReadResult(ValueStore::ReadResult read_result) {
+  if (read_result->HasError()) {
+    error_ = read_result->error().message;
     return false;
   }
 
-  SetResult(result->settings().release());
+  base::DictionaryValue* result = new base::DictionaryValue();
+  result->Swap(&read_result->settings());
+  SetResult(result);

[Matt Perry, 2013/09/06 20:07:30]

You have a PassError - how about a PassSettings?

[not at google - send to devlin, 2013/09/06 21:44:28]

I haven't need it yet - mostly because DictionaryValue has a swap() method,
which Error doesn't have. I can add it for consistency if you like.

[Matt Perry, 2013/09/06 21:50:37]

Not a big deal, but it does seem idiomatically clearer.

[not at google - send to devlin, 2013/09/10 00:21:32]

Done.

   return true;
 }
 
 bool SettingsFunction::UseWriteResult(ValueStore::WriteResult result) {
   if (result->HasError()) {
-    error_ = result->error();
+    error_ = result->error().message;
     return false;
   }
 
@@ -192,14 +186,14 @@ bool StorageStorageAreaGetFunction::RunWithStorage(ValueStore* storage) {
       }
 
       base::DictionaryValue* with_default_values = as_dict->DeepCopy();
-      with_default_values->MergeDictionary(result->settings().get());
+      with_default_values->MergeDictionary(&result->settings());
       return UseReadResult(
-          ValueStore::MakeReadResult(with_default_values));
+          ValueStore::MakeReadResult(make_scoped_ptr(with_default_values)));
     }
 
     default:
-      return UseReadResult(
-          ValueStore::MakeReadResult(kUnsupportedArgumentType));
+      EXTENSION_FUNCTION_VALIDATE(false);
+      return false;
   }
 }
 
@@ -231,7 +225,7 @@ bool StorageStorageAreaGetBytesInUseFunction::RunWithStorage(
     }
 
     default:
-      error_ = kUnsupportedArgumentType;
+      EXTENSION_FUNCTION_VALIDATE(false);
       return false;
   }
 
@@ -269,8 +263,8 @@ bool StorageStorageAreaRemoveFunction::RunWithStorage(ValueStore* storage) {
     }
 
     default:
-      return UseWriteResult(
-          ValueStore::MakeWriteResult(kUnsupportedArgumentType));
+      EXTENSION_FUNCTION_VALIDATE(false);
+      return false;
   };
 }