Media Galleries API Picasa: Add file watch to invalidate database data on disk write.

chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc

Index: chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc
diff --git a/chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc b/chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc
index 0a5190517798a6f0441ebcfe8f7737b2d92f3df7..ad0eb127ea6e2bc00238e04a4d82b248fcd813a0 100644
--- a/chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc
+++ b/chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc
@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "base/file_util.h"
+#include "base/files/file_enumerator.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
@@ -175,6 +176,61 @@ void VerifyAlbumsImagesIndex(PicasaDataProvider* data_provider,
 
 }  // namespace
 
+class TestPicasaDataProvider : public PicasaDataProvider {
+ public:
+  explicit TestPicasaDataProvider(const base::FilePath& database_path,

[vandebo (ex-Chrome), 2013/08/29 23:19:22]

Yea, it seems like it wouldn't be too much trouble to follow the same directory
layout for fake data.

[tommycli, 2013/09/03 20:20:43]

Done.

+                                  const base::FilePath& temp_dir_path)
+      : PicasaDataProvider(database_path, temp_dir_path),
+        temp_dir_path_(temp_dir_path) {}
+
+  virtual ~TestPicasaDataProvider() {}
+
+  // Simulates the actual writing process of moving all the database files
+  // from the temporary directory to the database directory in a loop.
+  void MoveTempFilesToDatabase() {
+    DCHECK(MediaFileSystemBackend::CurrentlyOnMediaTaskRunnerThread());
+
+    base::FileEnumerator file_enumerator(
+        temp_dir_path_, false /* recursive */, base::FileEnumerator::FILES);
+
+    for (base::FilePath src_path = file_enumerator.Next(); !src_path.empty();
+         src_path = file_enumerator.Next()) {
+      ASSERT_TRUE(
+          base::Move(src_path, database_path_.Append(src_path.BaseName())));
+    }
+  }
+
+  void SetInvalidateCallback(const base::Closure& callback) {
+    DCHECK(!invalidate_callback_.get());
+    invalidate_callback_.reset(new base::Closure(callback));
+  }
+
+  virtual void InvalidateData() OVERRIDE {

[vandebo (ex-Chrome), 2013/08/29 23:19:22]

Should this be private?

[tommycli, 2013/09/03 20:20:43]

No. This is explicitly called by a few tests. (I couldn't figure out a way to
avoid a race between file watcher on the FILE thread and the utility process
reporting on the Media Task Runner thread).

+    PicasaDataProvider::InvalidateData();
+
+    if (invalidate_callback_.get()) {
+      invalidate_callback_->Run();
+      invalidate_callback_.reset();
+    }
+  }
+
+  // Methods are used in the browser test to tweak internal data for testing.
+  void SetDatabasePathForTesting(const base::FilePath& database_path) {

[vandebo (ex-Chrome), 2013/08/29 23:19:22]

Do you still need this?

[tommycli, 2013/09/03 20:20:43]

Done.

+    database_path_ = database_path;
+  }
+
+  void SetAlbumMapsForTesting(const AlbumMap& album_map,
+                              const AlbumMap& folder_map) {
+    album_map_ = album_map;
+    folder_map_ = folder_map;
+  }
+
+ private:
+  scoped_ptr<base::Closure> invalidate_callback_;

[vandebo (ex-Chrome), 2013/08/29 23:19:22]

You don't need a scoped_ptr for Closure, just take a copy - it has an internally
ref counted object iirc.

[tommycli, 2013/09/03 20:20:43]

Done.

+
+  base::FilePath temp_dir_path_;
+};
+
 class PicasaDataProviderTest : public InProcessBrowserTest {
  public:
   PicasaDataProviderTest() : test_helper_(kPicasaAlbumTableName) {}
@@ -226,7 +282,7 @@ class PicasaDataProviderTest : public InProcessBrowserTest {
 
   PmpTestHelper* test_helper() { return &test_helper_; }
 
-  PicasaDataProvider* data_provider() const {
+  TestPicasaDataProvider* data_provider() const {
     return picasa_data_provider_.get();
   }
 
@@ -235,18 +291,21 @@ class PicasaDataProviderTest : public InProcessBrowserTest {
     DCHECK(MediaFileSystemBackend::CurrentlyOnMediaTaskRunnerThread());
     ASSERT_TRUE(test_folder_1_.CreateUniqueTempDir());
     ASSERT_TRUE(test_folder_2_.CreateUniqueTempDir());
+    ASSERT_TRUE(database_dir_.CreateUniqueTempDir());
     ASSERT_TRUE(test_helper_.Init());
-    picasa_data_provider_.reset(
-        new PicasaDataProvider(test_helper_.GetTempDirPath()));
+    picasa_data_provider_.reset(new TestPicasaDataProvider(
+        database_dir_.path(), test_helper_.GetTempDirPath()));
   }
 
   virtual void StartTestOnMediaTaskRunner() {
     DCHECK(MediaFileSystemBackend::CurrentlyOnMediaTaskRunnerThread());
 
-    data_provider()->RefreshData(
-        RequestedDataType(),
-        base::Bind(&PicasaDataProviderTest::VerifyRefreshResults,
-                   base::Unretained(this)));
+    data_provider()->MoveTempFilesToDatabase();
+
+    data_provider()
+        ->RefreshData(RequestedDataType(),

[vandebo (ex-Chrome), 2013/08/29 23:19:22]

Don't change this formatting.

[tommycli, 2013/09/03 20:20:43]

Done.

+                      base::Bind(&PicasaDataProviderTest::VerifyRefreshResults,
+                                 base::Unretained(this)));
   }
 
   void DestructDataProviderThenQuit() {
@@ -258,9 +317,10 @@ class PicasaDataProviderTest : public InProcessBrowserTest {
 
   base::ScopedTempDir test_folder_1_;
   base::ScopedTempDir test_folder_2_;
+  base::ScopedTempDir database_dir_;
 
   PmpTestHelper test_helper_;
-  scoped_ptr<PicasaDataProvider> picasa_data_provider_;
+  scoped_ptr<TestPicasaDataProvider> picasa_data_provider_;
 
   base::Closure quit_closure_;
 
@@ -397,6 +457,8 @@ class PicasaDataProviderMultipleMixedCallbacksTest
   virtual void StartTestOnMediaTaskRunner() OVERRIDE {
     DCHECK(MediaFileSystemBackend::CurrentlyOnMediaTaskRunnerThread());
 
+    data_provider()->MoveTempFilesToDatabase();
+

[vandebo (ex-Chrome), 2013/08/29 23:19:22]

There's a race here - you want to wait until it's moved until you call
RefreshData.

[tommycli, 2013/09/03 20:20:43]

Move is synchronous I believe.

[vandebo (ex-Chrome), 2013/09/05 16:49:38]

The move is synchronous, but the invalidation triggered by the file watcher is
not.  In fact, you're probably always losing the race here (because invalidation
happens on the media task runner) but starting a refresh anyway because the data
provider is in the initial state, then while you're reading the data, you might
get the invalidation from this move, which will cause you to read the data
again.

Since invalidation isn't important for this test, you could avoid the race by
writing to the database direction (instead of the temp directory) for this test.
Otherwise you should use your SetInvalidateCallback method.

[tommycli, 2013/09/05 21:37:37]

Oh, I see what you're saying. I took your recommendation of writing to the
database dir by default. I think that fixes and simplifies things.

     data_provider()->RefreshData(
         PicasaDataProvider::LIST_OF_ALBUMS_AND_FOLDERS_DATA,
         base::Bind(&PicasaDataProviderMultipleMixedCallbacksTest::ListCallback,
@@ -430,52 +492,39 @@ IN_PROC_BROWSER_TEST_F(PicasaDataProviderMultipleMixedCallbacksTest,
   RunTest();
 }
 
-class PicasaDataProviderInvalidateSimpleTest : public PicasaDataProviderTest {
+class PicasaDataProviderFileWatcherInvalidateTest
+    : public PicasaDataProviderGetListTest {
  protected:
-  virtual void FirstListCallback(bool parse_success) {
+  virtual void ListCallback(bool parse_success) {
     ASSERT_FALSE(parse_success);
-    WriteTestAlbumTable(
-        test_helper(), test_folder_1_path(), test_folder_2_path());
 
-    // TODO(tommycli): Remove this line once database is under file watch.
-    data_provider()->InvalidateData();
-
-    // Have to post this, otherwise this will run the callback immediately.
-    MediaFileSystemBackend::MediaTaskRunner()->PostTask(
-        FROM_HERE,
+    // Validate the list after the file move triggers an invalidate.
+    data_provider()->SetInvalidateCallback(base::Bind(
+        &PicasaDataProvider::RefreshData,
+        base::Unretained(data_provider()),
+        RequestedDataType(),
         base::Bind(
-            &PicasaDataProvider::RefreshData,
-            base::Unretained(data_provider()),
-            RequestedDataType(),
-            base::Bind(
-                &PicasaDataProviderInvalidateSimpleTest::SecondListCallback,
-                base::Unretained(this))));
-  }
+            &PicasaDataProviderFileWatcherInvalidateTest::VerifyRefreshResults,
+            base::Unretained(this))));
 
-  virtual void SecondListCallback(bool parse_success) {
-    ASSERT_TRUE(parse_success);
-    VerifyAlbumTable(
-        data_provider(), test_folder_1_path(), test_folder_2_path());
-    TestDone();
-  }
-
-  virtual PicasaDataProvider::DataType RequestedDataType() const OVERRIDE {
-    return PicasaDataProvider::ALBUMS_IMAGES_DATA;
+    data_provider()->MoveTempFilesToDatabase();
   }
 
  private:
   virtual void StartTestOnMediaTaskRunner() OVERRIDE {
     DCHECK(MediaFileSystemBackend::CurrentlyOnMediaTaskRunnerThread());
 
+    // Refresh before moving album table to database dir, guaranteeing failure.
     data_provider()->RefreshData(
         RequestedDataType(),
-        base::Bind(&PicasaDataProviderInvalidateSimpleTest::FirstListCallback,
-                   base::Unretained(this)));
+        base::Bind(
+            &PicasaDataProviderFileWatcherInvalidateTest::ListCallback,
+            base::Unretained(this)));
   }
 };
 
-IN_PROC_BROWSER_TEST_F(PicasaDataProviderInvalidateSimpleTest,
-                       InvalidateSimpleTest) {
+IN_PROC_BROWSER_TEST_F(PicasaDataProviderFileWatcherInvalidateTest,
+                       FileWatcherInvalidateTest) {
   RunTest();
 }
 
@@ -514,7 +563,7 @@ class PicasaDataProviderInvalidateInflightAlbumsIndexerTest
     ASSERT_TRUE(parse_success);
 
     // Empty the album maps to guarantee that the first utility process will
-    // give incorrect results.
+    // fail to get the correct albums-images index.
     data_provider()->SetAlbumMapsForTesting(AlbumMap(), AlbumMap());
     data_provider()->RefreshData(
         PicasaDataProvider::ALBUMS_IMAGES_DATA,
@@ -532,6 +581,8 @@ class PicasaDataProviderInvalidateInflightAlbumsIndexerTest
   virtual void StartTestOnMediaTaskRunner() OVERRIDE {
     DCHECK(MediaFileSystemBackend::CurrentlyOnMediaTaskRunnerThread());
 
+    data_provider()->MoveTempFilesToDatabase();
+

[vandebo (ex-Chrome), 2013/08/29 23:19:22]

And here

[tommycli, 2013/09/03 20:20:43]

See above.

     data_provider()->RefreshData(
         PicasaDataProvider::LIST_OF_ALBUMS_AND_FOLDERS_DATA,
         base::Bind(&PicasaDataProviderInvalidateInflightAlbumsIndexerTest::