Media Galleries API Picasa: Make PicasaDataProvider handle async PMP and INI parsing robustly.

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
new file mode 100644
index 0000000000000000000000000000000000000000..ed51ca7ec22ae1592034d9f77f7f25eb374ea819
--- /dev/null
+++ b/chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc
@@ -0,0 +1,503 @@
+// 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/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"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "content/public/test/test_browser_thread.h"
+
+using chrome::MediaFileSystemBackend;
+
+namespace picasa {
+
+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() {
+    DCHECK(MediaFileSystemBackend::CurrentlyOnMediaTaskRunnerThread());
+    ASSERT_TRUE(test_helper_.Init());
+    picasa_data_provider_.reset(
+        new PicasaDataProvider(test_helper_.GetTempDirPath()));
+  }
+
+  void RunTest() {
+    DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+    base::RunLoop loop;
+    quit_closure_ = loop.QuitClosure();
+    MediaFileSystemBackend::MediaTaskRunner()->PostTask(
+        FROM_HERE,
+        base::Bind(&PicasaDataProviderTest::StartTestOnMediaTaskRunner,
+                   base::Unretained(this)));
+    loop.Run();
+  }
+
+  virtual PicasaDataProvider::DataType RequestedDataType() const = 0;
+
+  // Start the test. The data provider is refreshed before calling StartTest
+  // and the result of the refresh is passed in.
+  virtual void VerifyRefreshResults(bool parse_success) = 0;
+
+  void TestDone() {
+    DCHECK(MediaFileSystemBackend::CurrentlyOnMediaTaskRunnerThread());
+
+    // The data provider must be destructed on the MediaTaskRunner. This is done
+    // in a posted task rather than directly because TestDone is called by
+    // PicasaDataProvider. The callee should not destroy the caller.
+    MediaFileSystemBackend::MediaTaskRunner()->PostTask(
+        FROM_HERE,
+        base::Bind(&PicasaDataProviderTest::DestructDataProviderThenQuit,
+                   base::Unretained(this)));
+  }
+
+  PmpTestHelper* test_helper() { return &test_helper_; }
+
+  PicasaDataProvider* data_provider() const {
+    return picasa_data_provider_.get();
+  }
+
+ private:
+  virtual void StartTestOnMediaTaskRunner() {
+    DCHECK(MediaFileSystemBackend::CurrentlyOnMediaTaskRunnerThread());
+    InitializeTestData();
+
+    data_provider()->RefreshData(
+        RequestedDataType(),
+        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_);
+  }
+
+  PmpTestHelper test_helper_;
+  scoped_ptr<PicasaDataProvider> 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;
+  }
+  virtual void VerifyRefreshResults(bool parse_success) OVERRIDE {
+    EXPECT_FALSE(parse_success);
+    TestDone();
+  }
+};
+
+IN_PROC_BROWSER_TEST_F(PicasaDataProviderNoDatabaseGetListTest,
+                       NoDatabaseGetList) {
+  RunTest();
+}
+
+class PicasaDataProviderNoDatabaseGetAlbumsImagesTest
+    : public PicasaDataProviderTest {
+ protected:
+  virtual PicasaDataProvider::DataType RequestedDataType() const OVERRIDE {
+    return PicasaDataProvider::ALBUMS_IMAGES_DATA;
+  }
+  virtual void VerifyRefreshResults(bool parse_success) OVERRIDE {
+    EXPECT_FALSE(parse_success);
+    TestDone();
+  }
+};
+
+IN_PROC_BROWSER_TEST_F(PicasaDataProviderNoDatabaseGetAlbumsImagesTest,
+                       NoDatabaseGetAlbumsImages) {
+  RunTest();
+}
+
+class PicasaDataProviderGetListTest : public PicasaDataProviderTest {
+ protected:
+  virtual void InitializeTestData() OVERRIDE {
+    PicasaDataProviderTest::InitializeTestData();
+    WritePicasaDatabase();
+  }
+
+  void WritePicasaDatabase() {

[vandebo (ex-Chrome), 2013/08/22 17:48:02]

Make this a helper function with a better name (instead of method) that just
takes the two ScopedTempDirs as arguments.  That way it can get reused without
artificial inheritance.

[tommycli, 2013/08/22 22:32:51]

Done.

+    ASSERT_TRUE(test_folder_1_.CreateUniqueTempDir());
+    ASSERT_TRUE(test_folder_2_.CreateUniqueTempDir());
+
+    std::vector<uint32> category_vector;
+    category_vector.push_back(kAlbumCategoryFolder);
+    category_vector.push_back(kAlbumCategoryInvalid);
+    category_vector.push_back(kAlbumCategoryAlbum);
+    category_vector.push_back(kAlbumCategoryFolder);
+    category_vector.push_back(kAlbumCategoryAlbum);
+
+    std::vector<double> date_vector;
+    date_vector.push_back(0.0);
+    date_vector.push_back(0.0);
+    date_vector.push_back(0.0);
+    date_vector.push_back(0.0);
+    date_vector.push_back(0.0);
+
+    std::vector<std::string> filename_vector;
+    filename_vector.push_back(test_folder_1_.path().AsUTF8Unsafe());
+    filename_vector.push_back("");
+    filename_vector.push_back("");
+    filename_vector.push_back(test_folder_2_.path().AsUTF8Unsafe());
+    filename_vector.push_back("");
+
+    std::vector<std::string> name_vector;
+    name_vector.push_back(test_folder_1_.path().BaseName().AsUTF8Unsafe());
+    name_vector.push_back("");
+    name_vector.push_back("Album 1 Name");
+    name_vector.push_back(test_folder_2_.path().BaseName().AsUTF8Unsafe());
+    name_vector.push_back("Album 2 Name");
+
+    std::vector<std::string> token_vector;
+    token_vector.push_back("");
+    token_vector.push_back("");
+    token_vector.push_back(std::string(kAlbumTokenPrefix) + "uid3");
+    token_vector.push_back("");
+    token_vector.push_back(std::string(kAlbumTokenPrefix) + "uid5");
+
+    std::vector<std::string> uid_vector;
+    uid_vector.push_back("uid1");
+    uid_vector.push_back("uid2");
+    uid_vector.push_back("uid3");
+    uid_vector.push_back("uid4");
+    uid_vector.push_back("uid5");
+
+    ASSERT_TRUE(test_helper()->WriteColumnFileFromVector(
+        "category", PMP_TYPE_UINT32, category_vector));
+    ASSERT_TRUE(test_helper()->WriteColumnFileFromVector(
+        "date", PMP_TYPE_DOUBLE64, date_vector));
+    ASSERT_TRUE(test_helper()->WriteColumnFileFromVector(
+        "filename", PMP_TYPE_STRING, filename_vector));
+    ASSERT_TRUE(test_helper()->WriteColumnFileFromVector(
+        "name", PMP_TYPE_STRING, name_vector));
+    ASSERT_TRUE(test_helper()->WriteColumnFileFromVector(
+        "token", PMP_TYPE_STRING, token_vector));
+    ASSERT_TRUE(test_helper()->WriteColumnFileFromVector(
+        "uid", PMP_TYPE_STRING, uid_vector));
+  }
+
+  virtual PicasaDataProvider::DataType RequestedDataType() const OVERRIDE {
+    return PicasaDataProvider::LIST_OF_ALBUMS_AND_FOLDERS_DATA;
+  }
+
+  virtual void VerifyRefreshResults(bool parse_success) OVERRIDE {
+    ASSERT_TRUE(parse_success);
+    VerifyListOfAlbumsAndFolders();
+    TestDone();
+  }
+
+  void VerifyListOfAlbumsAndFolders() {
+    scoped_ptr<AlbumMap> folders = data_provider()->GetFolders();
+    ASSERT_TRUE(folders.get());
+    EXPECT_EQ(2u, folders->size());
+
+    AlbumMap::const_iterator folder_1 = folders->find(
+        test_folder_1_.path().BaseName().AsUTF8Unsafe() + " 1899-12-30");
+    EXPECT_NE(folders->end(), folder_1);
+    EXPECT_EQ(test_folder_1_.path().BaseName().AsUTF8Unsafe(),
+              folder_1->second.name);
+    EXPECT_EQ(test_folder_1_.path(), folder_1->second.path);
+    EXPECT_EQ("uid1", folder_1->second.uid);
+
+    AlbumMap::const_iterator folder_2 = folders->find(
+        test_folder_2_.path().BaseName().AsUTF8Unsafe() + " 1899-12-30");
+    EXPECT_NE(folders->end(), folder_2);
+    EXPECT_EQ(test_folder_2_.path().BaseName().AsUTF8Unsafe(),
+              folder_2->second.name);
+    EXPECT_EQ(test_folder_2_.path(), folder_2->second.path);
+    EXPECT_EQ("uid4", folder_2->second.uid);
+
+    scoped_ptr<AlbumMap> albums = data_provider()->GetAlbums();
+    ASSERT_TRUE(albums.get());
+    EXPECT_EQ(2u, albums->size());
+
+    AlbumMap::const_iterator album_1 = albums->find("Album 1 Name 1899-12-30");
+    EXPECT_NE(albums->end(), album_1);
+    EXPECT_EQ("Album 1 Name", album_1->second.name);
+    EXPECT_EQ(base::FilePath(), album_1->second.path);
+    EXPECT_EQ("uid3", album_1->second.uid);
+
+    AlbumMap::const_iterator album_2 = albums->find("Album 2 Name 1899-12-30");
+    EXPECT_NE(albums->end(), album_2);
+    EXPECT_EQ("Album 2 Name", album_2->second.name);
+    EXPECT_EQ(base::FilePath(), album_2->second.path);
+    EXPECT_EQ("uid5", album_2->second.uid);
+  }
+
+  base::ScopedTempDir test_folder_1_;
+  base::ScopedTempDir test_folder_2_;
+};
+
+IN_PROC_BROWSER_TEST_F(PicasaDataProviderGetListTest, GetListTest) {
+  RunTest();
+}
+
+class PicasaDataProviderGetAlbumsImagesTest
+    : public PicasaDataProviderGetListTest {
+ protected:
+  virtual void InitializeTestData() OVERRIDE {
+    PicasaDataProviderGetListTest::InitializeTestData();
+    WriteFolderINIs();
+  }
+
+  void WriteFolderINIs() {
+    const char folder_1_test_ini[] =
+        "[InBoth.jpg]\n"
+        "albums=uid3,uid5\n"
+        "[InSecondAlbumOnly.jpg]\n"
+        "albums=uid5\n";
+    ASSERT_TRUE(file_util::WriteFile(
+        test_folder_1_.path().AppendASCII(kPicasaINIFilename),
+        folder_1_test_ini,
+        arraysize(folder_1_test_ini)));
+
+    const char folder_2_test_ini[] =
+        "[InFirstAlbumOnly.jpg]\n"
+        "albums=uid3\n";
+    ASSERT_TRUE(file_util::WriteFile(
+        test_folder_2_.path().AppendASCII(kPicasaINIFilename),
+        folder_2_test_ini,
+        arraysize(folder_2_test_ini)));
+  }
+
+  virtual PicasaDataProvider::DataType RequestedDataType() const OVERRIDE {
+    return PicasaDataProvider::ALBUMS_IMAGES_DATA;
+  }
+
+  virtual void VerifyRefreshResults(bool parse_success) OVERRIDE {
+    ASSERT_TRUE(parse_success);
+    VerifyListOfAlbumsAndFolders();
+    VerifyAlbumsImages();
+    TestDone();
+  }
+
+  void VerifyAlbumsImages() {
+    base::PlatformFileError error;
+    scoped_ptr<AlbumImages> album_1_images =
+        data_provider()->FindAlbumImages("uid3", &error);
+    ASSERT_TRUE(album_1_images);
+    EXPECT_EQ(base::PLATFORM_FILE_OK, error);
+    EXPECT_EQ(2u, album_1_images->size());
+    EXPECT_EQ(

[tommycli, 2013/08/21 21:11:59]

Technically correct though somewhat inconsistent formatting, but this is what
'git cl format' gave me.

[tommycli, 2013/08/22 22:32:51]

Done.

+        1u,
+        album_1_images->count(test_folder_1_.path().AppendASCII("InBoth.jpg")));
+    EXPECT_EQ(1u,
+              album_1_images->count(
+                  test_folder_2_.path().AppendASCII("InFirstAlbumOnly.jpg")));
+
+    scoped_ptr<AlbumImages> album_2_images =
+        data_provider()->FindAlbumImages("uid5", &error);
+    ASSERT_TRUE(album_2_images);
+    EXPECT_EQ(base::PLATFORM_FILE_OK, error);
+    EXPECT_EQ(2u, album_2_images->size());
+    EXPECT_EQ(
+        1u,
+        album_2_images->count(test_folder_1_.path().AppendASCII("InBoth.jpg")));
+    EXPECT_EQ(1u,
+              album_2_images->count(
+                  test_folder_1_.path().AppendASCII("InSecondAlbumOnly.jpg")));
+  }
+};
+
+IN_PROC_BROWSER_TEST_F(PicasaDataProviderGetAlbumsImagesTest,
+                       GetAlbumsImagesTest) {
+  RunTest();
+}
+
+class PicasaDataProviderMultipleMixedCallbacksTest
+    : public PicasaDataProviderGetAlbumsImagesTest {
+ public:
+  PicasaDataProviderMultipleMixedCallbacksTest()
+      : list_callbacks_called_(0), albums_images_callbacks_called_(0) {}
+
+ protected:
+  virtual void ListCallback(bool parse_success) {

[vandebo (ex-Chrome), 2013/08/22 17:48:02]

Make this take the expected value of list_callbacks (post increment) and verify
that it is correct.

[tommycli, 2013/08/22 22:32:51]

Done.

+    ASSERT_TRUE(parse_success);
+    ASSERT_LE(list_callbacks_called_, 2);
+    ASSERT_LE(albums_images_callbacks_called_, 2);
+    VerifyListOfAlbumsAndFolders();
+
+    ++list_callbacks_called_;
+    CheckTestDone();
+  }
+
+  virtual void AlbumsImagesCallback(bool parse_success) {
+    ASSERT_TRUE(parse_success);
+    ASSERT_LE(list_callbacks_called_, 2);
+    ASSERT_LE(albums_images_callbacks_called_, 2);
+    VerifyAlbumsImages();
+
+    ++albums_images_callbacks_called_;
+    CheckTestDone();
+  }
+
+ private:
+  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() {
+    DCHECK(MediaFileSystemBackend::CurrentlyOnMediaTaskRunnerThread());
+    InitializeTestData();
+
+    data_provider()->RefreshData(
+        PicasaDataProvider::LIST_OF_ALBUMS_AND_FOLDERS_DATA,
+        base::Bind(&PicasaDataProviderMultipleMixedCallbacksTest::ListCallback,
+                   base::Unretained(this)));
+    data_provider()->RefreshData(
+        PicasaDataProvider::ALBUMS_IMAGES_DATA,
+        base::Bind(
+            &PicasaDataProviderMultipleMixedCallbacksTest::AlbumsImagesCallback,
+            base::Unretained(this)));
+    data_provider()->RefreshData(
+        PicasaDataProvider::LIST_OF_ALBUMS_AND_FOLDERS_DATA,
+        base::Bind(&PicasaDataProviderMultipleMixedCallbacksTest::ListCallback,
+                   base::Unretained(this)));
+    data_provider()->RefreshData(
+        PicasaDataProvider::ALBUMS_IMAGES_DATA,
+        base::Bind(
+            &PicasaDataProviderMultipleMixedCallbacksTest::AlbumsImagesCallback,
+            base::Unretained(this)));
+  }
+
+  int list_callbacks_called_;
+  int albums_images_callbacks_called_;
+};
+
+IN_PROC_BROWSER_TEST_F(PicasaDataProviderMultipleMixedCallbacksTest,
+                       MultipleMixedCallbacks) {
+  RunTest();
+}
+
+class PicasaDataProviderInvalidateSimpleTest
+    : public PicasaDataProviderGetListTest {
+ protected:
+  virtual void FirstListCallback(bool parse_success) {
+    ASSERT_FALSE(parse_success);
+    WritePicasaDatabase();
+    data_provider()->InvalidateData();

[vandebo (ex-Chrome), 2013/08/22 17:48:02]

Add TODO here, to remove this line when you start watching the database files.

[tommycli, 2013/08/22 22:32:51]

Done.

+
+    // 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);
+    VerifyListOfAlbumsAndFolders();
+    TestDone();
+  }
+
+ private:
+  virtual void StartTestOnMediaTaskRunner() {
+    DCHECK(MediaFileSystemBackend::CurrentlyOnMediaTaskRunnerThread());
+
+    // We don't want to write the database until later.
+    PicasaDataProviderTest::InitializeTestData();
+
+    data_provider()->RefreshData(
+        RequestedDataType(),
+        base::Bind(&PicasaDataProviderInvalidateSimpleTest::FirstListCallback,
+                   base::Unretained(this)));
+  }
+};
+
+IN_PROC_BROWSER_TEST_F(PicasaDataProviderInvalidateSimpleTest,
+                       InvalidateSimpleTest) {
+  RunTest();
+}
+
+class PicasaDataProviderInvalidateInflightTableReaderTest

[vandebo (ex-Chrome), 2013/08/22 17:48:02]

Is this test racey?  I don't see how it works.

[tommycli, 2013/08/22 22:32:51]

Done.

+    : public PicasaDataProviderGetListTest {
+  virtual void StartTestOnMediaTaskRunner() {
+    DCHECK(MediaFileSystemBackend::CurrentlyOnMediaTaskRunnerThread());
+
+    // We don't want to write the database until later.
+    PicasaDataProviderTest::InitializeTestData();
+
+    // Kickoff an album table reader without data already written.
+    data_provider()->RefreshData(
+        RequestedDataType(),
+        base::Bind(&PicasaDataProviderInvalidateInflightTableReaderTest::
+                       VerifyRefreshResults,
+                   base::Unretained(this)));
+
+    // Now write the database and invalidate the inflight table reader.
+    WritePicasaDatabase();
+    data_provider()->InvalidateData();
+  }

[vandebo (ex-Chrome), 2013/08/22 17:48:02]

It seems like something needs to happen after the invalidate

[tommycli, 2013/08/22 22:32:51]

Done.

+};
+
+IN_PROC_BROWSER_TEST_F(PicasaDataProviderInvalidateInflightTableReaderTest,
+                       InvalidateInflightTableReaderTest) {
+  RunTest();
+}
+
+class PicasaDataProviderInvalidateInflightAlbumsIndexerTest
+    : public PicasaDataProviderGetAlbumsImagesTest {
+ protected:
+  virtual void ListCallback(bool parse_success) {
+    ASSERT_TRUE(parse_success);
+
+    // Kickoff an albums indexer without its required INI files written.
+    data_provider()->RefreshData(
+        PicasaDataProvider::ALBUMS_IMAGES_DATA,
+        base::Bind(&PicasaDataProviderInvalidateInflightAlbumsIndexerTest::
+                       VerifyRefreshResults,
+                   base::Unretained(this)));
+
+    // Now write the INI files and invalidate the inflight albums indexer.
+    WriteFolderINIs();

[vandebo (ex-Chrome), 2013/08/22 17:48:02]

This also seems to be racey.

[tommycli, 2013/08/22 22:32:51]

Done.

+    data_provider()->InvalidateData();
+  }
+
+ private:
+  virtual void StartTestOnMediaTaskRunner() {
+    DCHECK(MediaFileSystemBackend::CurrentlyOnMediaTaskRunnerThread());
+
+    // Write the Picasa database but not the INI files needed by albums indexer.
+    PicasaDataProviderGetListTest::InitializeTestData();
+
+    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