[SystemInfo API] Rewrite storage info provider using storage monitor impl.

chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc

Index: chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc
diff --git a/chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc b/chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc
index a22573521c66c0824eeafb83954cf34c6c2f9b49..78583d9fe5e00dc9daa5a6892680e736d5dd8e2a 100644
--- a/chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc
+++ b/chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc
@@ -7,6 +7,7 @@
 #include "base/message_loop.h"
 #include "base/stl_util.h"
 #include "chrome/browser/extensions/api/system_info_storage/storage_info_provider.h"
+#include "chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.h"
 #include "chrome/browser/storage_monitor/test_storage_monitor.h"
 #include "content/public/test/test_browser_thread.h"
 #include "content/public/test/test_utils.h"
@@ -18,6 +19,7 @@ namespace extensions {
 using api::experimental_system_info_storage::ParseStorageUnitType;
 using api::experimental_system_info_storage::StorageUnitInfo;
 using api::experimental_system_info_storage::StorageUnitType;
+using base::MessageLoop;
 using chrome::test::TestStorageMonitor;
 using content::BrowserThread;
 using content::RunAllPendingInMessageLoop;
@@ -25,20 +27,17 @@ using content::RunMessageLoop;
 using testing::Return;
 using testing::_;
 
-struct TestUnitInfo {
-  std::string id;
-  std::string type;
-  double capacity;
-  double available_capacity;
-  // The change step of free space.
-  int change_step;
-};
-
+// Available capacity in TestUnitInfo should minus the detla change step once

[Greg Billock, 2013/06/24 16:55:38]

"minus" --> "subtract"
"delta"

+// for onAvailableCapacityChanged event.

[Greg Billock, 2013/06/24 16:55:38]

"for every onAvailableCapacityChanged event."

+// In the onAvailableCapacityChanged event implementation(StorageInfoProvider::

[Greg Billock, 2013/06/24 16:55:38]

I didn't follow this. Is it a TODO to eliminate the call? Or are you saying the
override eliminates this? If the latter, I don't think you need to add a comment
here about it. Perhaps in that override itself.

[Haojian Wu, 2013/06/26 03:22:40]

I have adjusted the code of
TestStorageInfoProvider::GetStorageFreeSpace(hongbo@, please review this
function). Now there is no need to subtract the delta change step. Remove the
comment.

+// AddWatchedStorageOnBlockingPool), GetStorageFreeSpace in
+// TestStorageInfoProvider will be invoked to check whether origin mount_path is
+// valid. We should eliminate this function call for one time.
 const struct TestUnitInfo kTestingData[] = {
-  {"C:", systeminfo::kStorageTypeUnknown, 1000, 10, 0},
-  {"d:", systeminfo::kStorageTypeRemovable, 2000, 10, 1 },
-  {"/home", systeminfo::kStorageTypeFixed, 3000, 10, 2},
-  {"/", systeminfo::kStorageTypeRemovable, 4000, 10, 3}
+  {"device:001", "C:", systeminfo::kStorageTypeUnknown, 1000, 10-0, 0},
+  {"device:002", "d:", systeminfo::kStorageTypeRemovable, 2000, 10-1, 1},
+  {"device:003", "/home", systeminfo::kStorageTypeFixed, 3000, 10-2, 2},
+  {"device:004", "/", systeminfo::kStorageTypeRemovable, 4000, 10-3, 3}
 };
 
 // The watching interval for unit test is 1 milliseconds.
@@ -46,136 +45,62 @@ const size_t kWatchingIntervalMs = 1u;
 // The number of times of checking watched storages.
 const int kCheckTimes = 10;
 
-class MockStorageObserver : public StorageInfoObserver {
+class MockStorageObserver : public StorageFreeSpaceObserver {
  public:
   MockStorageObserver() {}
   virtual ~MockStorageObserver() {}
 
-  MOCK_METHOD3(OnStorageFreeSpaceChanged, void(const std::string&,
+  MOCK_METHOD3(OnFreeSpaceChanged, void(const std::string&,
      double, double));
 
  private:
   DISALLOW_COPY_AND_ASSIGN(MockStorageObserver);
 };
 
+const int kMaxChangeTimes = 10;

[Greg Billock, 2013/06/24 16:55:38]

How about "kMaxSpaceChanges"

[Haojian Wu, 2013/06/26 03:22:40]

Done.

+
 // A testing observer used to provide the statistics of how many times
 // that the storage free space has been changed and check the change against
 // our expectation.
-class TestStorageObserver : public StorageInfoObserver {
+class TestStorageObserver : public StorageFreeSpaceObserver {
  public:
-  TestStorageObserver() {
+  TestStorageObserver() : change_times_(0) {
     for (size_t i = 0; i < arraysize(kTestingData); ++i)
       testing_data_.push_back(kTestingData[i]);
   }
 
   virtual ~TestStorageObserver() {}
 
-  virtual void OnStorageFreeSpaceChanged(const std::string& id,
-                                         double old_value,
-                                         double new_value) OVERRIDE {
+  virtual void OnFreeSpaceChanged(const std::string& id,
+                                  double old_value,
+                                  double new_value) OVERRIDE {
     // The observer is added on UI thread, so the callback should be also
     // called on UI thread.
     ASSERT_TRUE(BrowserThread::CurrentlyOn(BrowserThread::UI));
-    for (size_t i = 0; i < testing_data_.size(); ++i) {
-      if (testing_data_[i].id == id) {
-        EXPECT_DOUBLE_EQ(old_value, testing_data_[i].available_capacity);
-        EXPECT_DOUBLE_EQ(new_value, testing_data_[i].available_capacity +
-                  testing_data_[i].change_step);
-        // Increase the available capacity with the change step for comparison
-        // next time.
-        testing_data_[i].available_capacity += testing_data_[i].change_step;
-        ++free_space_change_times_[id];
-        return;
+    size_t i = 0;
+    for (; i < testing_data_.size(); ++i) {
+      if (testing_data_[i].device_id == id) {
+        EXPECT_DOUBLE_EQ(new_value-old_value, testing_data_[i].change_step);
+        ++change_times_;
+        break;
       }
     }
-    EXPECT_TRUE(false);
-  }
+    // Post a quit task to UI thread for test result verfication.
+    if (change_times_ >= kMaxChangeTimes) {
+      BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+          MessageLoop::QuitClosure());
+      change_times_ = 0;

[Greg Billock, 2013/06/24 16:55:38]

Should this be here? How about an ASSERT that change_times_ <= kMaxChangeTimes

[Haojian Wu, 2013/06/26 03:22:40]

From my point of view, it should be here. If change_times >= kMaxChangeTimes, we
should quit the message loop(post a QuitClosure) and give UI thread a chance to
verify the result. But the QuitClosure task is not processed synchronously.
OnFreeSpaceChanged function may be called after post QuitClosure to UI thread.

+    }
 
-  // Returns the number of change times for the given storage |id|.
-  int GetFreeSpaceChangeTimes(const std::string& id) {
-    if (ContainsKey(free_space_change_times_, id))
-      return free_space_change_times_[id];
-    return 0;
+    ASSERT_TRUE(i != testing_data_.size());
   }
 
  private:
   // A copy of |kTestingData|.
   std::vector<TestUnitInfo> testing_data_;
-  // Mapping of storage id and the times of free space has been changed.
-  std::map<std::string, int> free_space_change_times_;
-};
-
-class TestStorageInfoProvider : public StorageInfoProvider {
- public:
-  TestStorageInfoProvider();
-
- private:
-  virtual ~TestStorageInfoProvider();
-
-  virtual bool QueryInfo(StorageInfo* info) OVERRIDE;
-  virtual bool QueryUnitInfo(const std::string& id,
-                             StorageUnitInfo* info) OVERRIDE;
-
-  // Called each time CheckWatchedStoragesOnBlockingPool is finished.
-  virtual void OnCheckWatchedStoragesFinishedForTesting() OVERRIDE;
-
-  // The testing data maintained on the blocking pool.
-  std::vector<TestUnitInfo> testing_data_;
-  // The number of times for checking watched storage.
-  int checking_times_;
+  int change_times_;
 };
 
-//
-// TestStorageInfoProvider Impl.
-//
-TestStorageInfoProvider::TestStorageInfoProvider(): checking_times_(0) {
-  for (size_t i = 0; i < arraysize(kTestingData); ++i)
-    testing_data_.push_back(kTestingData[i]);
-  SetWatchingIntervalForTesting(kWatchingIntervalMs);
-}
-
-TestStorageInfoProvider::~TestStorageInfoProvider() {}
-
-bool TestStorageInfoProvider::QueryInfo(StorageInfo* info) {
-  info->clear();
-
-  for (size_t i = 0; i < testing_data_.size(); ++i) {
-    linked_ptr<StorageUnitInfo> unit(new StorageUnitInfo());
-    QueryUnitInfo(testing_data_[i].id, unit.get());
-    info->push_back(unit);
-  }
-  return true;
-}
-
-bool TestStorageInfoProvider::QueryUnitInfo(
-    const std::string& id, StorageUnitInfo* info) {
-  for (size_t i = 0; i < testing_data_.size(); ++i) {
-    if (testing_data_[i].id == id) {
-      info->id = testing_data_[i].id;
-      info->type = ParseStorageUnitType(testing_data_[i].type);
-      info->capacity = testing_data_[i].capacity;
-      info->available_capacity = testing_data_[i].available_capacity;
-      // Increase the available capacity with a fixed change step.
-      testing_data_[i].available_capacity += testing_data_[i].change_step;
-      return true;
-    }
-  }
-  return false;
-}
-
-void TestStorageInfoProvider::OnCheckWatchedStoragesFinishedForTesting() {
-  ++checking_times_;
-  if (checking_times_ < kCheckTimes)
-    return;
-  checking_times_ = 0;
-  // Once the number of checking times reaches up to kCheckTimes, we need to
-  // quit the message loop to given UI thread a chance to verify the results.
-  // Note the QuitClosure is actually bound to QuitCurrentWhenIdle, it means
-  // that the UI thread wil continue to process pending messages util idle.
-  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-      base::MessageLoop::QuitClosure());
-}
-
 class StorageInfoProviderTest : public testing::Test {
  public:
   StorageInfoProviderTest();
@@ -207,7 +132,8 @@ StorageInfoProviderTest::~StorageInfoProviderTest() {
 void StorageInfoProviderTest::SetUp() {
   ASSERT_TRUE(BrowserThread::CurrentlyOn(BrowserThread::UI));
   storage_test_notifications_.reset(new TestStorageMonitor);
-  storage_info_provider_= new TestStorageInfoProvider();
+  storage_info_provider_= new TestStorageInfoProvider(kTestingData,
+                                                      arraysize(kTestingData));
 }
 
 void StorageInfoProviderTest::TearDown() {
@@ -228,13 +154,17 @@ TEST_F(StorageInfoProviderTest, WatchingNoChangedStorage) {
   // Case 1: watching a storage that the free space is not changed.
   MockStorageObserver observer;
   storage_info_provider_->AddObserver(&observer);
-  storage_info_provider_->StartWatching(kTestingData[0].id);
-  EXPECT_CALL(observer, OnStorageFreeSpaceChanged(kTestingData[0].id,  _,  _))
+  storage_info_provider_->StartWatching(kTestingData[0].device_id);
+  EXPECT_CALL(observer, OnFreeSpaceChanged(kTestingData[0].device_id,  _,  _))
       .Times(0);
+  // Quit the message loop even if no free space change happens after 500ms

[Greg Billock, 2013/06/24 16:55:38]

This should never happen right?

If the test breaks, we want it to hang and be killed by the machinery.

[Hongbo Min, 2013/06/25 01:55:07]

Our expectation is no free space change happens. This is a way to quit message,
otherwise, the test will hang although it is in our expectation.

+  // interval.
+  MessageLoop::current()->PostDelayedTask(FROM_HERE,
+      MessageLoop::QuitClosure(), base::TimeDelta::FromMilliseconds(500));
   RunLoopAndFlushBlockingPool();
 
   storage_info_provider_->RemoveObserver(&observer);
-  storage_info_provider_->StopWatching(kTestingData[0].id);
+  storage_info_provider_->StopWatching(kTestingData[0].device_id);
   RunAllPendingAndFlushBlockingPool();
 }
 
@@ -242,23 +172,10 @@ TEST_F(StorageInfoProviderTest, WatchingOneStorage) {
   // Case 2: only watching one storage.
   TestStorageObserver observer;
   storage_info_provider_->AddObserver(&observer);
-  storage_info_provider_->StartWatching(kTestingData[1].id);
+  storage_info_provider_->StartWatching(kTestingData[1].device_id);
   RunLoopAndFlushBlockingPool();
 
-  // The times of free space change is at least |kCheckTimes|, it is not easy
-  // to anticiapte the accurate number since it is still possible for blocking
-  // pool to run the pending checking task after completing |kCheckTimes|
-  // checking tasks and posting Quit to the message loop of UI thread. The
-  // observer guarantees that the free space is changed with the expected
-  // delta value.
-  EXPECT_GE(observer.GetFreeSpaceChangeTimes(kTestingData[1].id),
-            kCheckTimes);
-  // Other storages should not be changed. The first two entries are skipped.
-  for (size_t i = 2; i < arraysize(kTestingData); ++i) {
-    EXPECT_EQ(0, observer.GetFreeSpaceChangeTimes(kTestingData[i].id));
-  }
-
-  storage_info_provider_->StopWatching(kTestingData[1].id);
+  storage_info_provider_->StopWatching(kTestingData[1].device_id);
   // Give a chance to run StopWatching task on the blocking pool.
   RunAllPendingAndFlushBlockingPool();
 
@@ -266,7 +183,7 @@ TEST_F(StorageInfoProviderTest, WatchingOneStorage) {
   storage_info_provider_->AddObserver(&mock_observer);
   // The watched storage won't get free space change notification.
   EXPECT_CALL(mock_observer,
-      OnStorageFreeSpaceChanged(kTestingData[1].id, _, _)).Times(0);
+      OnFreeSpaceChanged(kTestingData[1].location, _, _)).Times(0);
   RunAllPendingAndFlushBlockingPool();
 
   storage_info_provider_->RemoveObserver(&observer);
@@ -280,37 +197,29 @@ TEST_F(StorageInfoProviderTest, WatchingMultipleStorages) {
   storage_info_provider_->AddObserver(&observer);
 
   for (size_t k = 1; k < arraysize(kTestingData); ++k) {
-    storage_info_provider_->StartWatching(kTestingData[k].id);
+    storage_info_provider_->StartWatching(kTestingData[k].device_id);
   }
   RunLoopAndFlushBlockingPool();
 
-  // Right now let's verify the results.
-  for (size_t k = 1; k < arraysize(kTestingData); ++k) {
-    // See the above comments about the reason why the times of free space
-    // changes is at least kCheckTimes.
-    EXPECT_GE(observer.GetFreeSpaceChangeTimes(kTestingData[k].id),
-              kCheckTimes);
-  }
-
   // Stop watching the first storage.
-  storage_info_provider_->StopWatching(kTestingData[1].id);
+  storage_info_provider_->StopWatching(kTestingData[1].device_id);
   RunAllPendingAndFlushBlockingPool();
 
   MockStorageObserver mock_observer;
   storage_info_provider_->AddObserver(&mock_observer);
   for (size_t k = 2; k < arraysize(kTestingData); ++k) {
     EXPECT_CALL(mock_observer,
-        OnStorageFreeSpaceChanged(kTestingData[k].id,  _, _))
+        OnFreeSpaceChanged(kTestingData[k].device_id,  _, _))
         .WillRepeatedly(Return());
   }
 
   // After stopping watching, the observer won't get change notification.
   EXPECT_CALL(mock_observer,
-      OnStorageFreeSpaceChanged(kTestingData[1].id, _, _)).Times(0);
+      OnFreeSpaceChanged(kTestingData[1].device_id, _, _)).Times(0);
   RunLoopAndFlushBlockingPool();
 
   for (size_t k = 1; k < arraysize(kTestingData); ++k) {
-    storage_info_provider_->StopWatching(kTestingData[k].id);
+    storage_info_provider_->StopWatching(kTestingData[k].device_id);
   }
   RunAllPendingAndFlushBlockingPool();
   storage_info_provider_->RemoveObserver(&observer);
@@ -324,7 +233,7 @@ TEST_F(StorageInfoProviderTest, WatchingInvalidStorage) {
   storage_info_provider_->AddObserver(&mock_observer);
   storage_info_provider_->StartWatching(invalid_id);
   EXPECT_CALL(mock_observer,
-      OnStorageFreeSpaceChanged(invalid_id, _, _)).Times(0);
+      OnFreeSpaceChanged(invalid_id, _, _)).Times(0);
   RunAllPendingAndFlushBlockingPool();
   storage_info_provider_->RemoveObserver(&mock_observer);
 }