[fsp] Refactor handling operations.

chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc

Index: chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc
diff --git a/chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc b/chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc
index 413826aabf5023f783d389a397c4719e8ad56d66..09591b6d974c14ddc8732ab1f5823894cc2b6a15 100644
--- a/chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc
+++ b/chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc
@@ -12,8 +12,8 @@
 #include "base/memory/scoped_vector.h"
 #include "base/memory/weak_ptr.h"
 #include "base/run_loop.h"
-#include "base/values.h"
 #include "chrome/browser/chromeos/file_system_provider/request_manager.h"
+#include "chrome/browser/chromeos/file_system_provider/request_value.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -24,42 +24,68 @@ namespace {
 // Logs calls of the success and error callbacks on requests.
 class EventLogger {
  public:
-  class SuccessEvent {
+  class ExecuteEvent {
    public:
-    SuccessEvent(scoped_ptr<base::DictionaryValue> result, bool has_next)
-        : result_(result.Pass()), has_next_(has_next) {}
-    ~SuccessEvent() {}
+    explicit ExecuteEvent(int request_id) : request_id_(request_id) {}
+    virtual ~ExecuteEvent() {}
+
+    int request_id() { return request_id_; }
 
-    base::DictionaryValue* result() { return result_.get(); }
+   private:
+    int request_id_;
+  };
+
+  class SuccessEvent {
+   public:
+    SuccessEvent(int request_id, scoped_ptr<RequestValue> result, bool has_next)
+        : request_id_(request_id),
+          result_(result.Pass()),
+          has_next_(has_next) {}
+    virtual ~SuccessEvent() {}
+
+    int request_id() { return request_id_; }
+    RequestValue* result() { return result_.get(); }
     bool has_next() { return has_next_; }
 
    private:
-    scoped_ptr<base::DictionaryValue> result_;
+    int request_id_;
+    scoped_ptr<RequestValue> result_;
     bool has_next_;
   };
 
   class ErrorEvent {
    public:
-    explicit ErrorEvent(base::File::Error error) : error_(error) {}
-    ~ErrorEvent() {}
+    explicit ErrorEvent(int request_id, base::File::Error error)
+        : request_id_(request_id), error_(error) {}
+    virtual ~ErrorEvent() {}
 
+    int request_id() { return request_id_; }
     base::File::Error error() { return error_; }
 
    private:
+    int request_id_;
     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 OnExecute(int request_id) {
+    execute_events_.push_back(new ExecuteEvent(request_id));
   }
 
-  void OnError(base::File::Error error) {
-    error_events_.push_back(new ErrorEvent(error));
+  void OnSuccess(int request_id,
+                 scoped_ptr<RequestValue> result,
+                 bool has_next) {
+    success_events_.push_back(
+        new SuccessEvent(request_id, result.Pass(), has_next));
   }
 
+  void OnError(int request_id, base::File::Error error) {
+    error_events_.push_back(new ErrorEvent(request_id, error));
+  }
+
+  ScopedVector<ExecuteEvent>& execute_events() { return execute_events_; }
   ScopedVector<SuccessEvent>& success_events() { return success_events_; }
   ScopedVector<ErrorEvent>& error_events() { return error_events_; }
 
@@ -68,9 +94,51 @@ class EventLogger {
   }
 
  private:
+  ScopedVector<ExecuteEvent> execute_events_;
   ScopedVector<SuccessEvent> success_events_;
   ScopedVector<ErrorEvent> error_events_;
   base::WeakPtrFactory<EventLogger> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(EventLogger);
+};
+
+// Fake handler, which forwards callbacks to the logger. The handler is owned
+// by a request manager, however the logger is owned by tests.
+class FakeHandler : public RequestManager::HandlerInterface {
+ public:
+  // The handler can outlive the passed logger, so using a weak pointer. The
+  // |execute_reply| value will be returned for the Execute() call.
+  explicit FakeHandler(base::WeakPtr<EventLogger> logger, bool execute_reply)

[kinaba, 2014/04/25 06:35:06]

nit: no need for explicit since it takes more than one parameter.

[mtomasz, 2014/04/28 00:42:47]

Done.

+      : logger_(logger), execute_reply_(execute_reply) {}
+
+  // RequestManager::Handler overrides.
+  virtual bool Execute(int request_id) OVERRIDE {
+    if (logger_.get())
+      logger_->OnExecute(request_id);
+
+    return execute_reply_;
+  }
+
+  // RequestManager::Handler overrides.
+  virtual void OnSuccess(int request_id,
+                         scoped_ptr<RequestValue> result,
+                         bool has_next) OVERRIDE {
+    if (logger_.get())
+      logger_->OnSuccess(request_id, result.Pass(), has_next);
+  }
+
+  // RequestManager::Handler overrides.
+  virtual void OnError(int request_id, base::File::Error error) OVERRIDE {
+    if (logger_.get())
+      logger_->OnError(request_id, error);
+  }
+
+  virtual ~FakeHandler() {}
+
+ private:
+  base::WeakPtr<EventLogger> logger_;
+  bool execute_reply_;
+  DISALLOW_COPY_AND_ASSIGN(FakeHandler);
 };
 
 }  // namespace
@@ -91,17 +159,17 @@ class FileSystemProviderRequestManagerTest : public testing::Test {
 TEST_F(FileSystemProviderRequestManagerTest, CreateAndFulFill) {
   EventLogger logger;
 
-  int request_id = request_manager_->CreateRequest(
-      base::Bind(&EventLogger::OnSuccess, logger.GetWeakPtr()),
-      base::Bind(&EventLogger::OnError, logger.GetWeakPtr()));
+  const int request_id = request_manager_->CreateRequest(
+      make_scoped_ptr<RequestManager::HandlerInterface>(
+          new FakeHandler(logger.GetWeakPtr(), true /* execute_reply */)));
 
   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());
+  scoped_ptr<RequestValue> response(
+      RequestValue::CreateForTesting("i-like-vanilla"));
   const bool has_next = false;
-  response->SetString("path", "i-like-vanilla");
 
   bool result =
       request_manager_->FulfillRequest(request_id, response.Pass(), has_next);
@@ -112,15 +180,15 @@ TEST_F(FileSystemProviderRequestManagerTest, CreateAndFulFill) {
   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);
+  std::string* response_test_string = event->result()->get_testing_params();
+  ASSERT_TRUE(response_test_string);
+  EXPECT_EQ("i-like-vanilla", *response_test_string);
   EXPECT_FALSE(event->has_next());
 
   // Confirm, that the request is removed. Basically, fulfilling again for the
   // same request, should fail.
   {
-    scoped_ptr<base::DictionaryValue> response;
+    scoped_ptr<RequestValue> response;
     bool retry =
         request_manager_->FulfillRequest(request_id, response.Pass(), has_next);
     EXPECT_FALSE(retry);
@@ -137,15 +205,15 @@ TEST_F(FileSystemProviderRequestManagerTest, CreateAndFulFill) {
 TEST_F(FileSystemProviderRequestManagerTest, CreateAndFulFill_WithHasNext) {
   EventLogger logger;
 
-  int request_id = request_manager_->CreateRequest(
-      base::Bind(&EventLogger::OnSuccess, logger.GetWeakPtr()),
-      base::Bind(&EventLogger::OnError, logger.GetWeakPtr()));
+  const int request_id = request_manager_->CreateRequest(
+      make_scoped_ptr<RequestManager::HandlerInterface>(
+          new FakeHandler(logger.GetWeakPtr(), true /* execute_reply */)));
 
   EXPECT_EQ(1, request_id);
   EXPECT_EQ(0u, logger.success_events().size());
   EXPECT_EQ(0u, logger.error_events().size());
 
-  scoped_ptr<base::DictionaryValue> response;
+  scoped_ptr<RequestValue> response;
   const bool has_next = true;
 
   bool result =
@@ -181,9 +249,9 @@ TEST_F(FileSystemProviderRequestManagerTest, CreateAndFulFill_WithHasNext) {
 TEST_F(FileSystemProviderRequestManagerTest, CreateAndReject) {
   EventLogger logger;
 
-  int request_id = request_manager_->CreateRequest(
-      base::Bind(&EventLogger::OnSuccess, logger.GetWeakPtr()),
-      base::Bind(&EventLogger::OnError, logger.GetWeakPtr()));
+  const int request_id = request_manager_->CreateRequest(
+      make_scoped_ptr<RequestManager::HandlerInterface>(
+          new FakeHandler(logger.GetWeakPtr(), true /* execute_reply */)));
 
   EXPECT_EQ(1, request_id);
   EXPECT_EQ(0u, logger.success_events().size());
@@ -202,7 +270,7 @@ TEST_F(FileSystemProviderRequestManagerTest, CreateAndReject) {
   // Confirm, that the request is removed. Basically, fulfilling again for the
   // same request, should fail.
   {
-    scoped_ptr<base::DictionaryValue> response;
+    scoped_ptr<RequestValue> response;
     bool has_next = false;
     bool retry =
         request_manager_->FulfillRequest(request_id, response.Pass(), has_next);
@@ -220,9 +288,9 @@ TEST_F(FileSystemProviderRequestManagerTest,
        CreateAndFulfillWithWrongRequestId) {
   EventLogger logger;
 
-  int request_id = request_manager_->CreateRequest(
-      base::Bind(&EventLogger::OnSuccess, logger.GetWeakPtr()),
-      base::Bind(&EventLogger::OnError, logger.GetWeakPtr()));
+  const int request_id = request_manager_->CreateRequest(
+      make_scoped_ptr<RequestManager::HandlerInterface>(
+          new FakeHandler(logger.GetWeakPtr(), true /* execute_reply */)));
 
   EXPECT_EQ(1, request_id);
   EXPECT_EQ(0u, logger.success_events().size());
@@ -247,9 +315,9 @@ TEST_F(FileSystemProviderRequestManagerTest,
        CreateAndRejectWithWrongRequestId) {
   EventLogger logger;
 
-  int request_id = request_manager_->CreateRequest(
-      base::Bind(&EventLogger::OnSuccess, logger.GetWeakPtr()),
-      base::Bind(&EventLogger::OnError, logger.GetWeakPtr()));
+  const int request_id = request_manager_->CreateRequest(
+      make_scoped_ptr<RequestManager::HandlerInterface>(
+          new FakeHandler(logger.GetWeakPtr(), true /* execute_reply */)));
 
   EXPECT_EQ(1, request_id);
   EXPECT_EQ(0u, logger.success_events().size());
@@ -273,13 +341,13 @@ TEST_F(FileSystemProviderRequestManagerTest,
 TEST_F(FileSystemProviderRequestManagerTest, UniqueIds) {
   EventLogger logger;
 
-  int first_request_id = request_manager_->CreateRequest(
-      base::Bind(&EventLogger::OnSuccess, logger.GetWeakPtr()),
-      base::Bind(&EventLogger::OnError, logger.GetWeakPtr()));
+  const int first_request_id = request_manager_->CreateRequest(
+      make_scoped_ptr<RequestManager::HandlerInterface>(
+          new FakeHandler(logger.GetWeakPtr(), true /* execute_reply */)));
 
-  int second_request_id = request_manager_->CreateRequest(
-      base::Bind(&EventLogger::OnSuccess, logger.GetWeakPtr()),
-      base::Bind(&EventLogger::OnError, logger.GetWeakPtr()));
+  const int second_request_id = request_manager_->CreateRequest(
+      make_scoped_ptr<RequestManager::HandlerInterface>(
+          new FakeHandler(logger.GetWeakPtr(), true /* execute_reply */)));
 
   EXPECT_EQ(1, first_request_id);
   EXPECT_EQ(2, second_request_id);
@@ -290,9 +358,9 @@ TEST_F(FileSystemProviderRequestManagerTest, AbortOnDestroy) {
 
   {
     RequestManager request_manager;
-    int request_id = request_manager.CreateRequest(
-        base::Bind(&EventLogger::OnSuccess, logger.GetWeakPtr()),
-        base::Bind(&EventLogger::OnError, logger.GetWeakPtr()));
+    const int request_id = request_manager.CreateRequest(
+        make_scoped_ptr<RequestManager::HandlerInterface>(
+            new FakeHandler(logger.GetWeakPtr(), true /* execute_reply */)));
 
     EXPECT_EQ(1, request_id);
     EXPECT_EQ(0u, logger.success_events().size());
@@ -300,8 +368,8 @@ TEST_F(FileSystemProviderRequestManagerTest, AbortOnDestroy) {
   }
 
   // All active requests should be aborted in the destructor of RequestManager.
-  EventLogger::ErrorEvent* event = logger.error_events()[0];
   ASSERT_EQ(1u, logger.error_events().size());
+  EventLogger::ErrorEvent* event = logger.error_events()[0];
   EXPECT_EQ(base::File::FILE_ERROR_ABORT, event->error());
 
   EXPECT_EQ(0u, logger.success_events().size());
@@ -312,9 +380,9 @@ TEST_F(FileSystemProviderRequestManagerTest, AbortOnTimeout) {
   base::RunLoop run_loop;
 
   request_manager_->SetTimeoutForTests(base::TimeDelta::FromSeconds(0));
-  int request_id = request_manager_->CreateRequest(
-      base::Bind(&EventLogger::OnSuccess, logger.GetWeakPtr()),
-      base::Bind(&EventLogger::OnError, logger.GetWeakPtr()));
+  const int request_id = request_manager_->CreateRequest(
+      make_scoped_ptr<RequestManager::HandlerInterface>(
+          new FakeHandler(logger.GetWeakPtr(), true /* execute_reply */)));
   EXPECT_LT(0, request_id);
 
   // Wait until the request is timeouted.