PicasaAlbumTableReader for Media Galleries API

webkit/fileapi/media/picasa/pmp_table_reader.cc

Index: webkit/fileapi/media/picasa/pmp_table_reader.cc
diff --git a/webkit/fileapi/media/picasa/pmp_table_reader.cc b/webkit/fileapi/media/picasa/pmp_table_reader.cc
index af50530f883454f6028e62a0e82baa2f1c2a9458..a8e16542c8d1d9185e025383018182df3baacf67 100644
--- a/webkit/fileapi/media/picasa/pmp_table_reader.cc
+++ b/webkit/fileapi/media/picasa/pmp_table_reader.cc
@@ -7,11 +7,10 @@
 #include <algorithm>
 
 #include "base/file_util.h"
-#include "base/files/file_path.h"
 #include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/utf_string_conversions.h"
 #include "webkit/fileapi/media/picasa/pmp_column_reader.h"
-#include "webkit/fileapi/media/picasa/pmp_constants.h"
 
 namespace picasaimport {
 
@@ -21,68 +20,56 @@ COMPILE_ASSERT(sizeof(double) == 8, double_must_be_8_bytes_long);
 
 }  // namespace
 
-PmpTableReader::PmpTableReader() : column_readers_(), max_row_count_(0) { }
+PmpTableReader::PmpTableReader(const std::string& table_name,
+                               const base::FilePath& directory_path)
+    : table_name_(table_name),
+      directory_path_(directory_path),
+      column_readers_(),
+      max_row_count_(0) { }
 
 PmpTableReader::~PmpTableReader() { }
 
-bool PmpTableReader::Init(const std::string& table_name,
-                          const base::FilePath& directory_path,
-                          const std::vector<std::string>& columns) {
-  DCHECK(!columns.empty());
-
-  if (!column_readers_.empty())
-    return false;
-
-  if (!file_util::DirectoryExists(directory_path))
+bool PmpTableReader::Init() {

[vandebo (ex-Chrome), 2013/04/08 23:57:24]

Maybe just put the contents on Init into the constructor.  If init fails, then
set some state that you can detect in AddColumn, then fail immediately in
AddColumn if Init would have failed ?

[tommycli, 2013/04/09 00:45:34]

I just added a 
  base::ThreadRestrictions::AssertIOAllowed();
to Init(). Is that safe to put in a constructor? Should I just add a boolean
member to keep track of whether or not construction succeeded?

[vandebo (ex-Chrome), 2013/04/09 19:16:56]

If the compiler is happy, I'm happy.

A Boolean is fine unless there's another member that you want to overload.

[tommycli, 2013/04/11 18:24:05]

Done.

+  if (!file_util::DirectoryExists(directory_path_))
     return false;
 
-  std::string table_prefix = table_name + "_";
-  std::string indicator_file_name = table_prefix + "0";
+  std::string indicator_file_name = table_name_ + "_0";
 
 #if defined(OS_WIN)
-  base::FilePath indicator_file = directory_path.Append(
+  base::FilePath indicator_file = directory_path_.Append(
       UTF8ToUTF16(indicator_file_name));
 #else
   base::FilePath indicator_file =
-      directory_path.Append(indicator_file_name);
+      directory_path_.Append(indicator_file_name);
 #endif
 
   // Look for the indicator_file file, indicating table existence.
-  if (!file_util::PathExists(indicator_file) ||
-      file_util::DirectoryExists(indicator_file)) {
-    return false;
-  }
-
-  ScopedVector<PmpColumnReader> column_readers;
-  uint32 max_row_count = 0;
+  return file_util::PathExists(indicator_file) &&
+         !file_util::DirectoryExists(indicator_file);
+}
 
-  for (std::vector<std::string>::const_iterator it = columns.begin();
-       it != columns.end(); ++it) {
-    std::string filename = table_prefix + *it + "." + kPmpExtension;
+const PmpColumnReader* PmpTableReader::AddColumn(
+    const std::string& column_name, const PmpFieldType expected_type) {
+  std::string filename = table_name_ + "_" + column_name + "." + kPmpExtension;
 
 #if defined(OS_WIN)
-    base::FilePath column_file_path = directory_path.Append(
-        UTF8ToUTF16(filename));
+  base::FilePath column_file_path = directory_path_.Append(
+      UTF8ToUTF16(filename));
 #else
-    base::FilePath column_file_path = directory_path.Append(filename);
+  base::FilePath column_file_path = directory_path_.Append(filename);
 #endif
 
+  scoped_ptr<PmpColumnReader> column_reader(new PmpColumnReader());
 
-    PmpColumnReader* column_reader = new PmpColumnReader();
-    column_readers.push_back(column_reader);
-
-    uint32 row_cnt;
-
-    if (!column_reader->Init(column_file_path, &row_cnt))
-      return false;
+  uint32 row_count;
+  if (!column_reader->Init(column_file_path, expected_type, &row_count))
+    return NULL;
 
-    max_row_count = std::max(max_row_count, row_cnt);
-  }
+  column_readers_.push_back(column_reader.release());
 
-  column_readers_ = column_readers.Pass();
-  max_row_count_ = max_row_count;
+  max_row_count_ = std::max(max_row_count_, row_count);
 
-  return true;
+  return column_readers_.back();
 }
 
 uint32 PmpTableReader::RowCount() const {