Media Galleries API - Picasa: Change PMP Parsing to deal with PlatformFile.

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

Index: chrome/browser/media_galleries/fileapi/picasa/picasa_album_table_reader.cc
diff --git a/chrome/browser/media_galleries/fileapi/picasa/picasa_album_table_reader.cc b/chrome/browser/media_galleries/fileapi/picasa/picasa_album_table_reader.cc
index e32e390ddc68d6c27fda89908b2d48ea76fb4dbd..ba92908cd6c20bbb3d7cf27abfb4e2e886496cec 100644
--- a/chrome/browser/media_galleries/fileapi/picasa/picasa_album_table_reader.cc
+++ b/chrome/browser/media_galleries/fileapi/picasa/picasa_album_table_reader.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/browser/media_galleries/fileapi/picasa/picasa_album_table_reader.h"
 
+#include <algorithm>
 #include <vector>
 
 #include "base/path_service.h"
@@ -11,7 +12,6 @@
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/media_galleries/fileapi/picasa/pmp_column_reader.h"
 #include "chrome/browser/media_galleries/fileapi/picasa/pmp_constants.h"
-#include "chrome/browser/media_galleries/fileapi/picasa/pmp_table_reader.h"
 
 namespace picasa {
 
@@ -25,6 +25,19 @@ base::Time TimeFromMicrosoftVariantTime(double variant_time) {
   return base::Time::FromLocalExploded(kPicasaVariantTimeEpoch) + variant_delta;
 }
 
+base::PlatformFile MakePlatformFile(const base::FilePath& directory_path,
+                                    const std::string& suffix) {
+  base::FilePath path = directory_path.Append(base::FilePath::FromUTF8Unsafe(
+      std::string(kPicasaAlbumTableName) + "_" + suffix));
+  int flags = base::PLATFORM_FILE_OPEN | base::PLATFORM_FILE_READ;
+  return base::CreatePlatformFile(path, flags, NULL, NULL);

[Greg Billock, 2013/06/20 22:06:38]

Is this guaranteed not to open the file? Looking at the implementation, it seems
like probably not, but it's a bit hard to tell. It seems like the struct being
built ought to be filenames, and make the PlatformFile's when they get opened.

[tommycli, 2013/06/20 23:14:46]

Yes, the PlatformFiles are opened on the constructor that takes the filenames.
They are closed via CloseAll (which I just added).

+}
+
+base::PlatformFile MakeColumnPlatformFile(const base::FilePath& directory_path,
+                                          const std::string& column_name) {
+  return MakePlatformFile(directory_path, column_name + "." + kPmpExtension);
+}
+
 }  // namespace
 
 AlbumInfo::AlbumInfo() {}
@@ -39,10 +52,22 @@ AlbumInfo::AlbumInfo(const std::string& name, const base::Time& timestamp,
 
 AlbumInfo::~AlbumInfo() {}
 
+PicasaAlbumTableFiles::PicasaAlbumTableFiles(
+    const base::FilePath& directory_path) {
+  indicator_file = MakePlatformFile(directory_path, "0");
+  category_file  = MakeColumnPlatformFile(directory_path, "category");
+  date_file      = MakeColumnPlatformFile(directory_path, "date");
+  filename_file  = MakeColumnPlatformFile(directory_path, "filename");
+  name_file      = MakeColumnPlatformFile(directory_path, "name");
+  token_file     = MakeColumnPlatformFile(directory_path, "token");
+  uid_file       = MakeColumnPlatformFile(directory_path, "uid");
+}
+
 PicasaAlbumTableReader::PicasaAlbumTableReader(
-    const base::FilePath& directory_path)
-    : directory_path_(directory_path),
-      initialized_(false) {}
+    const PicasaAlbumTableFiles& table_files)
+    : table_files_(table_files),
+      initialized_(false) {
+}
 
 PicasaAlbumTableReader::~PicasaAlbumTableReader() {}
 
@@ -60,33 +85,37 @@ bool PicasaAlbumTableReader::Init() {
   if (initialized_)
     return true;
 
-  PmpTableReader pmp_reader(kPicasaAlbumTableName, directory_path_);
-
-  const PmpColumnReader* category_column =
-      pmp_reader.AddColumn("category", PMP_TYPE_UINT32);
-  const PmpColumnReader* date_column =
-      pmp_reader.AddColumn("date", PMP_TYPE_DOUBLE64);
-  const PmpColumnReader* filename_column =
-      pmp_reader.AddColumn("filename", PMP_TYPE_STRING);
-  const PmpColumnReader* name_column =
-      pmp_reader.AddColumn("name", PMP_TYPE_STRING);
-  const PmpColumnReader* token_column =
-      pmp_reader.AddColumn("token", PMP_TYPE_STRING);
-  const PmpColumnReader* uid_column =
-      pmp_reader.AddColumn("uid", PMP_TYPE_STRING);
-
-  if (pmp_reader.Columns().size() != 6)
+  if (table_files_.indicator_file == base::kInvalidPlatformFileValue)
+    return false;
+
+  PmpColumnReader category_column, date_column, filename_column, name_column,
+                  token_column, uid_column;
+  if (!category_column.Init(table_files_.category_file, PMP_TYPE_UINT32) ||

[Greg Billock, 2013/06/20 22:06:38]

Seeing this here, I think "ReadColumn" or something like that would be better
than Init().

[tommycli, 2013/06/20 23:14:46]

Done.

+      !date_column.Init(table_files_.date_file, PMP_TYPE_DOUBLE64) ||
+      !filename_column.Init(table_files_.filename_file, PMP_TYPE_STRING) ||
+      !name_column.Init(table_files_.name_file, PMP_TYPE_STRING) ||
+      !token_column.Init(table_files_.token_file, PMP_TYPE_STRING) ||
+      !uid_column.Init(table_files_.uid_file, PMP_TYPE_STRING)) {
     return false;
+  }
+
+  uint32 row_count = 0;
+  row_count = std::max(row_count, category_column.rows());

[Greg Billock, 2013/06/20 22:06:38]

Given that !reader.ReadXxx(i) -> continue; below, should this really be
std::min?

[tommycli, 2013/06/20 23:14:46]

You are technically correct in that we could probably optimize here. However,
conceptually, PMP table length is determined by the longest column. I added some
comments.

+  row_count = std::max(row_count, date_column.rows());
+  row_count = std::max(row_count, filename_column.rows());
+  row_count = std::max(row_count, name_column.rows());
+  row_count = std::max(row_count, token_column.rows());
+  row_count = std::max(row_count, uid_column.rows());
 
-  for (uint32 i = 0; i < pmp_reader.RowCount(); i++) {
+  for (uint32 i = 0; i < row_count; i++) {
     uint32 category = kAlbumCategoryInvalid;
     double date = 0;
     std::string name;
     std::string uid;
-    if (!category_column->ReadUInt32(i, &category) ||
-        !date_column->ReadDouble64(i, &date) ||
-        !name_column->ReadString(i, &name) || name.empty() ||
-        !uid_column->ReadString(i, &uid) || uid.empty()) {
+    if (!category_column.ReadUInt32(i, &category) ||
+        !date_column.ReadDouble64(i, &date) ||
+        !name_column.ReadString(i, &name) || name.empty() ||
+        !uid_column.ReadString(i, &uid) || uid.empty()) {
       continue;
     }
 
@@ -95,7 +124,7 @@ bool PicasaAlbumTableReader::Init() {
     switch (category) {
       case kAlbumCategoryAlbum: {
         std::string token;
-        if (!token_column->ReadString(i, &token) || token.empty() ||
+        if (!token_column.ReadString(i, &token) || token.empty() ||
             !StartsWithASCII(token, kAlbumTokenPrefix, false)) {
           continue;
         }
@@ -105,7 +134,7 @@ bool PicasaAlbumTableReader::Init() {
       }
       case kAlbumCategoryFolder: {
         std::string filename;
-        if (!filename_column->ReadString(i, &filename) || filename.empty())
+        if (!filename_column.ReadString(i, &filename) || filename.empty())
           continue;
 
         base::FilePath path =