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

 // 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 "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"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "chrome/browser/media_galleries/fileapi/media_file_system_backend.h"
 #include "chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h"
 #include "chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.h"
 #include "chrome/common/media_galleries/picasa_types.h"
 #include "chrome/common/media_galleries/pmp_test_helper.h"
@@ skipping 152 matching lines @@
   EXPECT_EQ(test_folder_1_path.AppendASCII("InBoth.jpg"),
             (*album_2_images)["InBoth.jpg"]);
   EXPECT_NE(album_2_images->end(),
             album_2_images->find("InSecondAlbumOnly.jpg"));
   EXPECT_EQ(test_folder_1_path.AppendASCII("InSecondAlbumOnly.jpg"),
             (*album_2_images)["InSecondAlbumOnly.jpg"]);
 }
 
 }  // 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) {}
   virtual ~PicasaDataProviderTest() {}
 
  protected:
   // Runs on the MediaTaskRunner and designed to be overridden by subclasses.
   virtual void InitializeTestData() {}
 
   void RunTest() {
@@ skipping 31 matching lines @@
         FROM_HERE,
         base::Bind(&PicasaDataProviderTest::DestructDataProviderThenQuit,
                    base::Unretained(this)));
   }
 
   const base::FilePath& test_folder_1_path() { return test_folder_1_.path(); }
   const base::FilePath& test_folder_2_path() { return test_folder_2_.path(); }
 
   PmpTestHelper* test_helper() { return &test_helper_; }
 
-  PicasaDataProvider* data_provider() const {
+  TestPicasaDataProvider* data_provider() const {
     return picasa_data_provider_.get();
   }
 
  private:
   void SetupFoldersAndDataProvider() {
     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() {
     DCHECK(MediaFileSystemBackend::CurrentlyOnMediaTaskRunnerThread());
     picasa_data_provider_.reset();
     content::BrowserThread::PostTask(
         content::BrowserThread::UI, FROM_HERE, quit_closure_);
   }
 
   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_;
 
   DISALLOW_COPY_AND_ASSIGN(PicasaDataProviderTest);
 };
 
 class PicasaDataProviderNoDatabaseGetListTest : public PicasaDataProviderTest {
  protected:
   virtual PicasaDataProvider::DataType RequestedDataType() const OVERRIDE {
     return PicasaDataProvider::LIST_OF_ALBUMS_AND_FOLDERS_DATA;
@@ skipping 116 matching lines @@
   void CheckTestDone() {
     ASSERT_LE(list_callbacks_called_, 2);
     ASSERT_LE(albums_images_callbacks_called_, 2);
     if (list_callbacks_called_ == 2 && albums_images_callbacks_called_ == 2)
       TestDone();
   }
 
   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,
                    base::Unretained(this),
                    1));
     data_provider()->RefreshData(
         PicasaDataProvider::ALBUMS_IMAGES_DATA,
         base::Bind(
             &PicasaDataProviderMultipleMixedCallbacksTest::AlbumsImagesCallback,
             base::Unretained(this),
@@ skipping 13 matching lines @@
 
   int list_callbacks_called_;
   int albums_images_callbacks_called_;
 };
 
 IN_PROC_BROWSER_TEST_F(PicasaDataProviderMultipleMixedCallbacksTest,
                        MultipleMixedCallbacks) {
   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();
+    // Validate the list after the file move triggers an invalidate.
+    data_provider()->SetInvalidateCallback(base::Bind(
+        &PicasaDataProvider::RefreshData,
+        base::Unretained(data_provider()),
+        RequestedDataType(),
+        base::Bind(
+            &PicasaDataProviderFileWatcherInvalidateTest::VerifyRefreshResults,
+            base::Unretained(this))));
 
-    // Have to post this, otherwise this will run the callback immediately.
-    MediaFileSystemBackend::MediaTaskRunner()->PostTask(
-        FROM_HERE,
-        base::Bind(
-            &PicasaDataProvider::RefreshData,
-            base::Unretained(data_provider()),
-            RequestedDataType(),
-            base::Bind(
-                &PicasaDataProviderInvalidateSimpleTest::SecondListCallback,
-                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();
 }
 
 class PicasaDataProviderInvalidateInflightTableReaderTest
     : public PicasaDataProviderGetListTest {
  private:
   virtual void StartTestOnMediaTaskRunner() OVERRIDE {
     DCHECK(MediaFileSystemBackend::CurrentlyOnMediaTaskRunnerThread());
 
     // Temporarily empty the database path to guarantee that the first utility
@@ skipping 18 matching lines @@
   RunTest();
 }
 
 class PicasaDataProviderInvalidateInflightAlbumsIndexerTest
     : public PicasaDataProviderGetAlbumsImagesTest {
  protected:
   virtual void ListCallback(bool parse_success) {
     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,
         base::Bind(&PicasaDataProviderInvalidateInflightAlbumsIndexerTest::
                        VerifyRefreshResults,
                    base::Unretained(this)));
 
     // Now invalidate all the data. The album maps will be re-read.
     data_provider()->InvalidateData();
 
     // VerifyRefreshResults callback should receive correct results now.
   }
 
  private:
   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::
                        ListCallback,
                    base::Unretained(this)));
   }
 };
 
 IN_PROC_BROWSER_TEST_F(PicasaDataProviderInvalidateInflightAlbumsIndexerTest,
                        InvalidateInflightAlbumsIndexerTest) {
   RunTest();
 }
 
 }  // namespace picasa