Simple PMP reader to parse Picasa's metadata

webkit/fileapi/media/picasa/pmp_column_reader.h

+// Copyright (c) 2013 The Chromium Authors. All rights reserved.

[vandebo (ex-Chrome), 2013/03/26 01:12:14]

As of 8/12 the copyright format changed, there shouldn't be an "(c)" in there.

[tommycli, 2013/03/26 22:30:43]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef WEBKIT_FILEAPI_MEDIA_PICASA_PMP_COLUMN_READER_H_
+#define WEBKIT_FILEAPI_MEDIA_PICASA_PMP_COLUMN_READER_H_
+
+#include <string>
+#include <vector>
+
+#include "base/basictypes.h"
+
+namespace base {
+  class FilePath;

[vandebo (ex-Chrome), 2013/03/26 01:12:14]

nit: namespace does not indent.

[tommycli, 2013/03/26 22:30:43]

Done.

+}
+
+namespace fileapi {
+
+// Parses a PMP bytestream from either a file or directly from memory
+class PmpColumnReader {
+ public:
+  PmpColumnReader(std::string column_name);
+  virtual ~PmpColumnReader();
+
+  // Returns true if read successfully. rows_read is undefined if returns false

[vandebo (ex-Chrome), 2013/03/26 01:12:14]

|rows_read| and add a '.' at the end.  Comments should be full sentences
(multiple instances).

[tommycli, 2013/03/26 22:30:43]

Done.

+  bool ReadFromDisk(uint32* rows_read, const base::FilePath& filepath);

[vandebo (ex-Chrome), 2013/03/26 01:12:14]

Do you use both ReadFromDisk and ReadFromMemory?  You probably only need one.

[tommycli, 2013/03/26 22:30:43]

Makes sense. I consolidated ReadFromDisk functionality into PmpTableReader.

+  bool ReadFromMemory(uint32* rows_read, const uint8* data, const size_t len);
+
+  template<class T> bool Read(uint32 row, T* target);

[vandebo (ex-Chrome), 2013/03/26 01:12:14]

Probably want a family of methods here instead of a template.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Overl...

Only add the ones that you'll actually use.

[tommycli, 2013/03/26 22:30:43]

Yeah, this way is way more readable. I was able to keep the code succinct by
moving the templated version to private.

+
+  const std::string column_name_;

[vandebo (ex-Chrome), 2013/03/26 01:12:14]

Data members should be private.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Access_Control

[tommycli, 2013/03/26 22:30:43]

Done.

+
+ private:
+  bool ParseData(uint32* rows_read);
+  bool IndexStrings();
+  template<class T> bool SetAndValidateFieldsize();
+
+  uint8* data_;
+  size_t length_;
+  uint32 rows_;
+
+  // Store locations to strings if the data type is strings
+  std::vector<const char*> strings_;
+
+  // Size in bytes of each field. -1 if string type or uninitialized
+  int fieldsize_;
+
+  DISALLOW_COPY_AND_ASSIGN(PmpColumnReader);
+};
+
+template<> bool PmpColumnReader::Read(uint32 row, std::string* target);

[vandebo (ex-Chrome), 2013/03/26 01:12:14]

You're only read string columns?  If that's all you'll need to support, do just
that.  No need to make things more complicated than they have to be.

[tommycli, 2013/03/26 22:30:43]

This was here to declare a custom specialization for strings. The new way is
better though.

+
+}  // namespace fileapi
+
+#endif  // WEBKIT_FILEAPI_MEDIA_PICASA_PMP_COLUMN_READER_H_