Media Galleries API Picasa: Make PicasaFileUtil support album contents.

Media Galleries API Picasa: Make PicasaFileUtil support album contents.

Depends on:
https://codereview.chromium.org/18986012/

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

[tommycli, 2013-08-20 16:34:40]

gbillock: Need OWNER review

[Greg Billock, 2013-08-21 17:13:17]

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

https://codereview.chromium.org/22912008/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util.cc:52: // This
method returns a pointer to the AlbumImages instead of a
You could have const AlbumImages** as an outparam.

https://codereview.chromium.org/22912008/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util.cc:54: const
AlbumImages* FindAlbumImages(const std::string& key,
Should this be a method in the data provider?

https://codereview.chromium.org/22912008/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util.cc:59: if (!map)
{
should this be DCHECK?

https://codereview.chromium.org/22912008/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util.cc:80: if
(components.size() >= 2 && components[0] == kPicasaDirAlbums)
How about adding a comment to PicasaFileUtil explaining how the virtual FS is
laid out. I'm getting a /albums/album-name/filename impression, but I had to
work for it. :-)

https://codereview.chromium.org/22912008/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util.cc:116:
GetNeededDataType(url),
GetDataTypeForUrl ?

https://codereview.chromium.org/22912008/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util.cc:326:
GetDataProvider()->GetAlbumsImages();
Is this file missing from the change? I don't see GetAlbumsImages in the class.

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

https://codereview.chromium.org/22912008/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc:50:
// Defined up here because used by TestFolder.
can omit comment

https://codereview.chromium.org/22912008/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc:562:
EXPECT_FALSE(contents.begin()->is_directory);
Need to check the actual on-disk filename you get back to make sure its the
img.jpg filename?

[tommycli, 2013-08-21 21:37:06]

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

https://codereview.chromium.org/22912008/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util.cc:52: // This
method returns a pointer to the AlbumImages instead of a
On 2013/08/21 17:13:17, Greg Billock wrote:
> You could have const AlbumImages** as an outparam.

I moved it to the data provider. Can't do AlbumImages** there because need to
make a copy of the struct.

https://codereview.chromium.org/22912008/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util.cc:54: const
AlbumImages* FindAlbumImages(const std::string& key,
On 2013/08/21 17:13:17, Greg Billock wrote:
> Should this be a method in the data provider?

Yes, that's actually much better. I put those changes into the ancestor CL:
https://codereview.chromium.org/18986012/

https://codereview.chromium.org/22912008/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util.cc:59: if (!map)
{
On 2013/08/21 17:13:17, Greg Billock wrote:
> should this be DCHECK?

Done.

https://codereview.chromium.org/22912008/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util.cc:80: if
(components.size() >= 2 && components[0] == kPicasaDirAlbums)
On 2013/08/21 17:13:17, Greg Billock wrote:
> How about adding a comment to PicasaFileUtil explaining how the virtual FS is
> laid out. I'm getting a /albums/album-name/filename impression, but I had to
> work for it. :-)

Done. Good idea.

https://codereview.chromium.org/22912008/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util.cc:116:
GetNeededDataType(url),
On 2013/08/21 17:13:17, Greg Billock wrote:
> GetDataTypeForUrl ?

Done.

https://codereview.chromium.org/22912008/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util.cc:326:
GetDataProvider()->GetAlbumsImages();
On 2013/08/21 17:13:17, Greg Billock wrote:
> Is this file missing from the change? I don't see GetAlbumsImages in the
class.

Yes, this CL depends on https://codereview.chromium.org/18986012/. I updated the
description. Sorry.

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

https://codereview.chromium.org/22912008/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc:50:
// Defined up here because used by TestFolder.
On 2013/08/21 17:13:17, Greg Billock wrote:
> can omit comment

Done.

https://codereview.chromium.org/22912008/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc:562:
EXPECT_FALSE(contents.begin()->is_directory);
On 2013/08/21 17:13:17, Greg Billock wrote:
> Need to check the actual on-disk filename you get back to make sure its the
> img.jpg filename?

I can't easily access that information. The file util internally maps the
filename to the platform path when i try to access it or get the metadata, but
the actual platform path is not part of its public interface.

[Greg Billock, 2013-08-21 21:41:23]

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

https://codereview.chromium.org/22912008/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc:562:
EXPECT_FALSE(contents.begin()->is_directory);
On 2013/08/21 21:37:06, tommycli wrote:
> On 2013/08/21 17:13:17, Greg Billock wrote:
> > Need to check the actual on-disk filename you get back to make sure its the
> > img.jpg filename?
> 
> I can't easily access that information. The file util internally maps the
> filename to the platform path when i try to access it or get the metadata, but
> the actual platform path is not part of its public interface.

Is there a way to prove we can read it? Get the file contents and compare them
to WriteJPEGHeader?

[tommycli, 2013-08-21 22:19:26]

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

https://codereview.chromium.org/22912008/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc:562:
EXPECT_FALSE(contents.begin()->is_directory);
On 2013/08/21 21:41:23, Greg Billock wrote:
> On 2013/08/21 21:37:06, tommycli wrote:
> > On 2013/08/21 17:13:17, Greg Billock wrote:
> > > Need to check the actual on-disk filename you get back to make sure its
the
> > > img.jpg filename?
> > 
> > I can't easily access that information. The file util internally maps the
> > filename to the platform path when i try to access it or get the metadata,
but
> > the actual platform path is not part of its public interface.
> 
> Is there a way to prove we can read it? Get the file contents and compare them
> to WriteJPEGHeader?

Seems like creating a snapshot file is a way to verify the platform path. I did
that. Check it out.

[Greg Billock, 2013-08-22 16:01:00]

On 2013/08/21 22:19:26, tommycli wrote:
>
https://codereview.chromium.org/22912008/diff/1/chrome/browser/media_gallerie...
> File
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/22912008/diff/1/chrome/browser/media_gallerie...
>
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc:562:
> EXPECT_FALSE(contents.begin()->is_directory);
> On 2013/08/21 21:41:23, Greg Billock wrote:
> > On 2013/08/21 21:37:06, tommycli wrote:
> > > On 2013/08/21 17:13:17, Greg Billock wrote:
> > > > Need to check the actual on-disk filename you get back to make sure its
> the
> > > > img.jpg filename?
> > > 
> > > I can't easily access that information. The file util internally maps the
> > > filename to the platform path when i try to access it or get the metadata,
> but
> > > the actual platform path is not part of its public interface.
> > 
> > Is there a way to prove we can read it? Get the file contents and compare
them
> > to WriteJPEGHeader?
> 
> Seems like creating a snapshot file is a way to verify the platform path. I
did
> that. Check it out.

looks good.

lgtm

[commit-bot: I haz the power, 2013-09-03 14:21:31]

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

[commit-bot: I haz the power, 2013-09-03 18:42:28]

Change committed as 220988