[fsp] Extract common code to FileSystemProvidedInternalFunction.

chrome/browser/chromeos/extensions/file_system_provider/provider_function.cc

+// Copyright 2013 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/chromeos/extensions/file_system_provider/file_system_provider_api.h"
+
+#include "chrome/browser/chromeos/file_system_provider/provided_file_system_interface.h"
+#include "chrome/browser/chromeos/file_system_provider/request_manager.h"
+#include "chrome/browser/chromeos/file_system_provider/request_value.h"
+#include "chrome/browser/chromeos/file_system_provider/service.h"
+#include "chrome/common/extensions/api/file_system_provider_internal.h"
+
+using chromeos::file_system_provider::ProvidedFileSystemInterface;
+using chromeos::file_system_provider::RequestManager;
+using chromeos::file_system_provider::RequestValue;
+using chromeos::file_system_provider::Service;
+
+namespace extensions {
+
+const char kNotFoundErrorName[] = "NotFoundError";
+const char kSecurityErrorName[] = "SecurityError";
+
+const char kEmptyNameErrorMessage[] = "Empty display name is not allowed.";
+const char kMountFailedErrorMessage[] = "Mounting the file system failed.";
+const char kUnmountFailedErrorMessage[] = "Unmounting the file system failed.";
+const char kResponseFailedErrorMessage[] =
+    "Sending a response for the request failed.";
+
+base::DictionaryValue* CreateError(const std::string& name,
+                                   const std::string& message) {
+  base::DictionaryValue* error = new base::DictionaryValue();
+  error->SetString("name", name);
+  error->SetString("message", message);
+  return error;
+}
+
+base::File::Error ProviderErrorToFileError(
+    api::file_system_provider::ProviderError error) {
+  switch (error) {
+    case api::file_system_provider::PROVIDER_ERROR_OK:
+      return base::File::FILE_OK;
+    case api::file_system_provider::PROVIDER_ERROR_IN_USE:
+      return base::File::FILE_ERROR_IN_USE;
+    case api::file_system_provider::PROVIDER_ERROR_EXISTS:
+      return base::File::FILE_ERROR_EXISTS;
+    case api::file_system_provider::PROVIDER_ERROR_NOT_FOUND:
+      return base::File::FILE_ERROR_NOT_FOUND;
+    case api::file_system_provider::PROVIDER_ERROR_ACCESS_DENIED:
+      return base::File::FILE_ERROR_ACCESS_DENIED;
+    case api::file_system_provider::PROVIDER_ERROR_TOO_MANY_OPENED:
+      return base::File::FILE_ERROR_TOO_MANY_OPENED;
+    case api::file_system_provider::PROVIDER_ERROR_NO_MEMORY:
+      return base::File::FILE_ERROR_NO_MEMORY;
+    case api::file_system_provider::PROVIDER_ERROR_NO_SPACE:
+      return base::File::FILE_ERROR_NO_SPACE;
+    case api::file_system_provider::PROVIDER_ERROR_NOT_A_DIRECTORY:
+      return base::File::FILE_ERROR_NOT_A_DIRECTORY;
+    case api::file_system_provider::PROVIDER_ERROR_INVALID_OPERATION:
+      return base::File::FILE_ERROR_INVALID_OPERATION;
+    case api::file_system_provider::PROVIDER_ERROR_SECURITY:
+      return base::File::FILE_ERROR_SECURITY;
+    case api::file_system_provider::PROVIDER_ERROR_ABORT:
+      return base::File::FILE_ERROR_ABORT;
+    case api::file_system_provider::PROVIDER_ERROR_NOT_A_FILE:
+      return base::File::FILE_ERROR_NOT_A_FILE;
+    case api::file_system_provider::PROVIDER_ERROR_NOT_EMPTY:
+      return base::File::FILE_ERROR_NOT_EMPTY;
+    case api::file_system_provider::PROVIDER_ERROR_INVALID_URL:
+      return base::File::FILE_ERROR_INVALID_URL;
+    case api::file_system_provider::PROVIDER_ERROR_IO:
+      return base::File::FILE_ERROR_IO;
+    default:
+      NOTREACHED();

[hashimoto, 2014/05/01 09:20:12]

nit: How about removing "defualt" here and moving NOTREACHED to before the
return statement at the end of this function?
This way, when you add a new enum to ProviderError without updating this
function in future, compiler will raise an error here to let you know what to
update.

[mtomasz, 2014/05/02 01:26:22]

Done.

+  }
+  return base::File::FILE_ERROR_FAILED;
+}
+
+FileSystemProviderInternalFunction::FileSystemProviderInternalFunction()
+    : file_system_id_(0), request_id_(0), is_valid_(false) {
+}
+
+bool FileSystemProviderInternalFunction::Parse() {

[hashimoto, 2014/05/01 09:20:12]

nit: Our style guide says "Function declaration order should match function
definition order."
http://www.chromium.org/developers/coding-style/#Code_Formatting

[mtomasz, 2014/05/02 01:26:22]

Done.

+  if (!args_->GetInteger(0, &file_system_id_))

[hashimoto, 2014/05/01 09:20:12]

nit: !args_->GetInteger(0, &file_system_id_) && !args_->GetInteger(1,
&request_id_) ?

[mtomasz, 2014/05/02 01:26:22]

Done.

+    return false;
+
+  if (!args_->GetInteger(1, &request_id_))
+    return false;
+
+  Service* service = Service::Get(GetProfile());
+  DCHECK(service);
+  if (!service)
+    return false;
+
+  // Check if the file system exists. Note, that it may be gone if the api
+  // function involves any asynchronous operations.
+  ProvidedFileSystemInterface* file_system =
+      service->GetProvidedFileSystem(extension_id(), file_system_id_);
+  if (!file_system) {
+    SetErrorMessage(kNotFoundErrorName, kResponseFailedErrorMessage);
+    return false;
+  }
+
+  return true;
+}
+
+bool FileSystemProviderInternalFunction::RejectRequest(
+    base::File::Error error) {
+  DCHECK(is_valid_);
+  if (!is_valid_)
+    return false;
+
+  Service* service = Service::Get(GetProfile());
+  if (!service)
+    return false;
+
+  ProvidedFileSystemInterface* file_system =
+      service->GetProvidedFileSystem(extension_id(), file_system_id_);
+  if (!file_system) {
+    SetErrorMessage(kNotFoundErrorName, kResponseFailedErrorMessage);
+    return false;
+  }
+
+  RequestManager* request_manager = file_system->GetRequestManager();
+  DCHECK(request_manager);
+  if (!request_manager)
+    return false;
+
+  if (!request_manager->RejectRequest(request_id_, error)) {
+    SetErrorMessage(kSecurityErrorName, kResponseFailedErrorMessage);
+    return false;
+  }
+
+  return true;
+}
+
+bool FileSystemProviderInternalFunction::FulfillRequest(
+    scoped_ptr<RequestValue> value,
+    bool has_next) {
+  DCHECK(is_valid_);
+  if (!is_valid_)
+    return false;
+
+  Service* service = Service::Get(GetProfile());
+  if (!service)
+    return false;
+
+  ProvidedFileSystemInterface* file_system =
+      service->GetProvidedFileSystem(extension_id(), file_system_id_);
+  if (!file_system) {
+    SetErrorMessage(kNotFoundErrorName, kResponseFailedErrorMessage);
+    return false;
+  }
+
+  RequestManager* request_manager = file_system->GetRequestManager();
+  DCHECK(request_manager);
+  if (!request_manager)
+    return false;

[hashimoto, 2014/05/01 09:20:12]

You are repeating the same code to get RequestManager twice in RejectRequest and
FullfillRequest.
How about making this a separate function?

[mtomasz, 2014/05/02 01:26:22]

I simplified the code. Done.

+
+  if (!request_manager->FulfillRequest(request_id_, value.Pass(), has_next)) {
+    SetErrorMessage(kSecurityErrorName, kResponseFailedErrorMessage);
+    return false;
+  }
+
+  return true;
+}
+
+void FileSystemProviderInternalFunction::SetErrorMessage(
+    const std::string& name,
+    const std::string& message) {
+  base::ListValue* result = new base::ListValue();
+  result->Append(CreateError(kNotFoundErrorName, kResponseFailedErrorMessage));
+  SetResult(result);
+}
+
+bool FileSystemProviderInternalFunction::RunImpl() {
+  DCHECK(args_);
+  is_valid_ = Parse();

[hashimoto, 2014/05/01 09:20:12]

Why saving the result to is_valid_ here?
Do we still need to proceed to RunSync() when Parse() fails?

[mtomasz, 2014/05/02 01:26:22]

I wanted the macro EXTENSION_FUNCTION_VALIDATE to be always executed if the
request is malformed. However, for simplicity I changed as you suggested. Done.

+  SendResponse(RunSync());
+  return true;
+}
+
+}  // namespace extensions