Media Galleries API Picasa: Make PicasaFileUtil support album contents.

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

Index: chrome/browser/media_galleries/fileapi/picasa/picasa_file_util.cc
diff --git a/chrome/browser/media_galleries/fileapi/picasa/picasa_file_util.cc b/chrome/browser/media_galleries/fileapi/picasa/picasa_file_util.cc
index 8e75bbc1fc8814ca80d8c8842501f30419b923a0..3c83548956407524d39cf9fc179900810f7cb6a1 100644
--- a/chrome/browser/media_galleries/fileapi/picasa/picasa_file_util.cc
+++ b/chrome/browser/media_galleries/fileapi/picasa/picasa_file_util.cc
@@ -20,6 +20,7 @@
 #include "content/public/browser/browser_thread.h"
 #include "webkit/browser/fileapi/file_system_operation_context.h"
 #include "webkit/browser/fileapi/file_system_url.h"
+#include "webkit/browser/fileapi/native_file_util.h"
 #include "webkit/common/fileapi/file_system_util.h"
 
 using base::FilePath;
@@ -31,7 +32,6 @@ namespace picasa {
 
 namespace {
 
-// |error| is only set when the method fails and the return is NULL.
 base::PlatformFileError FindAlbumInfo(const std::string& key,
                                       const AlbumMap* map,
                                       AlbumInfo* album_info) {
@@ -49,6 +49,40 @@ base::PlatformFileError FindAlbumInfo(const std::string& key,
   return base::PLATFORM_FILE_OK;
 }
 
+// This method returns a pointer to the AlbumImages instead of a

[Greg Billock, 2013/08/21 17:13:17]

You could have const AlbumImages** as an outparam.

[tommycli, 2013/08/21 21:37:06]

I moved it to the data provider. Can't do AlbumImages** there because need to
make a copy of the struct.

+// base::PlatformFileError because copying an AlbumImages is expensive.
+const AlbumImages* FindAlbumImages(const std::string& key,

[Greg Billock, 2013/08/21 17:13:17]

Should this be a method in the data provider?

[tommycli, 2013/08/21 21:37:06]

Yes, that's actually much better. I put those changes into the ancestor CL:
https://codereview.chromium.org/18986012/

+                                   const AlbumImagesMap* map,
+                                   base::PlatformFileError* error) {
+  DCHECK(error);
+
+  if (!map) {

[Greg Billock, 2013/08/21 17:13:17]

should this be DCHECK?

[tommycli, 2013/08/21 21:37:06]

Done.

+    *error = base::PLATFORM_FILE_ERROR_FAILED;
+    return NULL;
+  }
+
+  AlbumImagesMap::const_iterator it = map->find(key);
+
+  if (it == map->end()) {
+    *error = base::PLATFORM_FILE_ERROR_NOT_FOUND;
+    return NULL;
+  }
+
+  *error = base::PLATFORM_FILE_OK;
+  return &(it->second);
+}
+
+PicasaDataProvider::DataType GetNeededDataType(
+    const fileapi::FileSystemURL& url) {
+  std::vector<std::string> components;
+  fileapi::VirtualPath::GetComponentsUTF8Unsafe(url.path(), &components);
+
+  if (components.size() >= 2 && components[0] == kPicasaDirAlbums)

[Greg Billock, 2013/08/21 17:13:17]

How about adding a comment to PicasaFileUtil explaining how the virtual FS is
laid out. I'm getting a /albums/album-name/filename impression, but I had to
work for it. :-)

[tommycli, 2013/08/21 21:37:06]

Done. Good idea.

+    return PicasaDataProvider::ALBUMS_IMAGES_DATA;
+
+  return PicasaDataProvider::LIST_OF_ALBUMS_AND_FOLDERS_DATA;
+}
+
 }  // namespace
 
 const char kPicasaDirAlbums[]  = "albums";
@@ -66,7 +100,7 @@ void PicasaFileUtil::GetFileInfoOnTaskRunnerThread(
     const fileapi::FileSystemURL& url,
     const GetFileInfoCallback& callback) {
   GetDataProvider()->RefreshData(
-      PicasaDataProvider::LIST_OF_ALBUMS_AND_FOLDERS_DATA,
+      GetNeededDataType(url),
       base::Bind(&PicasaFileUtil::GetFileInfoWithFreshDataProvider,
                  weak_factory_.GetWeakPtr(),
                  base::Passed(&context),
@@ -79,7 +113,7 @@ void PicasaFileUtil::ReadDirectoryOnTaskRunnerThread(
     const fileapi::FileSystemURL& url,
     const ReadDirectoryCallback& callback) {
   GetDataProvider()->RefreshData(
-      PicasaDataProvider::LIST_OF_ALBUMS_AND_FOLDERS_DATA,
+      GetNeededDataType(url),

[Greg Billock, 2013/08/21 17:13:17]

GetDataTypeForUrl ?

[tommycli, 2013/08/21 21:37:06]

Done.

       base::Bind(&PicasaFileUtil::ReadDirectoryWithFreshDataProvider,
                  weak_factory_.GetWeakPtr(),
                  base::Passed(&context),
@@ -206,7 +240,35 @@ base::PlatformFileError PicasaFileUtil::ReadDirectorySync(
       break;
     case 2:
       if (components[0] == kPicasaDirAlbums) {
-        // TODO(tommycli): Implement album contents.
+        scoped_ptr<AlbumMap> album_map = GetDataProvider()->GetAlbums();
+        AlbumInfo album_info;
+        base::PlatformFileError error =
+            FindAlbumInfo(components[1], album_map.get(), &album_info);
+        if (error != base::PLATFORM_FILE_OK)
+          return error;
+
+        scoped_ptr<AlbumImagesMap> album_images_map =
+            GetDataProvider()->GetAlbumsImages();
+        const AlbumImages* album_images =
+            FindAlbumImages(album_info.uid, album_images_map.get(), &error);
+        if (error != base::PLATFORM_FILE_OK)
+          return error;
+
+        for (AlbumImages::const_iterator it = album_images->begin();
+             it != album_images->end();
+             ++it) {
+          fileapi::DirectoryEntry entry;
+          base::PlatformFileInfo info;
+
+          // Simply skip files that we can't get info on.
+          if (fileapi::NativeFileUtil::GetFileInfo(it->second, &info) !=
+              base::PLATFORM_FILE_OK) {
+            continue;
+          }
+
+          file_list->push_back(DirectoryEntry(
+              it->first, DirectoryEntry::FILE, info.size, info.last_modified));
+        }
       }
 
       if (components[0] == kPicasaDirFolders) {
@@ -254,13 +316,25 @@ base::PlatformFileError PicasaFileUtil::GetLocalFilePath(
     case 3:
       if (components[0] == kPicasaDirAlbums) {
         scoped_ptr<AlbumMap> album_map = GetDataProvider()->GetAlbums();
+        AlbumInfo album_info;
         base::PlatformFileError error =
-            FindAlbumInfo(components[1], album_map.get(), NULL);
+            FindAlbumInfo(components[1], album_map.get(), &album_info);
         if (error != base::PLATFORM_FILE_OK)
           return error;
 
-        // TODO(tommycli): Implement album contents.
-        return base::PLATFORM_FILE_ERROR_NOT_FOUND;
+        scoped_ptr<AlbumImagesMap> album_images_map =
+            GetDataProvider()->GetAlbumsImages();

[Greg Billock, 2013/08/21 17:13:17]

Is this file missing from the change? I don't see GetAlbumsImages in the class.

[tommycli, 2013/08/21 21:37:06]

Yes, this CL depends on https://codereview.chromium.org/18986012/. I updated the
description. Sorry.

+        const AlbumImages* album_images =
+            FindAlbumImages(album_info.uid, album_images_map.get(), &error);
+        if (error != base::PLATFORM_FILE_OK)
+          return error;
+
+        AlbumImages::const_iterator it = album_images->find(components[2]);
+        if (it == album_images->end())
+          return base::PLATFORM_FILE_ERROR_NOT_FOUND;
+
+        *local_file_path = it->second;
+        return base::PLATFORM_FILE_OK;
       }
 
       if (components[0] == kPicasaDirFolders) {