[fsp] Add requestUnmount() method together with the request manager.

chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc

+// Copyright 2014 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 <string>
+
+#include "base/bind.h"
+#include "base/files/file.h"
+#include "base/files/file_path.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/scoped_vector.h"
+#include "base/memory/weak_ptr.h"
+#include "base/values.h"
+#include "chrome/browser/chromeos/file_system_provider/request_manager.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace chromeos {
+namespace file_system_provider {
+namespace {
+
+static const std::string kExtensionId = "mbflcebpggnecokmikipoihdbecnjfoj";

[hashimoto, 2014/03/25 10:57:48]

nit: No need to have const. You are already in the unnamed namespace where these
variables have internal linkage.

[kinaba, 2014/03/25 11:14:51]

I think hashimoto meant no need for static.

std::string => char[] (as far as I understand objects in global scope is not
allowed in Chromium)

[mtomasz, 2014/03/26 00:52:34]

Done.

[mtomasz, 2014/03/26 00:52:34]

Done.

[hashimoto, 2014/03/26 01:07:56]

oops, right, indeed.

+static const int kFileSystemId = 1;
+static const std::string kFileSystemName = "Camera Pictures";
+static const base::FilePath::CharType kMountPath[] = FILE_PATH_LITERAL(
+    "/provided/mbflcebpggnecokmikipoihdbecnjfoj-1-testing_profile-hash");
+
+class EventLogger {
+ public:
+  class SuccessEvent {
+   public:
+    SuccessEvent(scoped_ptr<base::DictionaryValue> result, bool has_next)
+        : result_(result.Pass()), has_next_(has_next) {}
+    ~SuccessEvent() {};

[kinaba, 2014/03/25 11:14:51]

semicolon not necessary.

[mtomasz, 2014/03/26 00:52:34]

Done.

+
+    base::DictionaryValue* result() { return result_.get(); }
+    bool has_next() { return has_next_; }
+
+   private:
+    scoped_ptr<base::DictionaryValue> result_;
+    bool has_next_;
+  };
+
+  class ErrorEvent {
+   public:
+    explicit ErrorEvent(base::File::Error error) : error_(error) {}
+    ~ErrorEvent() {};

[kinaba, 2014/03/25 11:14:51]

Drop semicolon

[mtomasz, 2014/03/26 00:52:34]

Done.

+
+    base::File::Error error() { return error_; }
+
+   private:
+    base::File::Error error_;
+  };
+
+  EventLogger() : weak_ptr_factory_(this) {}
+  virtual ~EventLogger() {}
+
+  void OnSuccess(scoped_ptr<base::DictionaryValue> result, bool has_next) {
+    success_events_.push_back(new SuccessEvent(result.Pass(), has_next));
+  }
+
+  void OnError(base::File::Error error) {
+    error_events_.push_back(new ErrorEvent(error));
+  }
+
+  ScopedVector<SuccessEvent>& success_events() { return success_events_; }
+  ScopedVector<ErrorEvent>& error_events() { return error_events_; }
+
+  base::WeakPtr<EventLogger> GetWeakPtr() {
+    return weak_ptr_factory_.GetWeakPtr();
+  }
+
+ private:
+  ScopedVector<SuccessEvent> success_events_;
+  ScopedVector<ErrorEvent> error_events_;
+  base::WeakPtrFactory<EventLogger> weak_ptr_factory_;
+};
+
+}  // namespace
+
+class FileSystemProviderRequestManagerTest : public testing::Test {
+ protected:
+  FileSystemProviderRequestManagerTest() {}
+  virtual ~FileSystemProviderRequestManagerTest() {}
+
+  virtual void SetUp() OVERRIDE {
+    request_manager_.reset(new RequestManager());
+
+    // Configure the testing file system.
+    file_system = ProvidedFileSystem(kExtensionId,
+                                     kFileSystemId,
+                                     kFileSystemName,
+                                     base::FilePath(kMountPath));
+  }
+
+  scoped_ptr<RequestManager> request_manager_;
+  ProvidedFileSystem file_system;
+};
+
+TEST_F(FileSystemProviderRequestManagerTest, CreateAndFulFill) {
+  EventLogger logger;
+
+  int request_id = request_manager_->CreateRequest(
+      file_system,
+      base::Bind(&EventLogger::OnSuccess, logger.GetWeakPtr()),
+      base::Bind(&EventLogger::OnError, logger.GetWeakPtr()));
+
+  EXPECT_EQ(1, request_id);
+  EXPECT_EQ(0u, logger.success_events().size());
+  EXPECT_EQ(0u, logger.error_events().size());
+
+  scoped_ptr<base::DictionaryValue> response(new base::DictionaryValue());
+  const bool has_next = false;
+  response->SetString("path", "i-like-vanilla");
+
+  bool result = request_manager_->FulfillRequest(
+      file_system, request_id, response.Pass(), has_next);
+  EXPECT_TRUE(result);
+
+  // Validate if the callback has correct arguments.
+  ASSERT_EQ(1u, logger.success_events().size());
+  EXPECT_EQ(0u, logger.error_events().size());
+  EventLogger::SuccessEvent* event = logger.success_events()[0];
+  ASSERT_TRUE(event->result());
+  std::string response_test_string;
+  EXPECT_TRUE(event->result()->GetString("path", &response_test_string));
+  EXPECT_EQ("i-like-vanilla", response_test_string);
+  EXPECT_EQ(has_next, event->has_next());
+
+  // Confirm, that the request is removed. Basically, fulfilling again for the
+  // same request, should fail.
+  {
+    bool retry = request_manager_->FulfillRequest(
+        file_system, request_id, response.Pass(), has_next);
+    EXPECT_FALSE(retry);
+  }
+
+  // Rejecting should also fail.
+  {
+    bool retry = request_manager_->RejectRequest(
+        file_system, request_id, base::File::FILE_ERROR_FAILED);
+    EXPECT_FALSE(retry);
+  }
+}
+
+TEST_F(FileSystemProviderRequestManagerTest, CreateAndFulFill_WithHasNext) {
+  EventLogger logger;
+
+  int request_id = request_manager_->CreateRequest(
+      file_system,
+      base::Bind(&EventLogger::OnSuccess, logger.GetWeakPtr()),
+      base::Bind(&EventLogger::OnError, logger.GetWeakPtr()));
+
+  EXPECT_EQ(1, request_id);
+  EXPECT_EQ(0u, logger.success_events().size());
+  EXPECT_EQ(0u, logger.error_events().size());
+
+  scoped_ptr<base::DictionaryValue> response;
+  const bool has_next = true;
+
+  bool result = request_manager_->FulfillRequest(
+      file_system, request_id, response.Pass(), has_next);
+  EXPECT_TRUE(result);
+
+  // Validate if the callback has correct arguments.
+  ASSERT_EQ(1u, logger.success_events().size());
+  EXPECT_EQ(0u, logger.error_events().size());
+  EventLogger::SuccessEvent* event = logger.success_events()[0];
+  ASSERT_FALSE(event->result());

[hashimoto, 2014/03/25 10:57:48]

nit: I think this can be EXPECT.
Why don't you allow the test move forward and emit more information to make
debugging easier?

[mtomasz, 2014/03/26 00:52:34]

Done.

+  EXPECT_EQ(has_next, event->has_next());
+
+  // Confirm, that the request is not removed (since it has has_next == true).
+  // Basically, fulfilling again for the same request, should not fail.
+  {
+    bool new_has_next = false;
+    bool retry = request_manager_->FulfillRequest(
+        file_system, request_id, response.Pass(), new_has_next);
+    EXPECT_TRUE(retry);
+  }
+
+  // Since |new_has_next| is false, then the request should be removed. To check
+  // it, try to fulfill again, what should fail.
+  {
+    bool new_has_next = false;
+    bool retry = request_manager_->FulfillRequest(
+        file_system, request_id, response.Pass(), new_has_next);
+    EXPECT_FALSE(retry);
+  }
+}
+
+TEST_F(FileSystemProviderRequestManagerTest, CreateAndReject) {
+  EventLogger logger;
+
+  int request_id = request_manager_->CreateRequest(
+      file_system,
+      base::Bind(&EventLogger::OnSuccess, logger.GetWeakPtr()),
+      base::Bind(&EventLogger::OnError, logger.GetWeakPtr()));
+
+  EXPECT_EQ(1, request_id);
+  EXPECT_EQ(0u, logger.success_events().size());
+  EXPECT_EQ(0u, logger.error_events().size());
+
+  base::File::Error error = base::File::FILE_ERROR_NO_MEMORY;
+  bool result = request_manager_->RejectRequest(file_system, request_id, error);
+  EXPECT_TRUE(result);
+
+  // Validate if the callback has correct arguments.
+  ASSERT_EQ(1u, logger.error_events().size());
+  EXPECT_EQ(0u, logger.success_events().size());
+  EventLogger::ErrorEvent* event = logger.error_events()[0];
+  EXPECT_EQ(error, event->error());
+
+  // Confirm, that the request is removed. Basically, fulfilling again for the
+  // same request, should fail.
+  {
+    scoped_ptr<base::DictionaryValue> response;
+    bool has_next = false;
+    bool retry = request_manager_->FulfillRequest(
+        file_system, request_id, response.Pass(), has_next);
+    EXPECT_FALSE(retry);
+  }
+
+  // Rejecting should also fail.
+  {
+    bool retry =
+        request_manager_->RejectRequest(file_system, request_id, error);
+    EXPECT_FALSE(retry);
+  }
+}
+
+TEST_F(FileSystemProviderRequestManagerTest,
+       CreateAndFulfillWithWrongRequestId) {
+  EventLogger logger;
+
+  int request_id = request_manager_->CreateRequest(
+      file_system,
+      base::Bind(&EventLogger::OnSuccess, logger.GetWeakPtr()),
+      base::Bind(&EventLogger::OnError, logger.GetWeakPtr()));
+
+  EXPECT_EQ(1, request_id);
+  EXPECT_EQ(0u, logger.success_events().size());
+  EXPECT_EQ(0u, logger.error_events().size());
+
+  base::File::Error error = base::File::FILE_ERROR_NO_MEMORY;
+  bool result =
+      request_manager_->RejectRequest(file_system, request_id + 1, error);
+  EXPECT_FALSE(result);
+
+  // Callbacks should not be called.
+  ASSERT_EQ(0u, logger.error_events().size());

[hashimoto, 2014/03/25 10:57:48]

nit: This can be EXPECT?

[mtomasz, 2014/03/26 00:52:34]

Done.

+  EXPECT_EQ(0u, logger.success_events().size());
+
+  // Confirm, that the request hasn't been removed, by rejecting it correctly.
+  {
+    bool retry =
+        request_manager_->RejectRequest(file_system, request_id, error);
+    EXPECT_TRUE(retry);
+  }
+}
+
+TEST_F(FileSystemProviderRequestManagerTest,
+       CreateAndRejectWithWrongRequestId) {
+  EventLogger logger;
+
+  int request_id = request_manager_->CreateRequest(
+      file_system,
+      base::Bind(&EventLogger::OnSuccess, logger.GetWeakPtr()),
+      base::Bind(&EventLogger::OnError, logger.GetWeakPtr()));
+
+  EXPECT_EQ(1, request_id);
+  EXPECT_EQ(0u, logger.success_events().size());
+  EXPECT_EQ(0u, logger.error_events().size());
+
+  base::File::Error error = base::File::FILE_ERROR_NO_MEMORY;
+  bool result =
+      request_manager_->RejectRequest(file_system, request_id + 1, error);
+  EXPECT_FALSE(result);
+
+  // Callbacks should not be called.
+  ASSERT_EQ(0u, logger.error_events().size());

[hashimoto, 2014/03/25 10:57:48]

nit: This can be EXPECT?

[mtomasz, 2014/03/26 00:52:34]

Done.

+  EXPECT_EQ(0u, logger.success_events().size());
+
+  // Confirm, that the request hasn't been removed, by rejecting it correctly.
+  {
+    bool retry =
+        request_manager_->RejectRequest(file_system, request_id, error);
+    EXPECT_TRUE(retry);
+  }
+}
+
+TEST_F(FileSystemProviderRequestManagerTest,
+       CreateAndFulfillWithUnownedRequestId) {
+  EventLogger logger;
+
+  int request_id = request_manager_->CreateRequest(
+      file_system,
+      base::Bind(&EventLogger::OnSuccess, logger.GetWeakPtr()),
+      base::Bind(&EventLogger::OnError, logger.GetWeakPtr()));
+  EXPECT_EQ(1, request_id);
+
+  // Create another file system, which has just a different fiel system id
+  // (1 -> 2).
+  ProvidedFileSystem another_file_system(
+      kExtensionId,
+      2,  // file_system_id
+      "Music",
+      base::FilePath(

[kinaba, 2014/03/25 11:14:51]

base::FilePath::FromUTF8Unsafe (or FILE_PATH_LITERAL)

[mtomasz, 2014/03/26 00:52:34]

Done.

+          "/provided/mbflcebpggnecokmikipoihdbecnjfoj-2-testing_profile-hash"));
+
+  scoped_ptr<base::DictionaryValue> response;
+  const bool has_next = false;
+
+  bool result = request_manager_->FulfillRequest(
+      another_file_system, request_id, response.Pass(), has_next);
+  EXPECT_FALSE(result);
+
+  // Callbacks should not be called.
+  ASSERT_EQ(0u, logger.error_events().size());

[hashimoto, 2014/03/25 10:57:48]

nit: This can be EXPECT?

[mtomasz, 2014/03/26 00:52:34]

Done.

+  EXPECT_EQ(0u, logger.success_events().size());
+
+  // Confirm, that the request hasn't been removed, by fulfilling it again, but
+  // with a correct file system.
+  {
+    bool retry = request_manager_->FulfillRequest(
+        file_system, request_id, response.Pass(), has_next);
+    EXPECT_TRUE(retry);
+  }
+}
+
+TEST_F(FileSystemProviderRequestManagerTest, UniqueIds) {
+  EventLogger logger;
+
+  int first_request_id = request_manager_->CreateRequest(
+      file_system,
+      base::Bind(&EventLogger::OnSuccess, logger.GetWeakPtr()),
+      base::Bind(&EventLogger::OnError, logger.GetWeakPtr()));
+
+  int second_request_id = request_manager_->CreateRequest(
+      file_system,
+      base::Bind(&EventLogger::OnSuccess, logger.GetWeakPtr()),
+      base::Bind(&EventLogger::OnError, logger.GetWeakPtr()));
+
+  EXPECT_EQ(1, first_request_id);
+  EXPECT_EQ(2, second_request_id);
+}
+
+}  // namespace file_system_provider
+}  // namespace chromeos