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..dbf2d467256505ae0931e448afac7d8a2ae59844 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,40 @@ namespace picasa {
 
 PicasaDataProvider::PicasaDataProvider(const base::FilePath& database_path)
     : database_path_(database_path),
-      needs_refresh_(true),
+      data_state_(NO_FRESH_DATA_STATE),
       weak_factory_(this) {
 }
 
 PicasaDataProvider::~PicasaDataProvider() {}
 
-void PicasaDataProvider::RefreshData(const base::Closure& ready_callback) {
+void PicasaDataProvider::RefreshData(
+    bool need_albums_images,

[vandebo (ex-Chrome), 2013/07/11 16:56:27]

Better to have this as an enum than a bool; it makes the call sites easier to
read.

[tommycli, 2013/07/11 21:19:17]

Done. This is kind of a strange case:

Any time we want the ALBUMS_IMAGES, we will also want the ALBUM_LIST, and
PicasaDataProvider is expected to have the ALBUM_LIST ready.

Should we make this relationship explicit by making callers pass in bitmasks
like ALBUM_LIST|ALBUMS_IMAGES ? Or just leave it as I've modified it to...

[vandebo (ex-Chrome), 2013/07/11 23:22:37]

I think it's ok the way it is.  If you want the ini data, you don't want the
other data, it's just that we need the other data to get the ini data -> it's an
implementation detail that one implies the other.

[tommycli, 2013/07/12 01:37:48]

Done.

+    const base::Closure& 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();
+

[vandebo (ex-Chrome), 2013/07/11 16:56:27]

Do an early exit here.  If state is already good enough for the request, run the
callback directly.  Otherwise, drop it into the queue it needs to go in and call
the appropriate ensure method (at that point though, it's not ensure, it's do). 

[tommycli, 2013/07/11 21:19:17]

The Ensure* functions perform the early exits themselves, and do it just as
efficiently (other than the overhead of the function call). It'd be kind of
duplicated to do it here also.

[vandebo (ex-Chrome), 2013/07/11 23:22:37]

I really think that things will end up being a lot simpler if the ensure methods
don't take the call backs and you don't add additional callbacks (i.e. all the
callbacks in both lists should be from calls to Refresh).

[tommycli, 2013/07/12 01:37:48]

Done.

+  if (need_albums_images)
+    EnsureAlbumListRefreshed(ready_callback);
+  else
+    EnsureAlbumsImagesRefreshed(ready_callback);
+}
+
+void PicasaDataProvider::InvalidateData() {
+  DCHECK(MediaFileSystemBackend::CurrentlyOnMediaTaskRunnerThread());
+  DCHECK(!(album_table_reader_ && albums_indexer_));
+
+  if (album_table_reader_) {

[vandebo (ex-Chrome), 2013/07/11 16:56:27]

This should first set the state to NO_FRESH_DATA_STATE.  Then if either of the
work tasks are outstanding, cancel it and start from the beginning. Actually,
better would be if there is anything in either of the callback queues, start
from the beginning.

[tommycli, 2013/07/11 21:19:17]

Done.

+    album_table_reader_ = NULL;
+    EnsureAlbumListRefreshed(base::Bind(&base::DoNothing));

[vandebo (ex-Chrome), 2013/07/11 16:56:27]

These DoNothing callbacks could possibly accumulate... I think there's a cleaner
way that doesn't need them.

[tommycli, 2013/07/11 21:19:17]

Done.

     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();
+  if (albums_indexer_) {
+    albums_indexer_ = NULL;
+    EnsureAlbumsImagesRefreshed(base::Bind(&base::DoNothing));
+    return;
+  }
 }
 
 scoped_ptr<AlbumMap> PicasaDataProvider::GetFolders() {
@@ -55,17 +68,105 @@ 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::EnsureAlbumListRefreshed(
+    const base::Closure& ready_callback) {
+  DCHECK(MediaFileSystemBackend::CurrentlyOnMediaTaskRunnerThread());
+  switch (data_state_) {
+    case NO_FRESH_DATA_STATE:
+      if (!album_table_reader_) {
+        album_table_reader_ = new SafePicasaAlbumTableReader(
+            AlbumTableFiles(database_path_),
+            base::Bind(&PicasaDataProvider::OnAlbumListRefreshed,
+                       weak_factory_.GetWeakPtr()));
+        album_table_reader_->Start();
+      }
+      album_list_ready_callbacks_.push(ready_callback);
+      break;
+    case ALBUM_LIST_FRESH_STATE:
+    case ALBUMS_IMAGES_FRESH_STATE:
+      ready_callback.Run();
+      break;
+    default:
+      NOTREACHED();
+  }
+}
+
+void PicasaDataProvider::EnsureAlbumsImagesRefreshed(
+    const base::Closure& ready_callback) {
+  DCHECK(MediaFileSystemBackend::CurrentlyOnMediaTaskRunnerThread());
+  switch (data_state_) {
+    case NO_FRESH_DATA_STATE:
+      // Album list not ready. Refresh that first, then call this again.
+      EnsureAlbumListRefreshed(
+          base::Bind(&PicasaDataProvider::EnsureAlbumsImagesRefreshed,
+                     weak_factory_.GetWeakPtr(),
+                     ready_callback));
+      break;
+    case ALBUM_LIST_FRESH_STATE:
+      if (!albums_indexer_) {
+        albums_indexer_ = new SafePicasaAlbumsIndexer(
+            album_map_,
+            folder_map_,
+            base::Bind(&PicasaDataProvider::OnAlbumsIndexerDone,
+                       weak_factory_.GetWeakPtr()));
+        albums_indexer_->Start();
+      }
+      albums_indexer_ready_callbacks_.push(ready_callback);
+      break;
+    case ALBUMS_IMAGES_FRESH_STATE:
+      ready_callback.Run();
+      break;
+    default:
+      NOTREACHED();
+  }
+}
+
+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/11 16:56:27]

What to do on parse_failure ?  That's why the Itunes refresh callback has a
boolean argument, to push up parse failure.

[tommycli, 2013/07/11 21:19:17]

Yeah - I'm not sure what the right thing to do in parse failure case is...

If it's because the data is in an in-between inconsistent state, we'd want to
try again. But we also don't want an infinite loop. Thoughts?

[vandebo (ex-Chrome), 2013/07/11 23:22:37]

I think we shouldn't trigger the invalidate until we think the data has finished
updating... not sure if we can do that, but I think we should aim for that.

[tommycli, 2013/07/12 01:37:48]

Done.

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

[vandebo (ex-Chrome), 2013/07/11 16:56:27]

Then, if |albums_indexer_ready_callbacks_| isn't empty, chain to the album
indexer.

[tommycli, 2013/07/11 21:19:17]

The chaining itself is one of the callbacks in the queue when it's needed. I
could also do it explicitly here. Both ways work.

Thoughts?

[vandebo (ex-Chrome), 2013/07/11 23:22:37]

I think this makes things more complicated and harder to reason about.  I think
the callbacks should only be those passed into Refresh.

[tommycli, 2013/07/12 01:37:48]

Done.

+}
+
+void PicasaDataProvider::OnAlbumsIndexerDone(
+    scoped_refptr<SafePicasaAlbumsIndexer> indexer,

[vandebo (ex-Chrome), 2013/07/11 16:56:27]

Should this also take a parse_success argument? 

[tommycli, 2013/07/11 21:19:17]

That parser doesn't have a success argument. It just throws out anything it
can't parse and returns an empty map in the extreme case.

Again, I'm not sure how to handle success/failures here. Should we retry? Should
we serve the previous data? Should we explicitly return an error to the
FileUtil?

I definitely don't want one bad INI file to prevent the program from returning
any albums images.

+    const picasa::AlbumImagesMap& albums_images) {
+  DCHECK(MediaFileSystemBackend::CurrentlyOnMediaTaskRunnerThread());
+  // If the indexer has already been deemed stale, ignore the result.
+  if (indexer != albums_indexer_)
+    return;
+
+  albums_images_ = albums_images;
+
+  data_state_ = ALBUMS_IMAGES_FRESH_STATE;
+
+  while (!albums_indexer_ready_callbacks_.empty()) {
+    albums_indexer_ready_callbacks_.front().Run();
+    albums_indexer_ready_callbacks_.pop();
+  }
 }
 
 // static