Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12704024:
Simple PMP reader to parse Picasa's metadata (Closed)
|
Created:
7 years, 9 months ago by tommycli Modified:
7 years, 8 months ago CC:
chromium-reviews, tzik+watch_chromium.org, jam, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, kinuko+watch Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionSimple PMP reader to parse Picasa's metadata
For Media Galleries project.
BUG=151701
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=193537
Patch Set 1 #Patch Set 2 : Fix memory issue. #
Total comments: 38
Patch Set 3 : Fix on some memory issues and table reading. #Patch Set 4 : Add explicit keywords. #Patch Set 5 : Address vandebo's comments. #Patch Set 6 : Formatting fixes. #Patch Set 7 : More formatting/ style changes. #
Total comments: 46
Patch Set 8 : Address vandebo's comments. Design fixes. #Patch Set 9 : Change to ScopedVector. #Patch Set 10 : Formatting fix. #Patch Set 11 : Fix rvalue usage in move operation. #
Total comments: 56
Patch Set 12 : Add field type accessor. Make naming uniform. #Patch Set 13 : Add test for PmpTableReader. Address vandebo's comments. #Patch Set 14 : Simplify table reader by making it non-reusable. #Patch Set 15 : Remove unused constant. #
Total comments: 80
Patch Set 16 : Simplify and make names more uniform. Remove comments. #Patch Set 17 : Assert on an IO allowed thread. #Patch Set 18 : Missed a change. #
Total comments: 10
Patch Set 19 : Change namespace to picasaimport. Minor fixes. #Patch Set 20 : Make PmpTestHelper stateful and possess the temporary directory. #
Total comments: 12
Patch Set 21 : Add Win specific code. #Patch Set 22 : Fix linux_clang and linux_rel builds. #Patch Set 23 : Fix mac build. #Patch Set 24 : Fix windows builds probably. #Patch Set 25 : Add missing include #Patch Set 26 : Add more windows specific code. #Patch Set 27 : Windows file paths. #Patch Set 28 : Add missing include #
Total comments: 8
Patch Set 29 : Address vandebo's comments #
Total comments: 22
Patch Set 30 : Remove Win specific defines and fix up PmpFieldType usage. #Messages
Total messages: 25 (0 generated)
Hi Steve, Can I get a first pass review on this?
https://codereview.chromium.org/12704024/diff/2001/content/content_tests.gypi File content/content_tests.gypi (right): https://codereview.chromium.org/12704024/diff/2001/content/content_tests.gypi... content/content_tests.gypi:469: '../webkit/fileapi/media/picasa/pmp_column_reader.cc', You don't want pmp_column_reader.* in here, unless those are test only. https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... File webkit/fileapi/media/picasa/pmp_column_reader.cc (right): https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... webkit/fileapi/media/picasa/pmp_column_reader.cc:23: nit: extra line https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... webkit/fileapi/media/picasa/pmp_column_reader.cc:27: delete[] data_; This means you probably want to use a scoped object as the data member instead. Maybe scoped_array<T> in base/memory/scoped_ptr.h. https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... webkit/fileapi/media/picasa/pmp_column_reader.cc:38: file.length() > 50*1024*1024) { Make this a constant as well. https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... webkit/fileapi/media/picasa/pmp_column_reader.cc:77: memcpy(target, data_ + kPmpHeaderSize + row*fieldsize_, fieldsize_); You probably want an explicit reinterpret_cast in here to make it clear what's going on instead of hiding it in the memcpy. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Casting https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... webkit/fileapi/media/picasa/pmp_column_reader.cc:111: if (memcmp(kPmpMagicO0S4, data_, 4) != 0|| You may want to make a packed struct of the different header fields. https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... webkit/fileapi/media/picasa/pmp_column_reader.cc:133: case 0x00: Make these constants. https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... webkit/fileapi/media/picasa/pmp_column_reader.cc:171: const uint8* itr = data_ + kPmpHeaderSize; How is the string encoded? UTF8? unspecified? https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... webkit/fileapi/media/picasa/pmp_column_reader.cc:175: memchr(itr, '\0', length_-bytes_parsed)); spaces around math operators. https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... webkit/fileapi/media/picasa/pmp_column_reader.cc:177: // fail if cannot find null termination. String runs on past file end. Capitalize https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... webkit/fileapi/media/picasa/pmp_column_reader.cc:178: if(ptr_to_str_end == NULL) { {}'s around single line conditionals with single line bodies are optional. Precedent is set by other instances in the file or the directory. webkit/fileapi doesn't seem to use them. https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... webkit/fileapi/media/picasa/pmp_column_reader.cc:206: } else { No else in this case. https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... File webkit/fileapi/media/picasa/pmp_column_reader.h (right): https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... webkit/fileapi/media/picasa/pmp_column_reader.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. As of 8/12 the copyright format changed, there shouldn't be an "(c)" in there. https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... webkit/fileapi/media/picasa/pmp_column_reader.h:14: class FilePath; nit: namespace does not indent. https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... webkit/fileapi/media/picasa/pmp_column_reader.h:25: // Returns true if read successfully. rows_read is undefined if returns false |rows_read| and add a '.' at the end. Comments should be full sentences (multiple instances). https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... webkit/fileapi/media/picasa/pmp_column_reader.h:26: bool ReadFromDisk(uint32* rows_read, const base::FilePath& filepath); Do you use both ReadFromDisk and ReadFromMemory? You probably only need one. https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... webkit/fileapi/media/picasa/pmp_column_reader.h:29: template<class T> bool Read(uint32 row, T* target); 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. https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... webkit/fileapi/media/picasa/pmp_column_reader.h:31: const std::string column_name_; Data members should be private. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Access_Control https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... webkit/fileapi/media/picasa/pmp_column_reader.h:51: template<> bool PmpColumnReader::Read(uint32 row, std::string* target); 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.
https://codereview.chromium.org/12704024/diff/2001/content/content_tests.gypi File content/content_tests.gypi (right): https://codereview.chromium.org/12704024/diff/2001/content/content_tests.gypi... content/content_tests.gypi:469: '../webkit/fileapi/media/picasa/pmp_column_reader.cc', On 2013/03/26 01:12:14, vandebo wrote: > You don't want pmp_column_reader.* in here, unless those are test only. Ohhhh.... I just had to add WEBKIT_STORAGE_EXPORT_PRIVATE to make the class visible during component builds. Cool. https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... File webkit/fileapi/media/picasa/pmp_column_reader.cc (right): https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... webkit/fileapi/media/picasa/pmp_column_reader.cc:23: On 2013/03/26 01:12:14, vandebo wrote: > nit: extra line Done. https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... webkit/fileapi/media/picasa/pmp_column_reader.cc:27: delete[] data_; On 2013/03/26 01:12:14, vandebo wrote: > This means you probably want to use a scoped object as the data member instead. > Maybe scoped_array<T> in base/memory/scoped_ptr.h. Done. https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... webkit/fileapi/media/picasa/pmp_column_reader.cc:38: file.length() > 50*1024*1024) { On 2013/03/26 01:12:14, vandebo wrote: > Make this a constant as well. Done. https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... webkit/fileapi/media/picasa/pmp_column_reader.cc:77: memcpy(target, data_ + kPmpHeaderSize + row*fieldsize_, fieldsize_); On 2013/03/26 01:12:14, vandebo wrote: > You probably want an explicit reinterpret_cast in here to make it clear what's > going on instead of hiding it in the memcpy. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Casting Done. https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... webkit/fileapi/media/picasa/pmp_column_reader.cc:111: if (memcmp(kPmpMagicO0S4, data_, 4) != 0|| On 2013/03/26 01:12:14, vandebo wrote: > You may want to make a packed struct of the different header fields. That's a good idea. I may need some help figuring out how to do this in a Chromium/x-platform way, since the unittests are currently run on every platform. https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... webkit/fileapi/media/picasa/pmp_column_reader.cc:133: case 0x00: On 2013/03/26 01:12:14, vandebo wrote: > Make these constants. Done. https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... webkit/fileapi/media/picasa/pmp_column_reader.cc:171: const uint8* itr = data_ + kPmpHeaderSize; On 2013/03/26 01:12:14, vandebo wrote: > How is the string encoded? UTF8? unspecified? Unspecified. I've sent an email to the Picasa ppl asking though. https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... webkit/fileapi/media/picasa/pmp_column_reader.cc:175: memchr(itr, '\0', length_-bytes_parsed)); On 2013/03/26 01:12:14, vandebo wrote: > spaces around math operators. Done. https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... webkit/fileapi/media/picasa/pmp_column_reader.cc:177: // fail if cannot find null termination. String runs on past file end. On 2013/03/26 01:12:14, vandebo wrote: > Capitalize Done. https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... webkit/fileapi/media/picasa/pmp_column_reader.cc:178: if(ptr_to_str_end == NULL) { On 2013/03/26 01:12:14, vandebo wrote: > {}'s around single line conditionals with single line bodies are optional. > Precedent is set by other instances in the file or the directory. > webkit/fileapi doesn't seem to use them. Done. https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... webkit/fileapi/media/picasa/pmp_column_reader.cc:206: } else { On 2013/03/26 01:12:14, vandebo wrote: > No else in this case. Done. https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... File webkit/fileapi/media/picasa/pmp_column_reader.h (right): https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... webkit/fileapi/media/picasa/pmp_column_reader.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/03/26 01:12:14, vandebo wrote: > As of 8/12 the copyright format changed, there shouldn't be an "(c)" in there. Done. https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... webkit/fileapi/media/picasa/pmp_column_reader.h:14: class FilePath; On 2013/03/26 01:12:14, vandebo wrote: > nit: namespace does not indent. Done. https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... webkit/fileapi/media/picasa/pmp_column_reader.h:25: // Returns true if read successfully. rows_read is undefined if returns false On 2013/03/26 01:12:14, vandebo wrote: > |rows_read| and add a '.' at the end. Comments should be full sentences > (multiple instances). Done. https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... webkit/fileapi/media/picasa/pmp_column_reader.h:26: bool ReadFromDisk(uint32* rows_read, const base::FilePath& filepath); On 2013/03/26 01:12:14, vandebo wrote: > Do you use both ReadFromDisk and ReadFromMemory? You probably only need one. Makes sense. I consolidated ReadFromDisk functionality into PmpTableReader. https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... webkit/fileapi/media/picasa/pmp_column_reader.h:29: template<class T> bool Read(uint32 row, T* target); On 2013/03/26 01:12:14, vandebo wrote: > 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. Yeah, this way is way more readable. I was able to keep the code succinct by moving the templated version to private. https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... webkit/fileapi/media/picasa/pmp_column_reader.h:31: const std::string column_name_; On 2013/03/26 01:12:14, vandebo wrote: > Data members should be private. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Access_Control Done. https://codereview.chromium.org/12704024/diff/2001/webkit/fileapi/media/picas... webkit/fileapi/media/picasa/pmp_column_reader.h:51: template<> bool PmpColumnReader::Read(uint32 row, std::string* target); On 2013/03/26 01:12:14, vandebo wrote: > 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. This was here to declare a custom specialization for strings. The new way is better though.
https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_column_reader.cc (right): https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:27: if(len < kPmpHeaderSize || data == NULL) { nit: omit {}'s here. https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:38: bool PmpColumnReader::ReadString(const uint32 row, std::string* target) const { const std::string* https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:42: if(row < rows_) { Check the field type? https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:165: if(bytes_parsed != length_) return false; return false on next line. https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_column_reader.h (right): https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.h:21: // Parses a PMP bytestream from either a file or directly from memory. nit: update or remove comment. https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.h:28: bool ReadFromMemory(uint32* rows_read, const uint8* data, const size_t len); output paramaters should go at the end of the list, so: bool ReadFromMemory(const uint8* data, const size_t len, uint32* rows_read); Does the "spec" have a limit on the number of rows? If so, you could return a signed 32 bit int and use negative for errors. https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.h:30: // Family of functions to read fields. A bit more description wouldn't hurt here. Only one type per column right? So... "Read a row from this column. These return false if the column isn't of the requested type or the requested row is out of range." A better name for the second arg might be result instead of target. https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_constants.h (right): https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_constants.h:21: const uint8 kPmpMagicO0S4[] = { 0xcd, 0xcc, 0xcc, 0x3f }; The 00S4 nomenclature isn't like anything else I've seen in Chrome. I think you just want something like this: const int kPmpMagic1Offset = 0 const int kPmpFieldTypeOffset = 4 const int kPmpMagic2Offset = 6 ... const uint32 kPmpMagic1 = 0xCDCCCC3F; const uint16 kPmpMagic2 = 0x3213 ... https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_constants.h:44: // 0x00 - cstrings Comments below aren't necessary, the code describes what's going on pretty well. For things like this, you can indent the values so they all line up. https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_table_reader.cc (right): https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:19: COMPILE_ASSERT(sizeof(double) == 8, double_must_be_8_bytes_long); no indent on namespace, blank lines before and after. https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:23: using std::string; we don't "use" anything from std. https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:26: nit: extra line. https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:40: if(!file_util::DirectoryExists(directory_path)) { nit: no {}s https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:45: string pmp_suffix = ".pmp"; Maybe this should go in the constants file? https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:62: base::FilePath colfile_path = directory_path.Append( nit: column_file_path https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:65: // Constructed on the heap due to PmpColumnReader disallowing Not sure this comment is necessary. https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:71: uint32 row_cnt; // row count unnecessary comment. https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:77: !column_reader->ReadFromMemory(&row_cnt, file.data(), file.length())) { This configuration forces a copy. If you just pass the file name, the column reader probably doesn't have to make a copy of the bytes, it can just use the mmap'd file. https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_table_reader.h (right): https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.h:23: explicit PmpTableReader(); explicit is only needed for exactly one arg. https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.h:30: bool ReadFromDisk( Why not put all these args in the constructor? https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.h:30: bool ReadFromDisk( If this interface reads from disk, it seems like the column reader should also read from disk? https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.h:39: uint32 Count() const; ColumnCount? https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.h:45: std::vector<PmpColumnReader*> column_readers_; probably want to use base/memory/scoped_vector.h here.
https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_column_reader.cc (right): https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:27: if(len < kPmpHeaderSize || data == NULL) { On 2013/03/27 00:06:19, vandebo wrote: > nit: omit {}'s here. Done. https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:38: bool PmpColumnReader::ReadString(const uint32 row, std::string* target) const { On 2013/03/27 00:06:19, vandebo wrote: > const std::string* I use operator= on it. https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:42: if(row < rows_) { On 2013/03/27 00:06:19, vandebo wrote: > Check the field type? Done. https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:165: if(bytes_parsed != length_) return false; On 2013/03/27 00:06:19, vandebo wrote: > return false on next line. Done. https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_column_reader.h (right): https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.h:21: // Parses a PMP bytestream from either a file or directly from memory. On 2013/03/27 00:06:19, vandebo wrote: > nit: update or remove comment. Done. https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.h:28: bool ReadFromMemory(uint32* rows_read, const uint8* data, const size_t len); On 2013/03/27 00:06:19, vandebo wrote: > output paramaters should go at the end of the list, so: > bool ReadFromMemory(const uint8* data, const size_t len, uint32* rows_read); > > Does the "spec" have a limit on the number of rows? If so, you could return a > signed 32 bit int and use negative for errors. Number of rows is defined as a uint32. We will run into the 50MB file limit way before that, of course. I'll keep it as a bool for now to make it consistent will all the other methods. https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.h:30: // Family of functions to read fields. On 2013/03/27 00:06:19, vandebo wrote: > A bit more description wouldn't hurt here. Only one type per column right? > So... "Read a row from this column. These return false if the column isn't of > the requested type or the requested row is out of range." > > A better name for the second arg might be result instead of target. > Done. https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_constants.h (right): https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_constants.h:21: const uint8 kPmpMagicO0S4[] = { 0xcd, 0xcc, 0xcc, 0x3f }; On 2013/03/27 00:06:19, vandebo wrote: > The 00S4 nomenclature isn't like anything else I've seen in Chrome. > > I think you just want something like this: > > const int kPmpMagic1Offset = 0 > const int kPmpFieldTypeOffset = 4 > const int kPmpMagic2Offset = 6 > ... > > const uint32 kPmpMagic1 = 0xCDCCCC3F; > const uint16 kPmpMagic2 = 0x3213 > ... Cool that's way better. https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_constants.h:44: // 0x00 - cstrings On 2013/03/27 00:06:19, vandebo wrote: > Comments below aren't necessary, the code describes what's going on pretty well. > For things like this, you can indent the values so they all line up. Done. https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_table_reader.cc (right): https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:19: COMPILE_ASSERT(sizeof(double) == 8, double_must_be_8_bytes_long); On 2013/03/27 00:06:19, vandebo wrote: > no indent on namespace, blank lines before and after. Done. https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:23: using std::string; On 2013/03/27 00:06:19, vandebo wrote: > we don't "use" anything from std. Done. https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:26: On 2013/03/27 00:06:19, vandebo wrote: > nit: extra line. Done. https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:40: if(!file_util::DirectoryExists(directory_path)) { On 2013/03/27 00:06:19, vandebo wrote: > nit: no {}s Done. https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:45: string pmp_suffix = ".pmp"; On 2013/03/27 00:06:19, vandebo wrote: > Maybe this should go in the constants file? Done. https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:62: base::FilePath colfile_path = directory_path.Append( On 2013/03/27 00:06:19, vandebo wrote: > nit: column_file_path Done. https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:65: // Constructed on the heap due to PmpColumnReader disallowing On 2013/03/27 00:06:19, vandebo wrote: > Not sure this comment is necessary. Done. https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:71: uint32 row_cnt; // row count On 2013/03/27 00:06:19, vandebo wrote: > unnecessary comment. Done. https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:77: !column_reader->ReadFromMemory(&row_cnt, file.data(), file.length())) { On 2013/03/27 00:06:19, vandebo wrote: > This configuration forces a copy. If you just pass the file name, the column > reader probably doesn't have to make a copy of the bytes, it can just use the > mmap'd file. OBE. https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_table_reader.h (right): https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.h:23: explicit PmpTableReader(); On 2013/03/27 00:06:19, vandebo wrote: > explicit is only needed for exactly one arg. Done. https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.h:30: bool ReadFromDisk( On 2013/03/27 00:06:19, vandebo wrote: > Why not put all these args in the constructor? Done. https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.h:30: bool ReadFromDisk( On 2013/03/27 00:06:19, vandebo wrote: > If this interface reads from disk, it seems like the column reader should also > read from disk? Done. https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.h:39: uint32 Count() const; On 2013/03/27 00:06:19, vandebo wrote: > ColumnCount? Done. https://codereview.chromium.org/12704024/diff/21002/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.h:45: std::vector<PmpColumnReader*> column_readers_; On 2013/03/27 00:06:19, vandebo wrote: > probably want to use base/memory/scoped_vector.h here. Yep! That's what I want. Btw - awesome that it implements a 'move' in C++03.
https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_column_reader.cc (right): https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:52: DCHECK(length_ > kPmpHeaderSize); DCHECK_GT https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:57: } else { Remove else and just return false at the end. https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:64: ReadPrimitive<uint32>(row, result)); Once you get everything else taken care, can you try to implement all this stuff without any templates? https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:88: *result = *(reinterpret_cast<T*>( Is this simpler? *result = reinterpret_cast<T*>(data_.get() + kPmPHeaderSize)[row]; https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:91: } else { remove else. and return false at the end. https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:101: if (memcmp(&kPmpMagic1, &data_[kPmpMagic1Offset], sizeof(kPmpMagic1)) != 0 || data_.get() + kPmpMagic1Offset ? https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:109: if (data_[kPmpFieldType1Offset] != data_[kPmpFieldType2Offset]) { This only tests the first byte of these two fields. Is the type a uint16 ? https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:109: if (data_[kPmpFieldType1Offset] != data_[kPmpFieldType2Offset]) { nit: extra {}'s https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:136: parse_success = false; return false; https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:142: if(rows_read != NULL) { nit: extra {}'s https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:153: DCHECK(length_ >= kPmpHeaderSize); DCHECK_GE https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:162: const uint8* ptr_to_str_end = static_cast<const uint8*>( variables should generally not be abbreviations and should not include the type, so string_end, or end_of_string https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:166: if(ptr_to_str_end == NULL) return false; two lines. https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:169: ptrdiff_t len_bytes = ptr_to_str_end - itr + 1; len_bytes = length ? https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:185: unsigned long calculated_n_rows = (length_ - kPmpHeaderSize) / sizeof(T); Also check that there's no remainder ? https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_column_reader.h (right): https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.h:24: explicit PmpColumnReader(std::string column_name); should column_name be part of the Init method? https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.h:28: bool ReadFromFile(const base::FilePath& filepath, uint32* rows_read); nit: InitFromFile ? https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.h:39: const std::string column_name() const { Do you use this? If not, you can probably just remove column name from this class all together. https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.h:47: // Only called from ReadFromMemory(). nit: outdated comment. Probably not needed anyway. https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_column_reader_unittest.cc (right): https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader_unittest.cc:18: std::vector<uint8> MakeHeader(const uint16 field_type, const uint32 row_count) { All these functions might be better organized in a class? But then maybe not. https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader_unittest.cc:65: std::vector<uint8> Combined(const std::vector<uint8>& a, CombineVectors ? https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_table_reader.cc (right): https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:53: ScopedVector<PmpColumnReader> current_col_readers; column_readers https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:54: uint32 current_max_rows_parsed = 0; max_row_count ? https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:67: // If file is not initialized, too small, or too large (50MB), fail out. redundant comment. https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:75: column_readers_.clear(); Do you need to clear before assignment ? https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_table_reader.h (right): https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.h:31: // Warning: Invalidates previously obtained pointers from GetColumns(). It seems that this class doesn't need to be reuseable. Is there any reason to call ReadFromDisk again? Is the only purpose to be able to give an error? How about making this an Init method, putting everything from the constructor here, and returning failure if multiple attempts to init are made?
Also added a unit test for PmpTableReader. https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_column_reader.cc (right): https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:52: DCHECK(length_ > kPmpHeaderSize); On 2013/03/28 00:56:04, vandebo wrote: > DCHECK_GT Done. https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:57: } else { On 2013/03/28 00:56:04, vandebo wrote: > Remove else and just return false at the end. Done. https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:64: ReadPrimitive<uint32>(row, result)); On 2013/03/28 00:56:04, vandebo wrote: > Once you get everything else taken care, can you try to implement all this stuff > without any templates? Since we are only using 4 field types, the best alternative to the template was to just copy-paste the template body and eliminate the template. Kinda worried about the copy-paste though - what do you think? https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:88: *result = *(reinterpret_cast<T*>( On 2013/03/28 00:56:04, vandebo wrote: > Is this simpler? > *result = reinterpret_cast<T*>(data_.get() + kPmPHeaderSize)[row]; Yeah, that's way better thanks. https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:91: } else { On 2013/03/28 00:56:04, vandebo wrote: > remove else. and return false at the end. Done. https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:101: if (memcmp(&kPmpMagic1, &data_[kPmpMagic1Offset], sizeof(kPmpMagic1)) != 0 || On 2013/03/28 00:56:04, vandebo wrote: > data_.get() + kPmpMagic1Offset ? Makes the command no longer fit on one line. Change still desirable? https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:109: if (data_[kPmpFieldType1Offset] != data_[kPmpFieldType2Offset]) { On 2013/03/28 00:56:04, vandebo wrote: > This only tests the first byte of these two fields. Is the type a uint16 ? Whoops, that's just a straight up bug. I've refactored the field_type_ handling to treat it as a uint16. https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:109: if (data_[kPmpFieldType1Offset] != data_[kPmpFieldType2Offset]) { On 2013/03/28 00:56:04, vandebo wrote: > nit: extra {}'s Done. https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:136: parse_success = false; On 2013/03/28 00:56:04, vandebo wrote: > return false; Done. https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:142: if(rows_read != NULL) { On 2013/03/28 00:56:04, vandebo wrote: > nit: extra {}'s Done. https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:153: DCHECK(length_ >= kPmpHeaderSize); On 2013/03/28 00:56:04, vandebo wrote: > DCHECK_GE Done. https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:162: const uint8* ptr_to_str_end = static_cast<const uint8*>( On 2013/03/28 00:56:04, vandebo wrote: > variables should generally not be abbreviations and should not include the type, > so string_end, or end_of_string Done. https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:166: if(ptr_to_str_end == NULL) return false; On 2013/03/28 00:56:04, vandebo wrote: > two lines. Done. https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:169: ptrdiff_t len_bytes = ptr_to_str_end - itr + 1; On 2013/03/28 00:56:04, vandebo wrote: > len_bytes = length ? Done. https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:185: unsigned long calculated_n_rows = (length_ - kPmpHeaderSize) / sizeof(T); On 2013/03/28 00:56:04, vandebo wrote: > Also check that there's no remainder ? Whoops. Totally forgot about the whole integer division thing. https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_column_reader.h (right): https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.h:24: explicit PmpColumnReader(std::string column_name); On 2013/03/28 00:56:04, vandebo wrote: > should column_name be part of the Init method? Done. https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.h:28: bool ReadFromFile(const base::FilePath& filepath, uint32* rows_read); On 2013/03/28 00:56:04, vandebo wrote: > nit: InitFromFile ? Done. https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.h:39: const std::string column_name() const { On 2013/03/28 00:56:04, vandebo wrote: > Do you use this? If not, you can probably just remove column name from this > class all together. Done. https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.h:47: // Only called from ReadFromMemory(). On 2013/03/28 00:56:04, vandebo wrote: > nit: outdated comment. Probably not needed anyway. Done. https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_column_reader_unittest.cc (right): https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader_unittest.cc:18: std::vector<uint8> MakeHeader(const uint16 field_type, const uint32 row_count) { On 2013/03/28 00:56:04, vandebo wrote: > All these functions might be better organized in a class? But then maybe not. Moving these to PmpTestUtil so I can use in multiple tests. https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader_unittest.cc:65: std::vector<uint8> Combined(const std::vector<uint8>& a, On 2013/03/28 00:56:04, vandebo wrote: > CombineVectors ? Done. https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_table_reader.cc (right): https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:53: ScopedVector<PmpColumnReader> current_col_readers; On 2013/03/28 00:56:04, vandebo wrote: > column_readers Done. https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:54: uint32 current_max_rows_parsed = 0; On 2013/03/28 00:56:04, vandebo wrote: > max_row_count ? Done. https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:67: // If file is not initialized, too small, or too large (50MB), fail out. On 2013/03/28 00:56:04, vandebo wrote: > redundant comment. Done. https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:75: column_readers_.clear(); On 2013/03/28 00:56:04, vandebo wrote: > Do you need to clear before assignment ? Done. https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_table_reader.h (right): https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.h:31: // Warning: Invalidates previously obtained pointers from GetColumns(). On 2013/03/28 00:56:04, vandebo wrote: > It seems that this class doesn't need to be reuseable. Is there any reason to > call ReadFromDisk again? Is the only purpose to be able to give an error? How > about making this an Init method, putting everything from the constructor here, > and returning failure if multiple attempts to init are made? Done.
https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_column_reader.cc (right): https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:64: ReadPrimitive<uint32>(row, result)); On 2013/03/29 00:16:01, tommycli wrote: > On 2013/03/28 00:56:04, vandebo wrote: > > Once you get everything else taken care, can you try to implement all this > stuff > > without any templates? > > Since we are only using 4 field types, the best alternative to the template was > to just copy-paste the template body and eliminate the template. > > Kinda worried about the copy-paste though - what do you think? I think the amount of code duplication is small and it makes it much easier to understand what's going on, so an overall win. https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:101: if (memcmp(&kPmpMagic1, &data_[kPmpMagic1Offset], sizeof(kPmpMagic1)) != 0 || On 2013/03/29 00:16:01, tommycli wrote: > On 2013/03/28 00:56:04, vandebo wrote: > > data_.get() + kPmpMagic1Offset ? > > Makes the command no longer fit on one line. Change still desirable? Your call. https://codereview.chromium.org/12704024/diff/61001/content/content_tests.gypi File content/content_tests.gypi (right): https://codereview.chromium.org/12704024/diff/61001/content/content_tests.gyp... content/content_tests.gypi:477: '../webkit/fileapi/media/picasa/pmp_test_helper.cc', pmp_test_utils.* would be a more common name in the Chrome codebase. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_column_reader.cc (right): https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:33: base::MemoryMappedFile file; So using a memory mapped file means you need enough virtual address space to map the entire file. But you really don't need a memory mapped file. How about using GetFileSize() and ReadFile() https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:33: base::MemoryMappedFile file; Add a DCHECK that we're on the FILE thread? https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:35: if(!file.Initialize(filepath) || nit: s/if(/if (/g https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:53: if(field_type_ == kPmpFieldTypeString && row < rows_) { We like early exits of functions, especially when it keeps the indent down... so: if (field_type_ != kPmpFieldTypeString || row >= rows_) return false; ... https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:63: DCHECK_GT(length_, kPmpHeaderSize + row*sizeof(uint32)); nit: space around math operators: row * sizeof() https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:121: // Read the field type nit: remove redundant comment. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:128: // Read the number of rows expected. nit: redundant comment. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:132: nit: extra line. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:154: if(parse_success) { if (parse_success && rows_read) *rows_read = rows_; return parse_success; https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:168: strings_.clear(); not needed https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:172: const uint8* itr = data_.get() + kPmpHeaderSize; nit: We usually use it for iterators... though this isn't actually an iterator. Maybe data? https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:183: ptrdiff_t length_in_bytes = string_end - itr + 1; Any word back from Picasa folks on the string encoding used? https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:190: // This means the file has more bytes at the end we haven't parsed. redundant comment. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:192: return false; return bytes_parsed == length_; https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:198: bool PmpColumnReader::ValidatePrimitiveArrayLength() { I think you can inline this template without much loss. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_column_reader.h (right): https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.h:27: // Returns true if read successfully. |rows_read| undefined if returns false. nit: ...is undefined... https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.h:28: bool InitFromFile(const base::FilePath& filepath, uint32* rows_read); nit: Since there's only one Init method and it's clear from the args that it reads from a file, just call this Init. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.h:30: // These functions read the value of that row into the |result| variable. nit: ..the value of |row| into |result|. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.h:31: // Function returns false if the column is of the wrong type or the row nit: Functions return https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.h:39: const uint16 field_type() const { The native encoding of field_type is returned? Say so in a comment. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.h:48: scoped_array<uint8> data_; This is really nit picky, but is there any organization to your member variables? Maybe grouping like this would make sense? (source data) data_ length_ (header data) field_type_ rows_ (computed info) strings_ https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_column_reader_unittest.cc (right): https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader_unittest.cc:20: bool ReaderReadFromVector(PmpColumnReader* const reader, InitColumnReaderFromMemory ? https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader_unittest.cc:68: for(uint32 i = 0; i < elems.size(); i++) { i < elems.size() && i < rows_read. You've already expected that they're equal. If they aren't, there's no point trying to read beyond the end. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_constants.h (right): https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_constants.h:35: const uint16 kPmpFieldTypeString = 0x00; It might clean up some interfaces if you make this an enum. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_table_reader.cc (right): https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. nit: no (c) https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:30: // Expect to read some columns, otherwise, a programming error. Remove comment. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:33: // Only allow initialization once Remove comment. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:37: // Directory must exist. Remove comment. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:43: // Look for the "%s_0".format(table_name_) file, indicating table existence. Does '"%s_0".format(table_name_)' mean table_name_ + "_0" ? If so, say that. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:55: // Construct the column readers. Remove comment. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:57: it != columns.end(); it++) { ++it https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:72: // Actually succeeded. Set member data. Remove comment. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_table_reader.h (right): https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.h:40: // Contains the last 'good' read of the disk. redundant comment now? https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.h:42: // Contains the number of rows obtained during the last 'good' read of disk. redundant comment now? https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_test_helper.cc (right): https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_test_helper.cc:67: template<class T> I suspect all the fancy mechanisms below are longer than just writing two versions of MakeHeaderAndBody. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_test_helper.h (right): https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_test_helper.h:21: class PmpTestHelper { You might want to make this a functional class. In the constructor make the temp dir and then have a method to write a column based on data or write a column based on bytes....
https://codereview.chromium.org/12704024/diff/61001/content/content_tests.gypi File content/content_tests.gypi (right): https://codereview.chromium.org/12704024/diff/61001/content/content_tests.gyp... content/content_tests.gypi:477: '../webkit/fileapi/media/picasa/pmp_test_helper.cc', On 2013/03/29 21:35:07, vandebo wrote: > pmp_test_utils.* would be a more common name in the Chrome codebase. I followed the convention in webkit/fileapi, i.e. file_system_database_test_helper and async_file_test_helper. Should I still change this? https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_column_reader.cc (right): https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:33: base::MemoryMappedFile file; On 2013/03/29 21:35:07, vandebo wrote: > So using a memory mapped file means you need enough virtual address space to map > the entire file. But you really don't need a memory mapped file. How about > using GetFileSize() and ReadFile() Done. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:33: base::MemoryMappedFile file; On 2013/03/29 21:35:07, vandebo wrote: > Add a DCHECK that we're on the FILE thread? Done. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:35: if(!file.Initialize(filepath) || On 2013/03/29 21:35:07, vandebo wrote: > nit: s/if(/if (/g Done. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:53: if(field_type_ == kPmpFieldTypeString && row < rows_) { On 2013/03/29 21:35:07, vandebo wrote: > We like early exits of functions, especially when it keeps the indent down... > so: > if (field_type_ != kPmpFieldTypeString || row >= rows_) > return false; > ... Nice. That's definitely an improvement. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:63: DCHECK_GT(length_, kPmpHeaderSize + row*sizeof(uint32)); On 2013/03/29 21:35:07, vandebo wrote: > nit: space around math operators: row * sizeof() Done. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:121: // Read the field type On 2013/03/29 21:35:07, vandebo wrote: > nit: remove redundant comment. Done. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:128: // Read the number of rows expected. On 2013/03/29 21:35:07, vandebo wrote: > nit: redundant comment. Done. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:132: On 2013/03/29 21:35:07, vandebo wrote: > nit: extra line. Done. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:154: if(parse_success) { On 2013/03/29 21:35:07, vandebo wrote: > if (parse_success && rows_read) > *rows_read = rows_; > return parse_success; That's ninja. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:168: strings_.clear(); On 2013/03/29 21:35:07, vandebo wrote: > not needed Done. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:172: const uint8* itr = data_.get() + kPmpHeaderSize; On 2013/03/29 21:35:07, vandebo wrote: > nit: We usually use it for iterators... though this isn't actually an iterator. > Maybe data? Done. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:183: ptrdiff_t length_in_bytes = string_end - itr + 1; On 2013/03/29 21:35:07, vandebo wrote: > Any word back from Picasa folks on the string encoding used? I fwded you email from them. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:190: // This means the file has more bytes at the end we haven't parsed. On 2013/03/29 21:35:07, vandebo wrote: > redundant comment. Done. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:192: return false; On 2013/03/29 21:35:07, vandebo wrote: > return bytes_parsed == length_; Done. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:198: bool PmpColumnReader::ValidatePrimitiveArrayLength() { On 2013/03/29 21:35:07, vandebo wrote: > I think you can inline this template without much loss. Done. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_column_reader.h (right): https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.h:27: // Returns true if read successfully. |rows_read| undefined if returns false. On 2013/03/29 21:35:07, vandebo wrote: > nit: ...is undefined... Done. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.h:28: bool InitFromFile(const base::FilePath& filepath, uint32* rows_read); On 2013/03/29 21:35:07, vandebo wrote: > nit: Since there's only one Init method and it's clear from the args that it > reads from a file, just call this Init. Done. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.h:30: // These functions read the value of that row into the |result| variable. On 2013/03/29 21:35:07, vandebo wrote: > nit: ..the value of |row| into |result|. Done. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.h:31: // Function returns false if the column is of the wrong type or the row On 2013/03/29 21:35:07, vandebo wrote: > nit: Functions return Done. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.h:39: const uint16 field_type() const { On 2013/03/29 21:35:07, vandebo wrote: > The native encoding of field_type is returned? Say so in a comment. Done. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.h:48: scoped_array<uint8> data_; On 2013/03/29 21:35:07, vandebo wrote: > This is really nit picky, but is there any organization to your member > variables? Maybe grouping like this would make sense? > > (source data) > data_ > length_ > > (header data) > field_type_ > rows_ > > (computed info) > strings_ Done. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_column_reader_unittest.cc (right): https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader_unittest.cc:20: bool ReaderReadFromVector(PmpColumnReader* const reader, On 2013/03/29 21:35:07, vandebo wrote: > InitColumnReaderFromMemory ? Done. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader_unittest.cc:68: for(uint32 i = 0; i < elems.size(); i++) { On 2013/03/29 21:35:07, vandebo wrote: > i < elems.size() && i < rows_read. You've already expected that they're equal. > If they aren't, there's no point trying to read beyond the end. Done. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_constants.h (right): https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_constants.h:35: const uint16 kPmpFieldTypeString = 0x00; On 2013/03/29 21:35:07, vandebo wrote: > It might clean up some interfaces if you make this an enum. For now, I want the types of field types to be uint16, so they match what's serialized in the file. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_table_reader.cc (right): https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/03/29 21:35:07, vandebo wrote: > nit: no (c) Done. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:30: // Expect to read some columns, otherwise, a programming error. On 2013/03/29 21:35:07, vandebo wrote: > Remove comment. Done. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:33: // Only allow initialization once On 2013/03/29 21:35:07, vandebo wrote: > Remove comment. Done. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:37: // Directory must exist. On 2013/03/29 21:35:07, vandebo wrote: > Remove comment. Done. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:43: // Look for the "%s_0".format(table_name_) file, indicating table existence. On 2013/03/29 21:35:07, vandebo wrote: > Does '"%s_0".format(table_name_)' mean table_name_ + "_0" ? If so, say that. Done. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:55: // Construct the column readers. On 2013/03/29 21:35:07, vandebo wrote: > Remove comment. Done. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:57: it != columns.end(); it++) { On 2013/03/29 21:35:07, vandebo wrote: > ++it Done. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.cc:72: // Actually succeeded. Set member data. On 2013/03/29 21:35:07, vandebo wrote: > Remove comment. Done. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_table_reader.h (right): https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.h:40: // Contains the last 'good' read of the disk. On 2013/03/29 21:35:07, vandebo wrote: > redundant comment now? Done. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.h:42: // Contains the number of rows obtained during the last 'good' read of disk. On 2013/03/29 21:35:07, vandebo wrote: > redundant comment now? Done. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_test_helper.cc (right): https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_test_helper.cc:67: template<class T> On 2013/03/29 21:35:07, vandebo wrote: > I suspect all the fancy mechanisms below are longer than just writing two > versions of MakeHeaderAndBody. MakeHeaderAndBody is called by a number of templated functions. I'd have to make two versions of all those functions too. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_test_helper.h (right): https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_test_helper.h:21: class PmpTestHelper { On 2013/03/29 21:35:07, vandebo wrote: > You might want to make this a functional class. > > In the constructor make the temp dir and then have a method to write a column > based on data or write a column based on bytes.... I could see that being useful. I'll do it if I add more TableReader tests.
I was thinking, we might want to put this code into a different namespace... picasaimport? https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_column_reader.cc (right): https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:183: ptrdiff_t length_in_bytes = string_end - itr + 1; On 2013/04/01 22:19:18, tommycli wrote: > On 2013/03/29 21:35:07, vandebo wrote: > > Any word back from Picasa folks on the string encoding used? > > I fwded you email from them. You got an non-answer back. Can you peek into the code, or test picasa with some non english strings to see what's going on? https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_constants.h (right): https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_constants.h:35: const uint16 kPmpFieldTypeString = 0x00; On 2013/04/01 22:19:18, tommycli wrote: > On 2013/03/29 21:35:07, vandebo wrote: > > It might clean up some interfaces if you make this an enum. > > For now, I want the types of field types to be uint16, so they match what's > serialized in the file. It will be a bit of extra code to convert what comes out of the file into the enum value, but I think it'll be a win in terms of readability of the interfaces. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_test_helper.h (right): https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_test_helper.h:21: class PmpTestHelper { On 2013/04/01 22:19:18, tommycli wrote: > On 2013/03/29 21:35:07, vandebo wrote: > > You might want to make this a functional class. > > > > In the constructor make the temp dir and then have a method to write a column > > based on data or write a column based on bytes.... > > I could see that being useful. I'll do it if I add more TableReader tests. But what if you're not the next one to add a test? :-) The better yours tests and test infrastructure is, the more likely other devs will be able to solve the problems on their own and not break your code. https://codereview.chromium.org/12704024/diff/84001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_column_reader.cc (right): https://codereview.chromium.org/12704024/diff/84001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:49: return (file_util::ReadFile(filepath, data_begin, length_) && nit: extra ()s https://codereview.chromium.org/12704024/diff/84001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:128: bool parse_success = false; If you have IndexStrings return bytes_parsed, you can remove this variable. and just check body_length == expected_body_length on line 156. https://codereview.chromium.org/12704024/diff/84001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:168: const uint8* body_cursor = data_.get() + kPmpHeaderSize; nit: body? data_cursor or just cursor. https://codereview.chromium.org/12704024/diff/84001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_column_reader_unittest.cc (right): https://codereview.chromium.org/12704024/diff/84001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader_unittest.cc:103: void TestPrimitives(const uint16 field_type) { nit: TestPrimitive https://codereview.chromium.org/12704024/diff/84001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_table_reader.h (right): https://codereview.chromium.org/12704024/diff/84001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.h:29: // subsequent columns to GetColumns() if InitFromDisk() succeeds. nit: Update comment (InitFromDisk)
I changed the namespace of these classes from fileapi to picasaimport. https://codereview.chromium.org/12704024/diff/84001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_column_reader.cc (right): https://codereview.chromium.org/12704024/diff/84001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:49: return (file_util::ReadFile(filepath, data_begin, length_) && On 2013/04/02 18:10:55, vandebo wrote: > nit: extra ()s Done. https://codereview.chromium.org/12704024/diff/84001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:128: bool parse_success = false; On 2013/04/02 18:10:55, vandebo wrote: > If you have IndexStrings return bytes_parsed, you can remove this variable. and > just check body_length == expected_body_length on line 156. Done. https://codereview.chromium.org/12704024/diff/84001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:168: const uint8* body_cursor = data_.get() + kPmpHeaderSize; On 2013/04/02 18:10:55, vandebo wrote: > nit: body? data_cursor or just cursor. Done. https://codereview.chromium.org/12704024/diff/84001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_column_reader_unittest.cc (right): https://codereview.chromium.org/12704024/diff/84001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader_unittest.cc:103: void TestPrimitives(const uint16 field_type) { On 2013/04/02 18:10:55, vandebo wrote: > nit: TestPrimitive Done. https://codereview.chromium.org/12704024/diff/84001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_table_reader.h (right): https://codereview.chromium.org/12704024/diff/84001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_table_reader.h:29: // subsequent columns to GetColumns() if InitFromDisk() succeeds. On 2013/04/02 18:10:55, vandebo wrote: > nit: Update comment (InitFromDisk) Done.
Made PmpTestHelper stateful and own the temporary directory. https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_column_reader.cc (right): https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:64: ReadPrimitive<uint32>(row, result)); On 2013/03/29 21:35:07, vandebo wrote: > On 2013/03/29 00:16:01, tommycli wrote: > > On 2013/03/28 00:56:04, vandebo wrote: > > > Once you get everything else taken care, can you try to implement all this > > stuff > > > without any templates? > > > > Since we are only using 4 field types, the best alternative to the template > was > > to just copy-paste the template body and eliminate the template. > > > > Kinda worried about the copy-paste though - what do you think? > > I think the amount of code duplication is small and it makes it much easier to > understand what's going on, so an overall win. Done. https://codereview.chromium.org/12704024/diff/40001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:101: if (memcmp(&kPmpMagic1, &data_[kPmpMagic1Offset], sizeof(kPmpMagic1)) != 0 || On 2013/03/29 21:35:07, vandebo wrote: > On 2013/03/29 00:16:01, tommycli wrote: > > On 2013/03/28 00:56:04, vandebo wrote: > > > data_.get() + kPmpMagic1Offset ? > > > > Makes the command no longer fit on one line. Change still desirable? > > Your call. Done. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_column_reader.cc (right): https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_column_reader.cc:183: ptrdiff_t length_in_bytes = string_end - itr + 1; On 2013/04/02 18:10:55, vandebo wrote: > On 2013/04/01 22:19:18, tommycli wrote: > > On 2013/03/29 21:35:07, vandebo wrote: > > > Any word back from Picasa folks on the string encoding used? > > > > I fwded you email from them. > > You got an non-answer back. Can you peek into the code, or test picasa with > some non english strings to see what's going on? Done. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_constants.h (right): https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_constants.h:35: const uint16 kPmpFieldTypeString = 0x00; On 2013/04/02 18:10:55, vandebo wrote: > On 2013/04/01 22:19:18, tommycli wrote: > > On 2013/03/29 21:35:07, vandebo wrote: > > > It might clean up some interfaces if you make this an enum. > > > > For now, I want the types of field types to be uint16, so they match what's > > serialized in the file. > > It will be a bit of extra code to convert what comes out of the file into the > enum value, but I think it'll be a win in terms of readability of the > interfaces. Done. https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_test_helper.h (right): https://codereview.chromium.org/12704024/diff/61001/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_test_helper.h:21: class PmpTestHelper { On 2013/04/02 18:10:55, vandebo wrote: > On 2013/04/01 22:19:18, tommycli wrote: > > On 2013/03/29 21:35:07, vandebo wrote: > > > You might want to make this a functional class. > > > > > > In the constructor make the temp dir and then have a method to write a > column > > > based on data or write a column based on bytes.... > > > > I could see that being useful. I'll do it if I add more TableReader tests. > > But what if you're not the next one to add a test? :-) The better yours tests > and test infrastructure is, the more likely other devs will be able to solve the > problems on their own and not break your code. Oh. That's a good point. I'll clean it up then.
Looking good. Just a bit more clean up. https://codereview.chromium.org/12704024/diff/89003/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_constants.h (right): https://codereview.chromium.org/12704024/diff/89003/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_constants.h:36: STRING_TYPE = 0x00, Style guide says we prefer to name enum elements like constants instead of macros. nit: kPmpString etc. https://codereview.chromium.org/12704024/diff/89003/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_test_helper.cc (right): https://codereview.chromium.org/12704024/diff/89003/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_test_helper.cc:13: PmpTestHelper::PmpTestHelper() : temp_dir_() { } You don't need temp_dir_() https://codereview.chromium.org/12704024/diff/89003/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_test_helper.h (right): https://codereview.chromium.org/12704024/diff/89003/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_test_helper.h:28: bool Init(); Why an empty Init, just put it in the constructor. https://codereview.chromium.org/12704024/diff/89003/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_test_helper.h:30: base::FilePath GetTempDirPath(); nit: temp_dir() https://codereview.chromium.org/12704024/diff/89003/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_test_helper.h:50: static bool WriteToFile(const base::FilePath& path, std::vector<uint8> data); These don't need to be part of the class, just make them functions in an anonymous namespace.
This version compiles and passes tests on all the build platforms (finally). https://chromiumcodereview.appspot.com/12704024/diff/89003/webkit/fileapi/med... File webkit/fileapi/media/picasa/pmp_constants.h (right): https://chromiumcodereview.appspot.com/12704024/diff/89003/webkit/fileapi/med... webkit/fileapi/media/picasa/pmp_constants.h:36: STRING_TYPE = 0x00, On 2013/04/02 22:17:58, vandebo wrote: > Style guide says we prefer to name enum elements like constants instead of > macros. > > nit: kPmpString etc. http://dev.chromium.org/developers/coding-style#Naming said to use MACRO_STYLE names. Which should I use? https://chromiumcodereview.appspot.com/12704024/diff/89003/webkit/fileapi/med... File webkit/fileapi/media/picasa/pmp_test_helper.cc (right): https://chromiumcodereview.appspot.com/12704024/diff/89003/webkit/fileapi/med... webkit/fileapi/media/picasa/pmp_test_helper.cc:13: PmpTestHelper::PmpTestHelper() : temp_dir_() { } On 2013/04/02 22:17:58, vandebo wrote: > You don't need temp_dir_() Done. https://chromiumcodereview.appspot.com/12704024/diff/89003/webkit/fileapi/med... File webkit/fileapi/media/picasa/pmp_test_helper.h (right): https://chromiumcodereview.appspot.com/12704024/diff/89003/webkit/fileapi/med... webkit/fileapi/media/picasa/pmp_test_helper.h:28: bool Init(); On 2013/04/02 22:17:58, vandebo wrote: > Why an empty Init, just put it in the constructor. This can fail if the temporary directory is not successfully created. I do ASSERT_TRUE(test_helper.Init()) before the body of the test. https://chromiumcodereview.appspot.com/12704024/diff/89003/webkit/fileapi/med... webkit/fileapi/media/picasa/pmp_test_helper.h:30: base::FilePath GetTempDirPath(); On 2013/04/02 22:17:58, vandebo wrote: > nit: temp_dir() The body of this is actually "return temp_dir_.path()". https://chromiumcodereview.appspot.com/12704024/diff/89003/webkit/fileapi/med... webkit/fileapi/media/picasa/pmp_test_helper.h:50: static bool WriteToFile(const base::FilePath& path, std::vector<uint8> data); On 2013/04/02 22:17:58, vandebo wrote: > These don't need to be part of the class, just make them functions in an > anonymous namespace. Good call.
https://chromiumcodereview.appspot.com/12704024/diff/89003/webkit/fileapi/med... File webkit/fileapi/media/picasa/pmp_constants.h (right): https://chromiumcodereview.appspot.com/12704024/diff/89003/webkit/fileapi/med... webkit/fileapi/media/picasa/pmp_constants.h:36: STRING_TYPE = 0x00, On 2013/04/05 16:49:24, tommycli wrote: > On 2013/04/02 22:17:58, vandebo wrote: > > Style guide says we prefer to name enum elements like constants instead of > > macros. > > > > nit: kPmpString etc. > > http://dev.chromium.org/developers/coding-style#Naming said to use MACRO_STYLE > names. Which should I use? Oops... Chrome style guide wins. But the names should be less generic... PMP_STRING https://chromiumcodereview.appspot.com/12704024/diff/136003/webkit/fileapi/me... File webkit/fileapi/media/picasa/pmp_column_reader.h (right): https://chromiumcodereview.appspot.com/12704024/diff/136003/webkit/fileapi/me... webkit/fileapi/media/picasa/pmp_column_reader.h:41: uint16 field_type() const { Shouldn't this return a PmpFieldType ? https://chromiumcodereview.appspot.com/12704024/diff/136003/webkit/fileapi/me... webkit/fileapi/media/picasa/pmp_column_reader.h:55: uint16 field_type_; And similarly store one? https://chromiumcodereview.appspot.com/12704024/diff/136003/webkit/fileapi/me... File webkit/fileapi/media/picasa/pmp_test_helper.cc (right): https://chromiumcodereview.appspot.com/12704024/diff/136003/webkit/fileapi/me... webkit/fileapi/media/picasa/pmp_test_helper.cc:81: table_name + "_" + column_name + "." + picasaimport::kPmpExtension)); You're in the picassaimport namespace here. https://chromiumcodereview.appspot.com/12704024/diff/136003/webkit/fileapi/me... webkit/fileapi/media/picasa/pmp_test_helper.cc:83: base::FilePath path = temp_dir_.path().Append( You may want to reorganize this a bit to reduce the duplication.
https://codereview.chromium.org/12704024/diff/89003/webkit/fileapi/media/pica... File webkit/fileapi/media/picasa/pmp_constants.h (right): https://codereview.chromium.org/12704024/diff/89003/webkit/fileapi/media/pica... webkit/fileapi/media/picasa/pmp_constants.h:36: STRING_TYPE = 0x00, On 2013/04/05 18:52:47, vandebo wrote: > On 2013/04/05 16:49:24, tommycli wrote: > > On 2013/04/02 22:17:58, vandebo wrote: > > > Style guide says we prefer to name enum elements like constants instead of > > > macros. > > > > > > nit: kPmpString etc. > > > > http://dev.chromium.org/developers/coding-style#Naming said to use MACRO_STYLE > > names. Which should I use? > > Oops... Chrome style guide wins. But the names should be less generic... > PMP_STRING Done. https://codereview.chromium.org/12704024/diff/136003/webkit/fileapi/media/pic... File webkit/fileapi/media/picasa/pmp_column_reader.h (right): https://codereview.chromium.org/12704024/diff/136003/webkit/fileapi/media/pic... webkit/fileapi/media/picasa/pmp_column_reader.h:41: uint16 field_type() const { On 2013/04/05 18:52:47, vandebo wrote: > Shouldn't this return a PmpFieldType ? Done. https://codereview.chromium.org/12704024/diff/136003/webkit/fileapi/media/pic... webkit/fileapi/media/picasa/pmp_column_reader.h:55: uint16 field_type_; On 2013/04/05 18:52:47, vandebo wrote: > And similarly store one? Done. https://codereview.chromium.org/12704024/diff/136003/webkit/fileapi/media/pic... File webkit/fileapi/media/picasa/pmp_test_helper.cc (right): https://codereview.chromium.org/12704024/diff/136003/webkit/fileapi/media/pic... webkit/fileapi/media/picasa/pmp_test_helper.cc:81: table_name + "_" + column_name + "." + picasaimport::kPmpExtension)); On 2013/04/05 18:52:47, vandebo wrote: > You're in the picassaimport namespace here. Done. https://codereview.chromium.org/12704024/diff/136003/webkit/fileapi/media/pic... webkit/fileapi/media/picasa/pmp_test_helper.cc:83: base::FilePath path = temp_dir_.path().Append( On 2013/04/05 18:52:47, vandebo wrote: > You may want to reorganize this a bit to reduce the duplication. Done.
Kinoko, please do an owners/final review. LGTM
lgtm + some comments. Apologies for my slow review!! Currently this one isn't called from anywhere, but I suppose it's hooked from some file_util code. I noticed the following code pattern in multiple places: #if defined(OS_WIN) base::FilePath foo = dir.Append(UTF8ToUTF16(file_name)); #else base::FilePath foo = dir.Append(file_name); #endif Can we write like following instead? In general we'd like to avoid having ifdefs if possible. base::FilePath foo = dir.Append(base::FilePath::FromUTF8Unsafe(file_name)); https://codereview.chromium.org/12704024/diff/140003/webkit/fileapi/media/pic... File webkit/fileapi/media/picasa/pmp_column_reader_unittest.cc (right): https://codereview.chromium.org/12704024/diff/140003/webkit/fileapi/media/pic... webkit/fileapi/media/picasa/pmp_column_reader_unittest.cc:55: for(uint32 i = 0; i < elems.size() && i < rows_read; i++) { nit: space between for and ( https://codereview.chromium.org/12704024/diff/140003/webkit/fileapi/media/pic... webkit/fileapi/media/picasa/pmp_column_reader_unittest.cc:97: for(uint32 i = 0; i < n; i++) { ditto https://codereview.chromium.org/12704024/diff/140003/webkit/fileapi/media/pic... File webkit/fileapi/media/picasa/pmp_constants.h (right): https://codereview.chromium.org/12704024/diff/140003/webkit/fileapi/media/pic... webkit/fileapi/media/picasa/pmp_constants.h:41: PMP_INVALID_TYPE = 0xff nit: it's fine if this fits other coding style in Media, but using a common prefix rather than suffix looks more common (e.g. PMP_TYPE_XXX or PMP_FIELD_TYPE_XXX). https://codereview.chromium.org/12704024/diff/140003/webkit/fileapi/media/pic... File webkit/fileapi/media/picasa/pmp_table_reader.cc (right): https://codereview.chromium.org/12704024/diff/140003/webkit/fileapi/media/pic... webkit/fileapi/media/picasa/pmp_table_reader.cc:48: #endif Could we replace these ifdefs with FilePath::FromUTF8Unsafe like following? Or do we specifically want to use UTF8ToUTF16? (Which does slightly different thing) directory_path.Append(FilePath::FromUTF8Unsafe(indicator_file_name)); https://codereview.chromium.org/12704024/diff/140003/webkit/fileapi/media/pic... File webkit/fileapi/media/picasa/pmp_table_reader_unittest.cc (right): https://codereview.chromium.org/12704024/diff/140003/webkit/fileapi/media/pic... webkit/fileapi/media/picasa/pmp_table_reader_unittest.cc:38: picasaimport::PMP_DOUBLE64_TYPE nit: indent https://codereview.chromium.org/12704024/diff/140003/webkit/fileapi/media/pic... webkit/fileapi/media/picasa/pmp_table_reader_unittest.cc:49: #endif ditto about the ifdef https://codereview.chromium.org/12704024/diff/140003/webkit/fileapi/media/pic... webkit/fileapi/media/picasa/pmp_table_reader_unittest.cc:72: for(int i = 0; i < 3; i++) { nit: space between for and ( https://codereview.chromium.org/12704024/diff/140003/webkit/fileapi/media/pic... File webkit/fileapi/media/picasa/pmp_test_helper.cc (right): https://codereview.chromium.org/12704024/diff/140003/webkit/fileapi/media/pic... webkit/fileapi/media/picasa/pmp_test_helper.cc:7: #include <iterator> #include <algorithm> for std::copy? https://codereview.chromium.org/12704024/diff/140003/webkit/fileapi/media/pic... File webkit/fileapi/media/picasa/pmp_test_helper.h (right): https://codereview.chromium.org/12704024/diff/140003/webkit/fileapi/media/pic... webkit/fileapi/media/picasa/pmp_test_helper.h:17: } // namespace base nit: for forward decl you can compress empty lines like: namespace base { class FilePath; } https://codereview.chromium.org/12704024/diff/140003/webkit/fileapi/media/pic... webkit/fileapi/media/picasa/pmp_test_helper.h:36: std::vector<T> elements_vector); can these params be passed by const ref? const std::string& table_name, const std::string& column_name, .. const std::vector<T>& elements_vector https://codereview.chromium.org/12704024/diff/140003/webkit/fileapi/media/pic... webkit/fileapi/media/picasa/pmp_test_helper.h:39: std::vector<uint8> data, uint32* rows_read); ditto for vector<uint8>
Thanks for the review kinuko. I fixed it up according to your comments, and will be sending this to the CQ now. https://codereview.chromium.org/12704024/diff/140003/webkit/fileapi/media/pic... File webkit/fileapi/media/picasa/pmp_column_reader_unittest.cc (right): https://codereview.chromium.org/12704024/diff/140003/webkit/fileapi/media/pic... webkit/fileapi/media/picasa/pmp_column_reader_unittest.cc:55: for(uint32 i = 0; i < elems.size() && i < rows_read; i++) { On 2013/04/10 02:27:54, kinuko wrote: > nit: space between for and ( Done. https://codereview.chromium.org/12704024/diff/140003/webkit/fileapi/media/pic... webkit/fileapi/media/picasa/pmp_column_reader_unittest.cc:97: for(uint32 i = 0; i < n; i++) { On 2013/04/10 02:27:54, kinuko wrote: > ditto Done. https://codereview.chromium.org/12704024/diff/140003/webkit/fileapi/media/pic... File webkit/fileapi/media/picasa/pmp_constants.h (right): https://codereview.chromium.org/12704024/diff/140003/webkit/fileapi/media/pic... webkit/fileapi/media/picasa/pmp_constants.h:41: PMP_INVALID_TYPE = 0xff On 2013/04/10 02:27:54, kinuko wrote: > nit: it's fine if this fits other coding style in Media, but using a common > prefix rather than suffix looks more common (e.g. PMP_TYPE_XXX or > PMP_FIELD_TYPE_XXX). Done. https://codereview.chromium.org/12704024/diff/140003/webkit/fileapi/media/pic... File webkit/fileapi/media/picasa/pmp_table_reader.cc (right): https://codereview.chromium.org/12704024/diff/140003/webkit/fileapi/media/pic... webkit/fileapi/media/picasa/pmp_table_reader.cc:48: #endif On 2013/04/10 02:27:54, kinuko wrote: > Could we replace these ifdefs with FilePath::FromUTF8Unsafe like following? Or > do we specifically want to use UTF8ToUTF16? (Which does slightly different > thing) > > directory_path.Append(FilePath::FromUTF8Unsafe(indicator_file_name)); Actually I believe semantically speaking, FromUTF8Unsafe is exactly what we want. Good catch! https://codereview.chromium.org/12704024/diff/140003/webkit/fileapi/media/pic... File webkit/fileapi/media/picasa/pmp_table_reader_unittest.cc (right): https://codereview.chromium.org/12704024/diff/140003/webkit/fileapi/media/pic... webkit/fileapi/media/picasa/pmp_table_reader_unittest.cc:38: picasaimport::PMP_DOUBLE64_TYPE On 2013/04/10 02:27:54, kinuko wrote: > nit: indent Done. https://codereview.chromium.org/12704024/diff/140003/webkit/fileapi/media/pic... webkit/fileapi/media/picasa/pmp_table_reader_unittest.cc:49: #endif On 2013/04/10 02:27:54, kinuko wrote: > ditto about the ifdef Done. https://codereview.chromium.org/12704024/diff/140003/webkit/fileapi/media/pic... webkit/fileapi/media/picasa/pmp_table_reader_unittest.cc:72: for(int i = 0; i < 3; i++) { On 2013/04/10 02:27:54, kinuko wrote: > nit: space between for and ( Done. https://codereview.chromium.org/12704024/diff/140003/webkit/fileapi/media/pic... File webkit/fileapi/media/picasa/pmp_test_helper.cc (right): https://codereview.chromium.org/12704024/diff/140003/webkit/fileapi/media/pic... webkit/fileapi/media/picasa/pmp_test_helper.cc:7: #include <iterator> On 2013/04/10 02:27:54, kinuko wrote: > #include <algorithm> for std::copy? Done. https://codereview.chromium.org/12704024/diff/140003/webkit/fileapi/media/pic... File webkit/fileapi/media/picasa/pmp_test_helper.h (right): https://codereview.chromium.org/12704024/diff/140003/webkit/fileapi/media/pic... webkit/fileapi/media/picasa/pmp_test_helper.h:17: } // namespace base On 2013/04/10 02:27:54, kinuko wrote: > nit: for forward decl you can compress empty lines like: > > namespace base { > class FilePath; > } Done. https://codereview.chromium.org/12704024/diff/140003/webkit/fileapi/media/pic... webkit/fileapi/media/picasa/pmp_test_helper.h:36: std::vector<T> elements_vector); On 2013/04/10 02:27:54, kinuko wrote: > can these params be passed by const ref? > > const std::string& table_name, > const std::string& column_name, > .. > const std::vector<T>& elements_vector Done. https://codereview.chromium.org/12704024/diff/140003/webkit/fileapi/media/pic... webkit/fileapi/media/picasa/pmp_test_helper.h:39: std::vector<uint8> data, uint32* rows_read); On 2013/04/10 02:27:54, kinuko wrote: > ditto for vector<uint8> Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tommycli@chromium.org/12704024/154001
Presubmit check for 12704024-154001 failed and returned exit status 1.
INFO:root:Found 11 file(s).
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
content/content_tests.gypi
Presubmit checks took 1.7s to calculate.
lgtm owner stampity stamp
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tommycli@chromium.org/12704024/154001
Message was sent while issue was closed.
Change committed as 193537 |
