Skip to content

Commit

Permalink
Introduce ExtensionFunction::RunImplTypesafe and SendResponseTypesafe,
Browse files Browse the repository at this point in the history 
new type-safe ways of responding to extension requests.

Use them in the Storage API as a proof of concept.

R=rockot@chromium.org
BUG=365732, 366282

Review URL: https://codereview.chromium.org/249713004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@266020 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
kalman@chromium.org committed Apr 24, 2014
1 parent cb3d560 commit f4e972d
Show file tree
Hide file tree
Showing 6 changed files with 271 additions and 71 deletions.
96 changes: 52 additions & 44 deletions extensions/browser/api/storage/storage_api.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,58 +32,61 @@ bool SettingsFunction::ShouldSkipQuotaLimiting() const {
std::string settings_namespace_string;
if (!args_->GetString(0, &settings_namespace_string)) {
// This should be EXTENSION_FUNCTION_VALIDATE(false) but there is no way
// to signify that from this function. It will be caught in RunImpl().
// to signify that from this function. It will be caught in
// RunImplTypesafe().
return false;
}
return settings_namespace_string != "sync";
}

bool SettingsFunction::RunImpl() {
ExtensionFunction::ResponseAction SettingsFunction::RunImplTypesafe() {
std::string settings_namespace_string;
EXTENSION_FUNCTION_VALIDATE(args_->GetString(0, &settings_namespace_string));
EXTENSION_FUNCTION_VALIDATE_TYPESAFE(
args_->GetString(0, &settings_namespace_string));
args_->Remove(0, NULL);
settings_namespace_ =
settings_namespace::FromString(settings_namespace_string);
EXTENSION_FUNCTION_VALIDATE(
settings_namespace_ != settings_namespace::INVALID);
EXTENSION_FUNCTION_VALIDATE_TYPESAFE(settings_namespace_ !=
settings_namespace::INVALID);

StorageFrontend* frontend = StorageFrontend::Get(browser_context());
if (!frontend->IsStorageEnabled(settings_namespace_)) {
error_ = base::StringPrintf(
"\"%s\" is not available in this instance of Chrome",
settings_namespace_string.c_str());
return false;
return RespondNow(Error(
base::StringPrintf("\"%s\" is not available in this instance of Chrome",
settings_namespace_string.c_str())));
}

observers_ = frontend->GetObservers();
frontend->RunWithStorage(
GetExtension(),
settings_namespace_,
base::Bind(&SettingsFunction::AsyncRunWithStorage, this));
return true;
return RespondLater();
}

void SettingsFunction::AsyncRunWithStorage(ValueStore* storage) {
bool success = RunWithStorage(storage);
BrowserThread::PostTask(
BrowserThread::UI,
FROM_HERE,
base::Bind(&SettingsFunction::SendResponse, this, success));
ResponseValue response = RunWithStorage(storage);
BrowserThread::PostTask(BrowserThread::UI,
FROM_HERE,
base::Bind(&SettingsFunction::SendResponseTypesafe,
this,
base::Passed(&response)));
}

bool SettingsFunction::UseReadResult(ValueStore::ReadResult result,
ValueStore* storage) {
ExtensionFunction::ResponseValue SettingsFunction::UseReadResult(
ValueStore::ReadResult result,
ValueStore* storage) {
if (result->HasError())
return HandleError(result->error(), storage);

base::DictionaryValue* dict = new base::DictionaryValue();
dict->Swap(&result->settings());
SetResult(dict);
return true;
return SingleArgument(dict);
}

bool SettingsFunction::UseWriteResult(ValueStore::WriteResult result,
ValueStore* storage) {
ExtensionFunction::ResponseValue SettingsFunction::UseWriteResult(
ValueStore::WriteResult result,
ValueStore* storage) {
if (result->HasError())
return HandleError(result->error(), storage);

Expand All @@ -95,11 +98,12 @@ bool SettingsFunction::UseWriteResult(ValueStore::WriteResult result,
ValueStoreChange::ToJson(result->changes()));
}

return true;
return NoArguments();
}

bool SettingsFunction::HandleError(const ValueStore::Error& error,
ValueStore* storage) {
ExtensionFunction::ResponseValue SettingsFunction::HandleError(
const ValueStore::Error& error,
ValueStore* storage) {
// If the method failed due to corruption, and we haven't tried to fix it, we
// can try to restore the storage and re-run it. Otherwise, the method has
// failed.
Expand All @@ -117,8 +121,7 @@ bool SettingsFunction::HandleError(const ValueStore::Error& error,
return RunWithStorage(storage);
}

error_ = error.message;
return false;
return Error(error.message);
}

// Concrete settings functions
Expand Down Expand Up @@ -174,9 +177,11 @@ void GetModificationQuotaLimitHeuristics(QuotaLimitHeuristics* heuristics) {

} // namespace

bool StorageStorageAreaGetFunction::RunWithStorage(ValueStore* storage) {
ExtensionFunction::ResponseValue StorageStorageAreaGetFunction::RunWithStorage(
ValueStore* storage) {
base::Value* input = NULL;
EXTENSION_FUNCTION_VALIDATE(args_->Get(0, &input));
if (!args_->Get(0, &input))
return BadMessage();

switch (input->GetType()) {
case base::Value::TYPE_NULL:
Expand Down Expand Up @@ -211,15 +216,15 @@ bool StorageStorageAreaGetFunction::RunWithStorage(ValueStore* storage) {
}

default:
EXTENSION_FUNCTION_VALIDATE(false);
return false;
return BadMessage();
}
}

bool StorageStorageAreaGetBytesInUseFunction::RunWithStorage(
ValueStore* storage) {
ExtensionFunction::ResponseValue
StorageStorageAreaGetBytesInUseFunction::RunWithStorage(ValueStore* storage) {
base::Value* input = NULL;
EXTENSION_FUNCTION_VALIDATE(args_->Get(0, &input));
if (!args_->Get(0, &input))
return BadMessage();

size_t bytes_in_use = 0;

Expand All @@ -244,17 +249,18 @@ bool StorageStorageAreaGetBytesInUseFunction::RunWithStorage(
}

default:
EXTENSION_FUNCTION_VALIDATE(false);
return false;
return BadMessage();
}

SetResult(new base::FundamentalValue(static_cast<int>(bytes_in_use)));
return true;
return SingleArgument(
new base::FundamentalValue(static_cast<int>(bytes_in_use)));
}

bool StorageStorageAreaSetFunction::RunWithStorage(ValueStore* storage) {
ExtensionFunction::ResponseValue StorageStorageAreaSetFunction::RunWithStorage(
ValueStore* storage) {
base::DictionaryValue* input = NULL;
EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(0, &input));
if (!args_->GetDictionary(0, &input))
return BadMessage();
return UseWriteResult(storage->Set(ValueStore::DEFAULTS, *input), storage);
}

Expand All @@ -263,9 +269,11 @@ void StorageStorageAreaSetFunction::GetQuotaLimitHeuristics(
GetModificationQuotaLimitHeuristics(heuristics);
}

bool StorageStorageAreaRemoveFunction::RunWithStorage(ValueStore* storage) {
ExtensionFunction::ResponseValue
StorageStorageAreaRemoveFunction::RunWithStorage(ValueStore* storage) {
base::Value* input = NULL;
EXTENSION_FUNCTION_VALIDATE(args_->Get(0, &input));
if (!args_->Get(0, &input))
return BadMessage();

switch (input->GetType()) {
case base::Value::TYPE_STRING: {
Expand All @@ -282,8 +290,7 @@ bool StorageStorageAreaRemoveFunction::RunWithStorage(ValueStore* storage) {
}

default:
EXTENSION_FUNCTION_VALIDATE(false);
return false;
return BadMessage();
};
}

Expand All @@ -292,7 +299,8 @@ void StorageStorageAreaRemoveFunction::GetQuotaLimitHeuristics(
GetModificationQuotaLimitHeuristics(heuristics);
}

bool StorageStorageAreaClearFunction::RunWithStorage(ValueStore* storage) {
ExtensionFunction::ResponseValue
StorageStorageAreaClearFunction::RunWithStorage(ValueStore* storage) {
return UseWriteResult(storage->Clear(), storage);
}

Expand Down
27 changes: 14 additions & 13 deletions extensions/browser/api/storage/storage_api.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,21 +22,20 @@ class SettingsFunction : public UIThreadExtensionFunction {

// ExtensionFunction:
virtual bool ShouldSkipQuotaLimiting() const OVERRIDE;
virtual bool RunImpl() OVERRIDE;
virtual ResponseAction RunImplTypesafe() OVERRIDE;

// Extension settings function implementations should do their work here.
// The StorageFrontend makes sure this is posted to the appropriate thread.
// Implementations should fill in args themselves, though (like RunImpl)
// may return false to imply failure.
virtual bool RunWithStorage(ValueStore* storage) = 0;
virtual ResponseValue RunWithStorage(ValueStore* storage) = 0;

// Handles the |result| of a read function.
// - If the result succeeded, this will set |result_| and return.
// - If |result| failed with a ValueStore::CORRUPTION error, this will call
// RestoreStorageAndRetry(), and return that result.
// - If the |result| failed with a different error, this will set |error_|
// and return.
bool UseReadResult(ValueStore::ReadResult result, ValueStore* storage);
ResponseValue UseReadResult(ValueStore::ReadResult result,
ValueStore* storage);

// Handles the |result| of a write function.
// - If the result succeeded, this will set |result_| and return.
Expand All @@ -45,10 +44,11 @@ class SettingsFunction : public UIThreadExtensionFunction {
// - If the |result| failed with a different error, this will set |error_|
// and return.
// This will also send out a change notification, if appropriate.
bool UseWriteResult(ValueStore::WriteResult result, ValueStore* storage);
ResponseValue UseWriteResult(ValueStore::WriteResult result,
ValueStore* storage);

private:
// Called via PostTask from RunImpl. Calls RunWithStorage and then
// Called via PostTask from RunImplTypesafe. Calls RunWithStorage and then
// SendResponse with its success value.
void AsyncRunWithStorage(ValueStore* storage);

Expand All @@ -57,7 +57,8 @@ class SettingsFunction : public UIThreadExtensionFunction {
// If the storage cannot be restored or was due to some other error, then sets
// error and returns. This also sets the |tried_restoring_storage_| flag to
// ensure we don't enter a loop.
bool HandleError(const ValueStore::Error& error, ValueStore* storage);
ResponseValue HandleError(const ValueStore::Error& error,
ValueStore* storage);

// The settings namespace the call was for. For example, SYNC if the API
// call was chrome.settings.experimental.sync..., LOCAL if .local, etc.
Expand All @@ -79,7 +80,7 @@ class StorageStorageAreaGetFunction : public SettingsFunction {
virtual ~StorageStorageAreaGetFunction() {}

// SettingsFunction:
virtual bool RunWithStorage(ValueStore* storage) OVERRIDE;
virtual ResponseValue RunWithStorage(ValueStore* storage) OVERRIDE;
};

class StorageStorageAreaSetFunction : public SettingsFunction {
Expand All @@ -90,7 +91,7 @@ class StorageStorageAreaSetFunction : public SettingsFunction {
virtual ~StorageStorageAreaSetFunction() {}

// SettingsFunction:
virtual bool RunWithStorage(ValueStore* storage) OVERRIDE;
virtual ResponseValue RunWithStorage(ValueStore* storage) OVERRIDE;

// ExtensionFunction:
virtual void GetQuotaLimitHeuristics(
Expand All @@ -105,7 +106,7 @@ class StorageStorageAreaRemoveFunction : public SettingsFunction {
virtual ~StorageStorageAreaRemoveFunction() {}

// SettingsFunction:
virtual bool RunWithStorage(ValueStore* storage) OVERRIDE;
virtual ResponseValue RunWithStorage(ValueStore* storage) OVERRIDE;

// ExtensionFunction:
virtual void GetQuotaLimitHeuristics(
Expand All @@ -120,7 +121,7 @@ class StorageStorageAreaClearFunction : public SettingsFunction {
virtual ~StorageStorageAreaClearFunction() {}

// SettingsFunction:
virtual bool RunWithStorage(ValueStore* storage) OVERRIDE;
virtual ResponseValue RunWithStorage(ValueStore* storage) OVERRIDE;

// ExtensionFunction:
virtual void GetQuotaLimitHeuristics(
Expand All @@ -135,7 +136,7 @@ class StorageStorageAreaGetBytesInUseFunction : public SettingsFunction {
virtual ~StorageStorageAreaGetBytesInUseFunction() {}

// SettingsFunction:
virtual bool RunWithStorage(ValueStore* storage) OVERRIDE;
virtual ResponseValue RunWithStorage(ValueStore* storage) OVERRIDE;
};

} // namespace extensions
Expand Down
2 changes: 2 additions & 0 deletions extensions/browser/api/storage/storage_api_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@ class StorageApiUnittest : public ExtensionApiUnittest {
scoped_ptr<base::Value> result = RunFunctionAndReturnValue(
new StorageStorageAreaGetFunction(),
base::StringPrintf("[\"local\", \"%s\"]", key.c_str()));
if (!result.get())
return testing::AssertionFailure() << "No result";
base::DictionaryValue* dict = NULL;
if (!result->GetAsDictionary(&dict))
return testing::AssertionFailure() << result << " was not a dictionary.";
Expand Down
Loading

0 comments on commit f4e972d

Please sign in to comment.