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

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

BUG=151701
R=vandebo

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

[tommycli, 2013-06-19 22:58:39]

vandebo: Need cc/h reviews.
thestig: Need gypi review.

https://codereview.chromium.org/17101030/diff/1/chrome/browser/media_gallerie...
File chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc
(right):

https://codereview.chromium.org/17101030/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:93:
PicasaAlbumTableReader album_table_reader((album_table_files));
Need parens for most vexing parse issue.

[tommycli, 2013-06-19 22:59:21]

+thestig

[Lei Zhang, 2013-06-19 23:02:59]

I only see 3 deleted file, but there's 5 lines removed from gypi files.

[tommycli, 2013-06-19 23:44:56]

On 2013/06/19 23:02:59, Lei Zhang wrote:
> I only see 3 deleted file, but there's 5 lines removed from gypi files.

I removed the 3 deleted lines, and also deleted 2 duplicate lines in
chrome_browser.gypi.

[tommycli, 2013-06-20 20:55:51]

On 2013/06/19 23:44:56, tommycli wrote:
> On 2013/06/19 23:02:59, Lei Zhang wrote:
> > I only see 3 deleted file, but there's 5 lines removed from gypi files.
> 
> I removed the 3 deleted lines, and also deleted 2 duplicate lines in
> chrome_browser.gypi.

Delay review - I'm moving this to common.

[tommycli, 2013-06-20 21:40:26]

thestig/vandebo: Ready for review.

The code is the same as Patchset 3, but I moved everything over to
common/media_galleries, as that's where it belongs.

[Greg Billock, 2013-06-20 22:06:37]

https://codereview.chromium.org/17101030/diff/3015/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/picasa/picasa_album_table_reader.cc
(right):

https://codereview.chromium.org/17101030/diff/3015/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_album_table_reader.cc:33:
return base::CreatePlatformFile(path, flags, NULL, NULL);
Is this guaranteed not to open the file? Looking at the implementation, it seems
like probably not, but it's a bit hard to tell. It seems like the struct being
built ought to be filenames, and make the PlatformFile's when they get opened.

https://codereview.chromium.org/17101030/diff/3015/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_album_table_reader.cc:93:
if (!category_column.Init(table_files_.category_file, PMP_TYPE_UINT32) ||
Seeing this here, I think "ReadColumn" or something like that would be better
than Init().

https://codereview.chromium.org/17101030/diff/3015/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_album_table_reader.cc:103:
row_count = std::max(row_count, category_column.rows());
Given that !reader.ReadXxx(i) -> continue; below, should this really be
std::min?

https://codereview.chromium.org/17101030/diff/3015/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/picasa/pmp_column_reader.h (right):

https://codereview.chromium.org/17101030/diff/3015/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/pmp_column_reader.h:42: uint32
rows() const;
How about rows_read()

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

https://codereview.chromium.org/17101030/diff/3015/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/picasa/picasa_album_table_reader.cc
(right):

https://codereview.chromium.org/17101030/diff/3015/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_album_table_reader.cc:33:
return base::CreatePlatformFile(path, flags, NULL, NULL);
On 2013/06/20 22:06:38, Greg Billock wrote:
> Is this guaranteed not to open the file? Looking at the implementation, it
seems
> like probably not, but it's a bit hard to tell. It seems like the struct being
> built ought to be filenames, and make the PlatformFile's when they get opened.

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

https://codereview.chromium.org/17101030/diff/3015/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_album_table_reader.cc:93:
if (!category_column.Init(table_files_.category_file, PMP_TYPE_UINT32) ||
On 2013/06/20 22:06:38, Greg Billock wrote:
> Seeing this here, I think "ReadColumn" or something like that would be better
> than Init().

Done.

https://codereview.chromium.org/17101030/diff/3015/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_album_table_reader.cc:103:
row_count = std::max(row_count, category_column.rows());
On 2013/06/20 22:06:38, Greg Billock wrote:
> Given that !reader.ReadXxx(i) -> continue; below, should this really be
> std::min?

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

https://codereview.chromium.org/17101030/diff/3015/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/picasa/pmp_column_reader.h (right):

https://codereview.chromium.org/17101030/diff/3015/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/pmp_column_reader.h:42: uint32
rows() const;
On 2013/06/20 22:06:38, Greg Billock wrote:
> How about rows_read()

Done.

[Lei Zhang, 2013-06-21 02:15:50]

https://codereview.chromium.org/17101030/diff/39001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/picasa/picasa_album_table_reader.h
(right):

https://codereview.chromium.org/17101030/diff/39001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_album_table_reader.h:43:
void CloseAll();
One you start adding methods, classes are preferred. Since there's only 1
method, just write it as a function that takes a PicasaAlbumTableFiles* ?

https://codereview.chromium.org/17101030/diff/39001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_album_table_reader.h:46: 
nit: why the extra blank line. Is |indicator_file| special in some way?

https://codereview.chromium.org/17101030/diff/39001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_album_table_reader.h:66:
PicasaAlbumTableFiles table_files_;
looks like this can be const.

https://codereview.chromium.org/17101030/diff/39001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/picasa/pmp_column_reader.cc (right):

https://codereview.chromium.org/17101030/diff/39001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/pmp_column_reader.cc:29: bool
PmpColumnReader::ReadFile(const base::PlatformFile& file,
PlatformFiles are just ints/HANDLEs. I think people usually just pass them by
value.

https://codereview.chromium.org/17101030/diff/39001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/picasa/pmp_test_helper.cc (right):

https://codereview.chromium.org/17101030/diff/39001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/pmp_test_helper.cc:136:
file_util::Delete(temp_path, true);
You probably don't want the second parameter to be true here.

[tommycli, 2013-06-21 16:14:23]

Thanks for review. Addressed gbillock and thestig comments.

https://codereview.chromium.org/17101030/diff/39001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/picasa/picasa_album_table_reader.h
(right):

https://codereview.chromium.org/17101030/diff/39001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_album_table_reader.h:43:
void CloseAll();
On 2013/06/21 02:15:50, Lei Zhang wrote:
> One you start adding methods, classes are preferred. Since there's only 1
> method, just write it as a function that takes a PicasaAlbumTableFiles* ?

Sounds good.

https://codereview.chromium.org/17101030/diff/39001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_album_table_reader.h:46: 
On 2013/06/21 02:15:50, Lei Zhang wrote:
> nit: why the extra blank line. Is |indicator_file| special in some way?

Yes, indicator_file is a special empty file used to confirm the existence of the
table. I'll add a comment to that effect.

https://codereview.chromium.org/17101030/diff/39001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_album_table_reader.h:66:
PicasaAlbumTableFiles table_files_;
On 2013/06/21 02:15:50, Lei Zhang wrote:
> looks like this can be const.

Done.

https://codereview.chromium.org/17101030/diff/39001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/picasa/pmp_column_reader.cc (right):

https://codereview.chromium.org/17101030/diff/39001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/pmp_column_reader.cc:29: bool
PmpColumnReader::ReadFile(const base::PlatformFile& file,
On 2013/06/21 02:15:50, Lei Zhang wrote:
> PlatformFiles are just ints/HANDLEs. I think people usually just pass them by
> value.

Done.

https://codereview.chromium.org/17101030/diff/39001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/picasa/pmp_test_helper.cc (right):

https://codereview.chromium.org/17101030/diff/39001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/pmp_test_helper.cc:136:
file_util::Delete(temp_path, true);
On 2013/06/21 02:15:50, Lei Zhang wrote:
> You probably don't want the second parameter to be true here.

Done.

[Lei Zhang, 2013-06-21 17:26:05]

No more concerns from me, lgtm.

[commit-bot: I haz the power, 2013-06-21 17:30:40]

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

[tommycli, 2013-06-21 17:31:10]

I checked the CQ box. There will be about five hours for anyone else to raise
objections.

[commit-bot: I haz the power, 2013-06-21 22:40:51]

Change committed as 207941