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

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

Index: chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc
diff --git a/chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc b/chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc
index a2f503cc35634af7addaa668f2a9d6032bd1705f..0f30a4ffac2aeb0b97b4473077370bc8d546693b 100644
--- a/chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc
+++ b/chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc
@@ -7,10 +7,11 @@
 #include <utility>
 
 #include "base/basictypes.h"
-#include "base/callback.h"
+#include "base/bind_helpers.h"
 #include "base/strings/stringprintf.h"
 #include "chrome/browser/media_galleries/fileapi/media_file_system_backend.h"
 #include "chrome/browser/media_galleries/fileapi/safe_picasa_album_table_reader.h"
+#include "chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.h"
 #include "chrome/browser/media_galleries/imported_media_gallery_registry.h"
 #include "webkit/browser/fileapi/file_system_operation_context.h"
 #include "webkit/browser/fileapi/file_system_url.h"
@@ -21,28 +22,59 @@ namespace picasa {
 
 PicasaDataProvider::PicasaDataProvider(const base::FilePath& database_path)
     : database_path_(database_path),
-      needs_refresh_(true),
+      data_state_(STALE_DATA_STATE),
       weak_factory_(this) {
 }
 
 PicasaDataProvider::~PicasaDataProvider() {}
 
-void PicasaDataProvider::RefreshData(const base::Closure& ready_callback) {
+void PicasaDataProvider::RefreshData(
+    DataType needed_data,
+    const ReadyCallback& ready_callback) {
   DCHECK(MediaFileSystemBackend::CurrentlyOnMediaTaskRunnerThread());
   // TODO(tommycli): Need to watch the database_path_ folder and handle
   // rereading the data when it changes.
-  if (!needs_refresh_) {
-    ready_callback.Run();
-    return;
-  }
 
-  needs_refresh_ = false;
-  album_table_reader_ = new SafePicasaAlbumTableReader(
-      AlbumTableFiles(database_path_),
-      base::Bind(&PicasaDataProvider::OnDataRefreshed,
-                 weak_factory_.GetWeakPtr(),
-                 ready_callback));
-  album_table_reader_->Start();
+  switch (data_state_) {

[vandebo (ex-Chrome), 2013/07/12 18:29:59]

An open coded version is a lot shorter and I think less confusing:


  if (needed_data == LIST_OF_ALBUMS_AND_FOLDERS_DATA) {
    if (state_ != STALE_DATA_STATE) {
      ready_callback.Run(true);
      return;
    }
    album_list_ready_callbacks_.push(ready_callback);
  } else {
    if (state_ == ALBUMS_IMAGES_FRESH_STATE) {
      ready_callback.Run(true);
      return;
    }
    albums_indexer_ready_callbacks_.push(ready_callback);
  }
  DoRefreshIfNecessary();

[tommycli, 2013/07/12 21:37:19]

Done.

+    case STALE_DATA_STATE:
+      StartAlbumListRefreshIfNecessary();
+      switch (needed_data) {
+        case LIST_OF_ALBUMS_AND_FOLDERS_DATA:
+          album_list_ready_callbacks_.push(ready_callback);
+          break;
+        case ALBUMS_IMAGES_DATA:
+          albums_indexer_ready_callbacks_.push(ready_callback);
+          break;
+        default:
+          NOTREACHED();
+      }
+      break;
+    case ALBUM_LIST_FRESH_STATE:
+      switch (needed_data) {
+        case LIST_OF_ALBUMS_AND_FOLDERS_DATA:
+          ready_callback.Run(true /* success */);
+          break;
+        case ALBUMS_IMAGES_DATA:
+          StartAlbumsImagesRefreshIfNecessary();
+          albums_indexer_ready_callbacks_.push(ready_callback);
+          break;
+        default:
+          NOTREACHED();
+      }
+      break;
+    case ALBUMS_IMAGES_FRESH_STATE:
+      switch (needed_data) {
+        case LIST_OF_ALBUMS_AND_FOLDERS_DATA:
+        case ALBUMS_IMAGES_DATA:
+          ready_callback.Run(true /* success */);
+          break;
+        default:
+          NOTREACHED();
+      }
+      break;
+    default:
+      NOTREACHED();
+  }
 }
 
 scoped_ptr<AlbumMap> PicasaDataProvider::GetFolders() {
@@ -55,17 +87,106 @@ scoped_ptr<AlbumMap> PicasaDataProvider::GetAlbums() {
   return make_scoped_ptr(new AlbumMap(album_map_));
 }
 
-void PicasaDataProvider::OnDataRefreshed(
-    const base::Closure& ready_callback,
+scoped_ptr<AlbumImagesMap> PicasaDataProvider::GetAlbumsImages() {
+  DCHECK(MediaFileSystemBackend::CurrentlyOnMediaTaskRunnerThread());
+  return make_scoped_ptr(new AlbumImagesMap(albums_images_));
+}
+
+void PicasaDataProvider::InvalidateData() {
+  DCHECK(MediaFileSystemBackend::CurrentlyOnMediaTaskRunnerThread());
+  DCHECK(!(album_table_reader_ && albums_indexer_));
+
+  // Set data state to stale and ignore responses from any in-flight processes.
+  // TODO(tommycli): Implement and call Cancel function for these
+  // UtilityProcessHostClients to actually kill the in-flight processes.
+  data_state_ = STALE_DATA_STATE;
+  album_table_reader_ = NULL;
+  albums_indexer_ = NULL;
+
+  if (!album_list_ready_callbacks_.empty() ||
+      !albums_indexer_ready_callbacks_.empty()) {
+    StartAlbumListRefreshIfNecessary();
+  }
+}
+
+void PicasaDataProvider::StartAlbumListRefreshIfNecessary() {

[vandebo (ex-Chrome), 2013/07/12 18:29:59]

It might be simpler to combine these two methods into one:

void DoRefreshIfNecessary() {
  DCHECK(state_ != ALBUMS_IMAGES_FRESH_STATE);
  if (state_ == STALE_DATA_STATE) {
    if (album_table_reader_)
      return;
    album_table_reader_ = ...
    album_table_reader_->Start();
  } else {
    if (album_table_reader_ || albums_indexer_ready_callbacks_.empty())
      return;
    album_inderer_ = ...
    album_indexer_->Start();
  }
} 

[tommycli, 2013/07/12 21:37:19]

Done.

+  if (!album_table_reader_)

[vandebo (ex-Chrome), 2013/07/12 18:29:59]

Is this negated?

[tommycli, 2013/07/12 21:37:19]

Done.

+    return;
+  album_table_reader_ = new SafePicasaAlbumTableReader(
+      AlbumTableFiles(database_path_),
+      base::Bind(&PicasaDataProvider::OnAlbumListRefreshed,
+                 weak_factory_.GetWeakPtr()));
+  album_table_reader_->Start();
+}
+
+void PicasaDataProvider::StartAlbumsImagesRefreshIfNecessary() {
+  if (!albums_indexer_)
+    return;
+  albums_indexer_ = new SafePicasaAlbumsIndexer(
+      album_map_,
+      folder_map_,
+      base::Bind(&PicasaDataProvider::OnAlbumsIndexerDone,
+                 weak_factory_.GetWeakPtr()));
+  albums_indexer_->Start();
+}
+
+void PicasaDataProvider::OnAlbumListRefreshed(
+    scoped_refptr<SafePicasaAlbumTableReader> reader,
     bool parse_success,
     const std::vector<AlbumInfo>& albums,
     const std::vector<AlbumInfo>& folders) {
   DCHECK(MediaFileSystemBackend::CurrentlyOnMediaTaskRunnerThread());
+  // If the reader has already been deemed stale, ignore the result.
+  if (reader != album_table_reader_)
+    return;
+
   if (parse_success) {

[vandebo (ex-Chrome), 2013/07/12 18:29:59]

You may want to handle parsing failed first..
You probably need another state to; no point it trying to reparse the same pmp
that is invalid.

[tommycli, 2013/07/12 21:37:19]

Done.

+    data_state_ = ALBUM_LIST_FRESH_STATE;
+
+    album_map_.clear();
+    folder_map_.clear();
     UniquifyNames(albums, &album_map_);
     UniquifyNames(folders, &folder_map_);
   }
-  ready_callback.Run();
+
+  while (!album_list_ready_callbacks_.empty()) {

[vandebo (ex-Chrome), 2013/07/12 18:29:59]

Probably want to pull this loop into a small helper function:
RunCallbacks(std::queue<ReadyCallback>* callback, bool result);

[tommycli, 2013/07/12 21:37:19]

Done.

+    album_list_ready_callbacks_.front().Run(parse_success);
+    album_list_ready_callbacks_.pop();
+  }
+
+  if (parse_success) {
+    // Chain from this process onto refreshing the albums images if necessary.
+    if (!albums_indexer_ready_callbacks_.empty())
+      StartAlbumsImagesRefreshIfNecessary();
+  } else {
+    // If we didn't get the list successfully, fail all those waiting for
+    // the albums indexer also.
+    while (!albums_indexer_ready_callbacks_.empty()) {
+      albums_indexer_ready_callbacks_.front().Run(false /* success */);
+      albums_indexer_ready_callbacks_.pop();
+    }
+  }
+}
+
+void PicasaDataProvider::OnAlbumsIndexerDone(
+    scoped_refptr<SafePicasaAlbumsIndexer> indexer,
+    bool success,
+    const picasa::AlbumImagesMap& albums_images) {
+  DCHECK(MediaFileSystemBackend::CurrentlyOnMediaTaskRunnerThread());
+  // If the indexer has already been deemed stale, ignore the result.
+  if (indexer != albums_indexer_)
+    return;
+
+  if (success) {
+    data_state_ = ALBUMS_IMAGES_FRESH_STATE;
+
+    albums_images_ = albums_images;
+  }
+
+  while (!albums_indexer_ready_callbacks_.empty()) {
+    albums_indexer_ready_callbacks_.front().Run(success);
+    albums_indexer_ready_callbacks_.pop();
+  }
 }
 
 // static