PicasaAlbumTableReader for Media Galleries API

PicasaAlbumTableReader for Media Galleries API

BUG=151701
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=196323

[tommycli, 2013-04-05 23:04:07]

Might as well get the review on this started. This depends on the other CL to
compile.

https://chromiumcodereview.appspot.com/13529028/diff/2001/webkit/fileapi/medi...
File webkit/fileapi/media/picasa/pmp_albumdata_table_reader.cc (right):

https://chromiumcodereview.appspot.com/13529028/diff/2001/webkit/fileapi/medi...
webkit/fileapi/media/picasa/pmp_albumdata_table_reader.cc:122: #if
defined(OS_WIN)
I'm using this ugly idiom so often. file_path.h already has a
FILE_PATH_LITERAL(x) macro for string literals. Should I add a
FILE_PATH_FROM_UTF8(x) macro for std::string UTF-8s?

[vandebo (ex-Chrome), 2013-04-06 01:10:30]

https://chromiumcodereview.appspot.com/13529028/diff/2001/webkit/fileapi/medi...
File webkit/fileapi/media/picasa/album_info.h (right):

https://chromiumcodereview.appspot.com/13529028/diff/2001/webkit/fileapi/medi...
webkit/fileapi/media/picasa/album_info.h:15: 
I think this can all go into pmp_albumdata_table_reader.h

https://chromiumcodereview.appspot.com/13529028/diff/2001/webkit/fileapi/medi...
webkit/fileapi/media/picasa/album_info.h:22: AlbumType category;
type?

https://chromiumcodereview.appspot.com/13529028/diff/2001/webkit/fileapi/medi...
webkit/fileapi/media/picasa/album_info.h:23: base::Time timestamp;
Do you plan on using timestamp?  If not, omit it.

https://chromiumcodereview.appspot.com/13529028/diff/2001/webkit/fileapi/medi...
File webkit/fileapi/media/picasa/pmp_albumdata_table_reader.cc (right):

https://chromiumcodereview.appspot.com/13529028/diff/2001/webkit/fileapi/medi...
webkit/fileapi/media/picasa/pmp_albumdata_table_reader.cc:52: base::TimeDelta
variant_delta = base::TimeDelta::FromMicroseconds(
why not FromDays ?

https://chromiumcodereview.appspot.com/13529028/diff/2001/webkit/fileapi/medi...
webkit/fileapi/media/picasa/pmp_albumdata_table_reader.cc:91:
if(!columns[kColCategory]->ReadUInt32(i, &category) ||
columns[constant number] suggests that maybe a better interface for the table
reader is to return a map from column name to column reader.

https://chromiumcodereview.appspot.com/13529028/diff/2001/webkit/fileapi/medi...
webkit/fileapi/media/picasa/pmp_albumdata_table_reader.cc:122: #if
defined(OS_WIN)
On 2013/04/05 23:04:08, tommycli wrote:
> I'm using this ugly idiom so often. file_path.h already has a
> FILE_PATH_LITERAL(x) macro for string literals. Should I add a
> FILE_PATH_FROM_UTF8(x) macro for std::string UTF-8s?

The bar for adding to base is pretty high...

https://chromiumcodereview.appspot.com/13529028/diff/2001/webkit/fileapi/medi...
webkit/fileapi/media/picasa/pmp_albumdata_table_reader.cc:128:
folders_.push_back(album);
It looks like some some fields are valid only for certain types.  Maybe a per
type struct would be better.  Have a struct with fields that aren't always valid
is one of my pet peeves.

https://chromiumcodereview.appspot.com/13529028/diff/2001/webkit/fileapi/medi...
File webkit/fileapi/media/picasa/pmp_albumdata_table_reader.h (right):

https://chromiumcodereview.appspot.com/13529028/diff/2001/webkit/fileapi/medi...
webkit/fileapi/media/picasa/pmp_albumdata_table_reader.h:4: 
nit: extra line

https://chromiumcodereview.appspot.com/13529028/diff/2001/webkit/fileapi/medi...
webkit/fileapi/media/picasa/pmp_albumdata_table_reader.h:16: class
WEBKIT_STORAGE_EXPORT_PRIVATE PmpAlbumdataTableReader
At this level, I think this is Picasa specific and not Pmp specific...
PicasaAlbums ?

https://chromiumcodereview.appspot.com/13529028/diff/2001/webkit/fileapi/medi...
webkit/fileapi/media/picasa/pmp_albumdata_table_reader.h:17: : public
PmpTableReader {
Does this need to inherit the table reader?  In general, we prefer composition
over inheritance.

https://chromiumcodereview.appspot.com/13529028/diff/2001/webkit/fileapi/medi...
webkit/fileapi/media/picasa/pmp_albumdata_table_reader.h:24:
std::vector<AlbumInfo> folders() {
If you're going to separate folders and user albums in the interface, is it
still useful to have the type in the AlbumInfo?

https://chromiumcodereview.appspot.com/13529028/diff/2001/webkit/fileapi/medi...
webkit/fileapi/media/picasa/pmp_albumdata_table_reader.h:33:
std::vector<AlbumInfo> folders_;
Since you're storing all the data at this level, I don't think you want to even
have a table reader as a part of the class; Init should use one and then destroy
it.

[tommycli, 2013-04-08 17:17:29]

Consolidate some files. Rationalize naming. Remove unnecessary inheritance.
Remove unnecessary struct fields.

https://codereview.chromium.org/13529028/diff/2001/webkit/fileapi/media/picas...
File webkit/fileapi/media/picasa/album_info.h (right):

https://codereview.chromium.org/13529028/diff/2001/webkit/fileapi/media/picas...
webkit/fileapi/media/picasa/album_info.h:15: 
On 2013/04/06 01:10:30, vandebo wrote:
> I think this can all go into pmp_albumdata_table_reader.h

Done.

https://codereview.chromium.org/13529028/diff/2001/webkit/fileapi/media/picas...
webkit/fileapi/media/picasa/album_info.h:22: AlbumType category;
On 2013/04/06 01:10:30, vandebo wrote:
> type?

I'll make two different structs as discussed in next file.

https://codereview.chromium.org/13529028/diff/2001/webkit/fileapi/media/picas...
webkit/fileapi/media/picasa/album_info.h:23: base::Time timestamp;
On 2013/04/06 01:10:30, vandebo wrote:
> Do you plan on using timestamp?  If not, omit it.

Used as first layer of de-duping identical names.

https://codereview.chromium.org/13529028/diff/2001/webkit/fileapi/media/picas...
File webkit/fileapi/media/picasa/pmp_albumdata_table_reader.cc (right):

https://codereview.chromium.org/13529028/diff/2001/webkit/fileapi/media/picas...
webkit/fileapi/media/picasa/pmp_albumdata_table_reader.cc:52: base::TimeDelta
variant_delta = base::TimeDelta::FromMicroseconds(
On 2013/04/06 01:10:30, vandebo wrote:
> why not FromDays ?

Because the argument is an integer, and VariantTime specifies the specific time
during the day in the fractional part of the floating point.

https://codereview.chromium.org/13529028/diff/2001/webkit/fileapi/media/picas...
webkit/fileapi/media/picasa/pmp_albumdata_table_reader.cc:91:
if(!columns[kColCategory]->ReadUInt32(i, &category) ||
On 2013/04/06 01:10:30, vandebo wrote:
> columns[constant number] suggests that maybe a better interface for the table
> reader is to return a map from column name to column reader.

Agreed. I'd like to move forward with what we have right now and circle back to
improve the interface later.

https://codereview.chromium.org/13529028/diff/2001/webkit/fileapi/media/picas...
webkit/fileapi/media/picasa/pmp_albumdata_table_reader.cc:128:
folders_.push_back(album);
On 2013/04/06 01:10:30, vandebo wrote:
> It looks like some some fields are valid only for certain types.  Maybe a per
> type struct would be better.  Have a struct with fields that aren't always
valid
> is one of my pet peeves.

Done.

https://codereview.chromium.org/13529028/diff/2001/webkit/fileapi/media/picas...
File webkit/fileapi/media/picasa/pmp_albumdata_table_reader.h (right):

https://codereview.chromium.org/13529028/diff/2001/webkit/fileapi/media/picas...
webkit/fileapi/media/picasa/pmp_albumdata_table_reader.h:4: 
On 2013/04/06 01:10:30, vandebo wrote:
> nit: extra line

Done.

https://codereview.chromium.org/13529028/diff/2001/webkit/fileapi/media/picas...
webkit/fileapi/media/picasa/pmp_albumdata_table_reader.h:16: class
WEBKIT_STORAGE_EXPORT_PRIVATE PmpAlbumdataTableReader
On 2013/04/06 01:10:30, vandebo wrote:
> At this level, I think this is Picasa specific and not Pmp specific...
> PicasaAlbums ?

Done.

https://codereview.chromium.org/13529028/diff/2001/webkit/fileapi/media/picas...
webkit/fileapi/media/picasa/pmp_albumdata_table_reader.h:17: : public
PmpTableReader {
On 2013/04/06 01:10:30, vandebo wrote:
> Does this need to inherit the table reader?  In general, we prefer composition
> over inheritance.

Done.

https://codereview.chromium.org/13529028/diff/2001/webkit/fileapi/media/picas...
webkit/fileapi/media/picasa/pmp_albumdata_table_reader.h:24:
std::vector<AlbumInfo> folders() {
On 2013/04/06 01:10:30, vandebo wrote:
> If you're going to separate folders and user albums in the interface, is it
> still useful to have the type in the AlbumInfo?

Done.

https://codereview.chromium.org/13529028/diff/2001/webkit/fileapi/media/picas...
webkit/fileapi/media/picasa/pmp_albumdata_table_reader.h:33:
std::vector<AlbumInfo> folders_;
On 2013/04/06 01:10:30, vandebo wrote:
> Since you're storing all the data at this level, I don't think you want to
even
> have a table reader as a part of the class; Init should use one and then
destroy
> it.

Done.

[vandebo (ex-Chrome), 2013-04-08 19:02:30]

https://codereview.chromium.org/13529028/diff/2001/webkit/fileapi/media/picas...
File webkit/fileapi/media/picasa/pmp_albumdata_table_reader.cc (right):

https://codereview.chromium.org/13529028/diff/2001/webkit/fileapi/media/picas...
webkit/fileapi/media/picasa/pmp_albumdata_table_reader.cc:91:
if(!columns[kColCategory]->ReadUInt32(i, &category) ||
On 2013/04/08 17:17:29, tommycli wrote:
> On 2013/04/06 01:10:30, vandebo wrote:
> > columns[constant number] suggests that maybe a better interface for the
table
> > reader is to return a map from column name to column reader.
> 
> Agreed. I'd like to move forward with what we have right now and circle back
to
> improve the interface later.

The other CL can go in as is, but lets fix it in this CL.

https://codereview.chromium.org/13529028/diff/9001/webkit/fileapi/media/picas...
File webkit/fileapi/media/picasa/picasa_albumdata_reader.cc (right):

https://codereview.chromium.org/13529028/diff/9001/webkit/fileapi/media/picas...
webkit/fileapi/media/picasa/picasa_albumdata_reader.cc:38: const uint16
kAlbumCategoryInvalid   = 0xffff;
Is this actually defined in PMP, or is this your sentinel value?

https://codereview.chromium.org/13529028/diff/9001/webkit/fileapi/media/picas...
webkit/fileapi/media/picasa/picasa_albumdata_reader.cc:68:
std::vector<std::string> column_names;
As we discussed, to simplify this code, we can change the TableReader interface
to be Init(tablename, path) and then have an addColumn method.

https://codereview.chromium.org/13529028/diff/9001/webkit/fileapi/media/picas...
webkit/fileapi/media/picasa/picasa_albumdata_reader.cc:92: std::string name,
filename, token, uid;
Move token, filename, and uid into the scope that they are used.  It's easier
for the reader if variables are in the most specific scope possible and also if
they are defined closer to where they are used.

https://codereview.chromium.org/13529028/diff/9001/webkit/fileapi/media/picas...
webkit/fileapi/media/picasa/picasa_albumdata_reader.cc:100: switch(category) {
nit: space

https://codereview.chromium.org/13529028/diff/9001/webkit/fileapi/media/picas...
webkit/fileapi/media/picasa/picasa_albumdata_reader.cc:102:
if(!columns[kColName]->ReadString(i, &name) || name.empty() ||
You already read name, right?

https://codereview.chromium.org/13529028/diff/9001/webkit/fileapi/media/picas...
webkit/fileapi/media/picasa/picasa_albumdata_reader.cc:109: AlbumInfo album;
A constructor for AlbumInfo would make this code simpler:

user_albums_.push_back(AlbumInfo(name, TimeFrom..., uid));

https://codereview.chromium.org/13529028/diff/9001/webkit/fileapi/media/picas...
webkit/fileapi/media/picasa/picasa_albumdata_reader.cc:123: folder.timestamp =
TimeFromMicrosoftVariantTime(date);
Pull the conversion to base::Time to before the switch?

https://codereview.chromium.org/13529028/diff/9001/webkit/fileapi/media/picas...
File webkit/fileapi/media/picasa/picasa_albumdata_reader.h (right):

https://codereview.chromium.org/13529028/diff/9001/webkit/fileapi/media/picas...
webkit/fileapi/media/picasa/picasa_albumdata_reader.h:27: class
WEBKIT_STORAGE_EXPORT_PRIVATE PicasaAlbumdataReader {
nit: PicasaAlbumDataReader

[tommycli, 2013-04-08 23:27:09]

I made the AddColumn method also take an |expected_type| parameter and validate
it.

https://codereview.chromium.org/13529028/diff/9001/webkit/fileapi/media/picas...
File webkit/fileapi/media/picasa/picasa_albumdata_reader.cc (right):

https://codereview.chromium.org/13529028/diff/9001/webkit/fileapi/media/picas...
webkit/fileapi/media/picasa/picasa_albumdata_reader.cc:38: const uint16
kAlbumCategoryInvalid   = 0xffff;
On 2013/04/08 19:02:30, vandebo wrote:
> Is this actually defined in PMP, or is this your sentinel value?

Just a sentinel value.

https://codereview.chromium.org/13529028/diff/9001/webkit/fileapi/media/picas...
webkit/fileapi/media/picasa/picasa_albumdata_reader.cc:68:
std::vector<std::string> column_names;
On 2013/04/08 19:02:30, vandebo wrote:
> As we discussed, to simplify this code, we can change the TableReader
interface
> to be Init(tablename, path) and then have an addColumn method.

Done.

https://codereview.chromium.org/13529028/diff/9001/webkit/fileapi/media/picas...
webkit/fileapi/media/picasa/picasa_albumdata_reader.cc:92: std::string name,
filename, token, uid;
On 2013/04/08 19:02:30, vandebo wrote:
> Move token, filename, and uid into the scope that they are used.  It's easier
> for the reader if variables are in the most specific scope possible and also
if
> they are defined closer to where they are used.

Done.

https://codereview.chromium.org/13529028/diff/9001/webkit/fileapi/media/picas...
webkit/fileapi/media/picasa/picasa_albumdata_reader.cc:100: switch(category) {
On 2013/04/08 19:02:30, vandebo wrote:
> nit: space

Done.

https://codereview.chromium.org/13529028/diff/9001/webkit/fileapi/media/picas...
webkit/fileapi/media/picasa/picasa_albumdata_reader.cc:102:
if(!columns[kColName]->ReadString(i, &name) || name.empty() ||
On 2013/04/08 19:02:30, vandebo wrote:
> You already read name, right?

Done.

https://codereview.chromium.org/13529028/diff/9001/webkit/fileapi/media/picas...
webkit/fileapi/media/picasa/picasa_albumdata_reader.cc:109: AlbumInfo album;
On 2013/04/08 19:02:30, vandebo wrote:
> A constructor for AlbumInfo would make this code simpler:
> 
> user_albums_.push_back(AlbumInfo(name, TimeFrom..., uid));

Done.

https://codereview.chromium.org/13529028/diff/9001/webkit/fileapi/media/picas...
webkit/fileapi/media/picasa/picasa_albumdata_reader.cc:123: folder.timestamp =
TimeFromMicrosoftVariantTime(date);
On 2013/04/08 19:02:30, vandebo wrote:
> Pull the conversion to base::Time to before the switch?

Done.

https://codereview.chromium.org/13529028/diff/9001/webkit/fileapi/media/picas...
File webkit/fileapi/media/picasa/picasa_albumdata_reader.h (right):

https://codereview.chromium.org/13529028/diff/9001/webkit/fileapi/media/picas...
webkit/fileapi/media/picasa/picasa_albumdata_reader.h:27: class
WEBKIT_STORAGE_EXPORT_PRIVATE PicasaAlbumdataReader {
On 2013/04/08 19:02:30, vandebo wrote:
> nit: PicasaAlbumDataReader

Done.

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
File webkit/fileapi/media/picasa/picasa_album_data_reader.cc (right):

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_data_reader.cc:74:
pmp_reader.AddColumn("uid", PMP_STRING_TYPE);
I wish all of these lined up in a pretty way.

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_data_reader.cc:77: column_token &&
column_uid)) {
Is there a smarter way to check that they're all non-NULL?

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
File webkit/fileapi/media/picasa/pmp_column_reader.h (right):

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/pmp_column_reader.h:31: uint32* rows_read);
Turns out that in every use case, I already know what type I expect it to be -
and it will fail later if I get it wrong, so I might as well fail immediately.

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

https://codereview.chromium.org/13529028/diff/9001/webkit/fileapi/media/picas...
File webkit/fileapi/media/picasa/picasa_albumdata_reader.cc (right):

https://codereview.chromium.org/13529028/diff/9001/webkit/fileapi/media/picas...
webkit/fileapi/media/picasa/picasa_albumdata_reader.cc:38: const uint16
kAlbumCategoryInvalid   = 0xffff;
On 2013/04/08 23:27:10, tommycli wrote:
> On 2013/04/08 19:02:30, vandebo wrote:
> > Is this actually defined in PMP, or is this your sentinel value?
> 
> Just a sentinel value.

Maybe a quick comment to that effect then.

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
File webkit/fileapi/media/picasa/picasa_album_data_reader.cc (right):

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_data_reader.cc:30: // First set to Dec
30, 1899, (variant time epoch).
remove comment.

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_data_reader.cc:45: : name(name),
timestamp(timestamp), uid(uid) { }
nit: if the struct/class initializer list doesn't fit all on the first line of
the constructor, then each initializer should be on its own line.

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_data_reader.cc:51:
PicasaAlbumDataReader::PicasaAlbumDataReader() { }
nit: no space; {} or {
}

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_data_reader.cc:63: const
PmpColumnReader* column_category =
nit: category_column or just category

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_data_reader.cc:77: column_token &&
column_uid)) {
On 2013/04/08 23:27:10, tommycli wrote:
> Is there a smarter way to check that they're all non-NULL?

It's not technically the same, but you could do GetColumns()->size() != 6)

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_data_reader.cc:84: double date = 0;
very nit picky nit: move to the previous line so declaration and use are in the
same order.

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_data_reader.cc:85: 
nit: remove line

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_data_reader.cc:97: 
nit: remove line

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_data_reader.cc:109: 
nit: remove line

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_data_reader.cc:118: 
nit: remove line

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
File webkit/fileapi/media/picasa/pmp_table_reader.cc (right):

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/pmp_table_reader.cc:32: bool PmpTableReader::Init()
{
Maybe just put the contents on Init into the constructor.  If init fails, then
set some state that you can detect in AddColumn, then fail immediately in
AddColumn if Init would have failed ?

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
File webkit/fileapi/media/picasa/pmp_table_reader.h (right):

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/pmp_table_reader.h:31: // |columns| parameter will
define, in-order, the columns returned by
Comment is out of date.

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/pmp_table_reader.h:40: std::vector<const
PmpColumnReader*> GetColumns() const;
std:map

[tommycli, 2013-04-09 00:45:33]

https://codereview.chromium.org/13529028/diff/2001/webkit/fileapi/media/picas...
File webkit/fileapi/media/picasa/pmp_albumdata_table_reader.cc (right):

https://codereview.chromium.org/13529028/diff/2001/webkit/fileapi/media/picas...
webkit/fileapi/media/picasa/pmp_albumdata_table_reader.cc:91:
if(!columns[kColCategory]->ReadUInt32(i, &category) ||
On 2013/04/08 19:02:30, vandebo wrote:
> On 2013/04/08 17:17:29, tommycli wrote:
> > On 2013/04/06 01:10:30, vandebo wrote:
> > > columns[constant number] suggests that maybe a better interface for the
> table
> > > reader is to return a map from column name to column reader.
> > 
> > Agreed. I'd like to move forward with what we have right now and circle back
> to
> > improve the interface later.
> 
> The other CL can go in as is, but lets fix it in this CL.

Done.

https://codereview.chromium.org/13529028/diff/9001/webkit/fileapi/media/picas...
File webkit/fileapi/media/picasa/picasa_albumdata_reader.cc (right):

https://codereview.chromium.org/13529028/diff/9001/webkit/fileapi/media/picas...
webkit/fileapi/media/picasa/picasa_albumdata_reader.cc:38: const uint16
kAlbumCategoryInvalid   = 0xffff;
On 2013/04/08 23:57:24, vandebo wrote:
> On 2013/04/08 23:27:10, tommycli wrote:
> > On 2013/04/08 19:02:30, vandebo wrote:
> > > Is this actually defined in PMP, or is this your sentinel value?
> > 
> > Just a sentinel value.
> 
> Maybe a quick comment to that effect then.

Done.

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
File webkit/fileapi/media/picasa/picasa_album_data_reader.cc (right):

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_data_reader.cc:30: // First set to Dec
30, 1899, (variant time epoch).
On 2013/04/08 23:57:24, vandebo wrote:
> remove comment.

Done.

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_data_reader.cc:45: : name(name),
timestamp(timestamp), uid(uid) { }
On 2013/04/08 23:57:24, vandebo wrote:
> nit: if the struct/class initializer list doesn't fit all on the first line of
> the constructor, then each initializer should be on its own line.

Done.

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_data_reader.cc:51:
PicasaAlbumDataReader::PicasaAlbumDataReader() { }
On 2013/04/08 23:57:24, vandebo wrote:
> nit: no space; {} or {
> }

Done.

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_data_reader.cc:63: const
PmpColumnReader* column_category =
On 2013/04/08 23:57:24, vandebo wrote:
> nit: category_column or just category

Done.

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_data_reader.cc:77: column_token &&
column_uid)) {
On 2013/04/08 23:57:24, vandebo wrote:
> On 2013/04/08 23:27:10, tommycli wrote:
> > Is there a smarter way to check that they're all non-NULL?
> 
> It's not technically the same, but you could do GetColumns()->size() != 6)

Done.

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_data_reader.cc:84: double date = 0;
On 2013/04/08 23:57:24, vandebo wrote:
> very nit picky nit: move to the previous line so declaration and use are in
the
> same order.

Done.

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_data_reader.cc:85: 
On 2013/04/08 23:57:24, vandebo wrote:
> nit: remove line

Done.

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_data_reader.cc:97: 
On 2013/04/08 23:57:24, vandebo wrote:
> nit: remove line

Done.

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_data_reader.cc:109: 
On 2013/04/08 23:57:24, vandebo wrote:
> nit: remove line

Done.

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_data_reader.cc:118: 
On 2013/04/08 23:57:24, vandebo wrote:
> nit: remove line

Done.

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
File webkit/fileapi/media/picasa/pmp_table_reader.cc (right):

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/pmp_table_reader.cc:32: bool PmpTableReader::Init()
{
On 2013/04/08 23:57:24, vandebo wrote:
> Maybe just put the contents on Init into the constructor.  If init fails, then
> set some state that you can detect in AddColumn, then fail immediately in
> AddColumn if Init would have failed ?

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

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
File webkit/fileapi/media/picasa/pmp_table_reader.h (right):

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/pmp_table_reader.h:31: // |columns| parameter will
define, in-order, the columns returned by
On 2013/04/08 23:57:24, vandebo wrote:
> Comment is out of date.

I see what you mean about comments being a burden sometimes.

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/pmp_table_reader.h:40: std::vector<const
PmpColumnReader*> GetColumns() const;
On 2013/04/08 23:57:24, vandebo wrote:
> std:map

Done.

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

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
File webkit/fileapi/media/picasa/pmp_table_reader.cc (right):

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/pmp_table_reader.cc:32: bool PmpTableReader::Init()
{
On 2013/04/09 00:45:34, tommycli wrote:
> On 2013/04/08 23:57:24, vandebo wrote:
> > Maybe just put the contents on Init into the constructor.  If init fails,
then
> > set some state that you can detect in AddColumn, then fail immediately in
> > AddColumn if Init would have failed ?
> 
> I just added a 
>   base::ThreadRestrictions::AssertIOAllowed();
> to Init(). Is that safe to put in a constructor? Should I just add a boolean
> member to keep track of whether or not construction succeeded?

If the compiler is happy, I'm happy.

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

https://codereview.chromium.org/13529028/diff/47001/webkit/fileapi/media/pica...
File webkit/fileapi/media/picasa/picasa_album_data_reader.cc (right):

https://codereview.chromium.org/13529028/diff/47001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_data_reader.cc:20: const uint16
kAlbumCategoryInvalid   = 0xffff; // Sentinel value.
nit: two spaces before a comment.

https://codereview.chromium.org/13529028/diff/47001/webkit/fileapi/media/pica...
File webkit/fileapi/media/picasa/picasa_album_data_reader.h (right):

https://codereview.chromium.org/13529028/diff/47001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_data_reader.h:5: #ifndef
WEBKIT_FILEAPI_MEDIA_PICASA_PMP_ALBUM_DATA_TABLE_READER_H_
Filename and #define should match.

https://codereview.chromium.org/13529028/diff/47001/webkit/fileapi/media/pica...
File webkit/fileapi/media/picasa/pmp_column_reader.cc (right):

https://codereview.chromium.org/13529028/diff/47001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/pmp_column_reader.cc:50: ParseData(expected_type,
rows_read);
nit: indent is wrong

https://codereview.chromium.org/13529028/diff/47001/webkit/fileapi/media/pica...
File webkit/fileapi/media/picasa/pmp_column_reader_unittest.cc (right):

https://codereview.chromium.org/13529028/diff/47001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/pmp_column_reader_unittest.cc:70: PmpColumnReader
reader1(""), reader2(""), reader3(""), reader4("");
probably should put these on multiple lines now... actually each one should be
declared just before it is used.  Wouldn't hurt to give them a name too.

https://codereview.chromium.org/13529028/diff/47001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/pmp_column_reader_unittest.cc:113: PmpColumnReader
reader1(""), reader2(""), reader3(""), reader4(""),
same here

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

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
File webkit/fileapi/media/picasa/pmp_table_reader.cc (right):

https://codereview.chromium.org/13529028/diff/18001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/pmp_table_reader.cc:32: bool PmpTableReader::Init()
{
On 2013/04/09 19:16:56, vandebo wrote:
> On 2013/04/09 00:45:34, tommycli wrote:
> > On 2013/04/08 23:57:24, vandebo wrote:
> > > Maybe just put the contents on Init into the constructor.  If init fails,
> then
> > > set some state that you can detect in AddColumn, then fail immediately in
> > > AddColumn if Init would have failed ?
> > 
> > I just added a 
> >   base::ThreadRestrictions::AssertIOAllowed();
> > to Init(). Is that safe to put in a constructor? Should I just add a boolean
> > member to keep track of whether or not construction succeeded?
> 
> If the compiler is happy, I'm happy.
> 
> A Boolean is fine unless there's another member that you want to overload.

Done.

https://codereview.chromium.org/13529028/diff/47001/webkit/fileapi/media/pica...
File webkit/fileapi/media/picasa/picasa_album_data_reader.cc (right):

https://codereview.chromium.org/13529028/diff/47001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_data_reader.cc:20: const uint16
kAlbumCategoryInvalid   = 0xffff; // Sentinel value.
On 2013/04/09 19:16:56, vandebo wrote:
> nit: two spaces before a comment.

Done.

https://codereview.chromium.org/13529028/diff/47001/webkit/fileapi/media/pica...
File webkit/fileapi/media/picasa/picasa_album_data_reader.h (right):

https://codereview.chromium.org/13529028/diff/47001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_data_reader.h:5: #ifndef
WEBKIT_FILEAPI_MEDIA_PICASA_PMP_ALBUM_DATA_TABLE_READER_H_
On 2013/04/09 19:16:56, vandebo wrote:
> Filename and #define should match.

Done.

https://codereview.chromium.org/13529028/diff/47001/webkit/fileapi/media/pica...
File webkit/fileapi/media/picasa/pmp_column_reader.cc (right):

https://codereview.chromium.org/13529028/diff/47001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/pmp_column_reader.cc:50: ParseData(expected_type,
rows_read);
On 2013/04/09 19:16:56, vandebo wrote:
> nit: indent is wrong

Left as is.

https://codereview.chromium.org/13529028/diff/47001/webkit/fileapi/media/pica...
File webkit/fileapi/media/picasa/pmp_column_reader_unittest.cc (right):

https://codereview.chromium.org/13529028/diff/47001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/pmp_column_reader_unittest.cc:70: PmpColumnReader
reader1(""), reader2(""), reader3(""), reader4("");
On 2013/04/09 19:16:56, vandebo wrote:
> probably should put these on multiple lines now... actually each one should be
> declared just before it is used.  Wouldn't hurt to give them a name too.

Done.

https://codereview.chromium.org/13529028/diff/47001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/pmp_column_reader_unittest.cc:113: PmpColumnReader
reader1(""), reader2(""), reader3(""), reader4(""),
On 2013/04/09 19:16:56, vandebo wrote:
> same here

Done.

[vandebo (ex-Chrome), 2013-04-11 20:29:22]

+Greg for a fresh look

LGTM after the following comments.

https://codereview.chromium.org/13529028/diff/54001/webkit/fileapi/media/pica...
File webkit/fileapi/media/picasa/picasa_album_data_reader.cc (right):

https://codereview.chromium.org/13529028/diff/54001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_data_reader.cc:108: #if defined(OS_WIN)
FromUTF8Unsafe

https://codereview.chromium.org/13529028/diff/54001/webkit/fileapi/media/pica...
File webkit/fileapi/media/picasa/picasa_album_data_reader.h (right):

https://codereview.chromium.org/13529028/diff/54001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_data_reader.h:33: class
WEBKIT_STORAGE_EXPORT_PRIVATE PicasaAlbumDataReader {
nit: Does "data" add anything to the name?  How about PicasaAlbumTableReader or
PicasaAlbumListReader ?

https://codereview.chromium.org/13529028/diff/54001/webkit/fileapi/media/pica...
File webkit/fileapi/media/picasa/pmp_table_reader.cc (right):

https://codereview.chromium.org/13529028/diff/54001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/pmp_table_reader.cc:29: column_readers_(),
remove

[tommycli, 2013-04-11 20:51:27]

https://codereview.chromium.org/13529028/diff/54001/webkit/fileapi/media/pica...
File webkit/fileapi/media/picasa/picasa_album_data_reader.cc (right):

https://codereview.chromium.org/13529028/diff/54001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_data_reader.cc:108: #if defined(OS_WIN)
On 2013/04/11 20:29:22, vandebo wrote:
> FromUTF8Unsafe

Done.

https://codereview.chromium.org/13529028/diff/54001/webkit/fileapi/media/pica...
File webkit/fileapi/media/picasa/picasa_album_data_reader.h (right):

https://codereview.chromium.org/13529028/diff/54001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_data_reader.h:33: class
WEBKIT_STORAGE_EXPORT_PRIVATE PicasaAlbumDataReader {
On 2013/04/11 20:29:22, vandebo wrote:
> nit: Does "data" add anything to the name?  How about PicasaAlbumTableReader
or
> PicasaAlbumListReader ?

Done.

https://codereview.chromium.org/13529028/diff/54001/webkit/fileapi/media/pica...
File webkit/fileapi/media/picasa/pmp_table_reader.cc (right):

https://codereview.chromium.org/13529028/diff/54001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/pmp_table_reader.cc:29: column_readers_(),
On 2013/04/11 20:29:22, vandebo wrote:
> remove

Done.

[Greg Billock, 2013-04-11 23:12:44]

https://codereview.chromium.org/13529028/diff/69002/webkit/fileapi/media/pica...
File webkit/fileapi/media/picasa/picasa_album_table_reader.cc (right):

https://codereview.chromium.org/13529028/diff/69002/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_table_reader.cc:58: bool
PicasaAlbumTableReader::Init(const base::FilePath& directory_path) {
Need test.

https://codereview.chromium.org/13529028/diff/69002/webkit/fileapi/media/pica...
File webkit/fileapi/media/picasa/picasa_album_table_reader.h (right):

https://codereview.chromium.org/13529028/diff/69002/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_table_reader.h:35:
PicasaAlbumTableReader();
Any reason this can't take the path argument and get rid of Init?

https://codereview.chromium.org/13529028/diff/69002/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_table_reader.h:38: virtual bool
Init(const base::FilePath& directory_path);
Is this path like the path to a metadata file or something? Add a comment about
how to correctly make one.

https://codereview.chromium.org/13529028/diff/69002/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_table_reader.h:40:
std::vector<FolderInfo> folders() {
Do you want to return const & here and below? Or relying on RVO?

https://codereview.chromium.org/13529028/diff/69002/webkit/fileapi/media/pica...
File webkit/fileapi/media/picasa/pmp_table_reader.cc (right):

https://codereview.chromium.org/13529028/diff/69002/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/pmp_table_reader.cc:24:
PmpTableReader::PmpTableReader(const std::string& table_name,
Should this just take the full indicator_file FilePath? It can get the
directory_path_ from indicator_file.DirName.

https://codereview.chromium.org/13529028/diff/69002/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/pmp_table_reader.cc:32: if
(!file_util::DirectoryExists(directory_path_))
silent failure?

https://codereview.chromium.org/13529028/diff/69002/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/pmp_table_reader.cc:56: scoped_ptr<PmpColumnReader>
column_reader(new PmpColumnReader(column_name));
Seems like this should take the column_file_path, or that it should be doing the
right file arithmetic. Or does it need to know the column name independently?

[tommycli, 2013-04-12 20:47:44]

Added a unit test and addressed gbillock's comments.

https://codereview.chromium.org/13529028/diff/69002/webkit/fileapi/media/pica...
File webkit/fileapi/media/picasa/picasa_album_table_reader.cc (right):

https://codereview.chromium.org/13529028/diff/69002/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_table_reader.cc:58: bool
PicasaAlbumTableReader::Init(const base::FilePath& directory_path) {
On 2013/04/11 23:12:44, Greg Billock wrote:
> Need test.

Done.

https://codereview.chromium.org/13529028/diff/69002/webkit/fileapi/media/pica...
File webkit/fileapi/media/picasa/picasa_album_table_reader.h (right):

https://codereview.chromium.org/13529028/diff/69002/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_table_reader.h:35:
PicasaAlbumTableReader();
On 2013/04/11 23:12:44, Greg Billock wrote:
> Any reason this can't take the path argument and get rid of Init?

Callers need to know when it explicitly failed, and handle that case.

https://codereview.chromium.org/13529028/diff/69002/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_table_reader.h:38: virtual bool
Init(const base::FilePath& directory_path);
On 2013/04/11 23:12:44, Greg Billock wrote:
> Is this path like the path to a metadata file or something? Add a comment
about
> how to correctly make one.

Done.

https://codereview.chromium.org/13529028/diff/69002/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_table_reader.h:40:
std::vector<FolderInfo> folders() {
On 2013/04/11 23:12:44, Greg Billock wrote:
> Do you want to return const & here and below? Or relying on RVO?

Done.

https://codereview.chromium.org/13529028/diff/69002/webkit/fileapi/media/pica...
File webkit/fileapi/media/picasa/pmp_table_reader.cc (right):

https://codereview.chromium.org/13529028/diff/69002/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/pmp_table_reader.cc:24:
PmpTableReader::PmpTableReader(const std::string& table_name,
On 2013/04/11 23:12:44, Greg Billock wrote:
> Should this just take the full indicator_file FilePath? It can get the
> directory_path_ from indicator_file.DirName.

Yes, this is possible, but I want the existence of the indicator file to be an
implementation detail that only the PmpTableReader needs to worry about.

https://codereview.chromium.org/13529028/diff/69002/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/pmp_table_reader.cc:32: if
(!file_util::DirectoryExists(directory_path_))
On 2013/04/11 23:12:44, Greg Billock wrote:
> silent failure?

initialized_ will be false and all attempted calls to AddColumn will return
NULL. The alternative to this would be to add back an Init method.

https://codereview.chromium.org/13529028/diff/69002/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/pmp_table_reader.cc:56: scoped_ptr<PmpColumnReader>
column_reader(new PmpColumnReader(column_name));
On 2013/04/11 23:12:44, Greg Billock wrote:
> Seems like this should take the column_file_path, or that it should be doing
the
> right file arithmetic. Or does it need to know the column name independently?

Done. Good call.

[Greg Billock, 2013-04-12 23:17:58]

https://codereview.chromium.org/13529028/diff/96001/webkit/fileapi/media/pica...
File webkit/fileapi/media/picasa/picasa_album_table_reader.cc (right):

https://codereview.chromium.org/13529028/diff/96001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_table_reader.cc:76: std::string name,
uid;
separate lines

https://codereview.chromium.org/13529028/diff/96001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_table_reader.cc:114: return
initialized_ = true;
Looks like it maybe ought to be "return initialized_ == true". I'd split it into

initialized_ = true;
return true;

https://codereview.chromium.org/13529028/diff/96001/webkit/fileapi/media/pica...
File webkit/fileapi/media/picasa/picasa_album_table_reader_unittest.cc (right):

https://codereview.chromium.org/13529028/diff/96001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_table_reader_unittest.cc:35:
base::FilePath(base::FilePath::FromUTF8Unsafe("C:\\Pix4dalulz"));
Get the base dir from the test machinery with ScopedTempDir? Or from test
helper? This looks like it'll work win only.

https://codereview.chromium.org/13529028/diff/96001/webkit/fileapi/media/pica...
File webkit/fileapi/media/picasa/pmp_table_reader_unittest.cc (right):

https://codereview.chromium.org/13529028/diff/96001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/pmp_table_reader_unittest.cc:45: column_names[0],
column_field_types[0], strings_vector));
I'm not sure I follow this, but the test helper calls this first arg the table
name. Is there a mishap? Or is it like "the table for the column"?

I'm not sure how it's all put together. I'm getting the sense of like one BDB
file per column or something... ?

[tommycli, 2013-04-12 23:44:04]

https://codereview.chromium.org/13529028/diff/96001/webkit/fileapi/media/pica...
File webkit/fileapi/media/picasa/picasa_album_table_reader.cc (right):

https://codereview.chromium.org/13529028/diff/96001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_table_reader.cc:76: std::string name,
uid;
On 2013/04/12 23:17:58, Greg Billock wrote:
> separate lines

Done.

https://codereview.chromium.org/13529028/diff/96001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_table_reader.cc:114: return
initialized_ = true;
On 2013/04/12 23:17:58, Greg Billock wrote:
> Looks like it maybe ought to be "return initialized_ == true". I'd split it
into
> 
> initialized_ = true;
> return true;

Done.

https://codereview.chromium.org/13529028/diff/96001/webkit/fileapi/media/pica...
File webkit/fileapi/media/picasa/picasa_album_table_reader_unittest.cc (right):

https://codereview.chromium.org/13529028/diff/96001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/picasa_album_table_reader_unittest.cc:35:
base::FilePath(base::FilePath::FromUTF8Unsafe("C:\\Pix4dalulz"));
On 2013/04/12 23:17:58, Greg Billock wrote:
> Get the base dir from the test machinery with ScopedTempDir? Or from test
> helper? This looks like it'll work win only.

This is supposed to represent a location where the user has stored his images -
not the directory in PmpTestHelper, which is where the PMP files are.

This test works on Linux. Haven't tried other platforms.

https://codereview.chromium.org/13529028/diff/96001/webkit/fileapi/media/pica...
File webkit/fileapi/media/picasa/pmp_table_reader_unittest.cc (right):

https://codereview.chromium.org/13529028/diff/96001/webkit/fileapi/media/pica...
webkit/fileapi/media/picasa/pmp_table_reader_unittest.cc:45: column_names[0],
column_field_types[0], strings_vector));
On 2013/04/12 23:17:58, Greg Billock wrote:
> I'm not sure I follow this, but the test helper calls this first arg the table
> name. Is there a mishap? Or is it like "the table for the column"?
> 
> I'm not sure how it's all put together. I'm getting the sense of like one BDB
> file per column or something... ?

I just recently changed the signature of WriteColumnFileFromVector to take the
column_name in the first argument.

And yes - it's one file per column (kind of messed up).

[Greg Billock, 2013-04-16 19:01:05]

lgtm

[Avi (use Gerrit), 2013-04-16 19:05:03]

lgtm as it matches up with existing code...

but I'm not happy about all this picasa code being there.

[darin (slow to review), 2013-04-16 19:38:52]

It looks like OWNERS review is still required.  I feel like this code does not
belong in this module.

[tommycli, 2013-04-23 17:53:04]

jam: This change is based on the CL you just reviewed
(https://codereview.chromium.org/14247034). It puts the new picasa stuff in the
chrome/browser/media_galleries directory.

[tommycli, 2013-04-23 17:54:01]

jam: I need an lgtm since it modifies the chrome/*.gypi files.

[jam, 2013-04-23 20:30:06]

On 2013/04/23 17:54:01, tommycli wrote:
> jam: I need an lgtm since it modifies the chrome/*.gypi files.

please pick one of the reviewers from chrome/owners

[tommycli, 2013-04-23 22:12:42]

thestig: need lgtm on chrome/*.gypi files.

[Lei Zhang, 2013-04-23 22:22:55]

lgtm since the code has been moved out of webkit/fileapi

[commit-bot: I haz the power, 2013-04-24 23:57:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/13529028/130001

[commit-bot: I haz the power, 2013-04-24 23:59:55]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...

[commit-bot: I haz the power, 2013-04-25 00:07:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/13529028/142001

[commit-bot: I haz the power, 2013-04-25 00:38:20]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-25 01:38:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/13529028/149001

[commit-bot: I haz the power, 2013-04-25 04:25:07]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-04-25 04:37:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/13529028/149001

[commit-bot: I haz the power, 2013-04-25 05:47:45]

Change committed as 196323