iTunes file util and data provider for media galleries

iTunes file util and data provider for media galleries

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

[vandebo (ex-Chrome), 2013-06-05 20:54:25]

Checkpoint - compiles

[vandebo (ex-Chrome), 2013-06-05 22:18:04]

Basic bug fixes

[vandebo (ex-Chrome), 2013-06-05 22:49:32]

https://codereview.chromium.org/16158004/diff/6001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/native_media_file_util.cc (right):

https://codereview.chromium.org/16158004/diff/6001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/native_media_file_util.cc:101: // static
Same here - just reordering methods to be in the right order - they were not
right even before the header reordering.

https://codereview.chromium.org/16158004/diff/6001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/native_media_file_util.h (right):

https://codereview.chromium.org/16158004/diff/6001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/native_media_file_util.h:22: // Uses the
MIME sniffer code, which actually looks into the file,
All changes in this file are just movement and reorderings.

https://codereview.chromium.org/16158004/diff/6001/chrome/browser/media_galle...
File chrome/browser/media_galleries/media_file_system_registry.cc (left):

https://codereview.chromium.org/16158004/diff/6001/chrome/browser/media_galle...
chrome/browser/media_galleries/media_file_system_registry.cc:597:
IsolatedContext::GetInstance()->RevokeFileSystem(fsid);
This was a bug, we never tried to unregister picasa file systems.

[vandebo (ex-Chrome), 2013-06-06 03:45:41]

+cc tommycli

Tommy, since you've been working with this code a lot, feel free to comment on
anything that's not right.

[Lei Zhang, 2013-06-06 03:48:47]

Here's one incomplete pass.

https://codereview.chromium.org/16158004/diff/6001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/native_media_file_util.cc (right):

https://codereview.chromium.org/16158004/diff/6001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/native_media_file_util.cc:101: // static
On 2013/06/05 22:49:32, vandebo wrote:
> Same here - just reordering methods to be in the right order - they were not
> right even before the header reordering.

Ok, maybe in a separate CL next time?

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/itunes_data_provider.h (right):

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider.h:13: #include
"base/callback.h"
base/callback_forward.h instead?

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider.h:19: class
ITunesDataProvider {
Needs some comment to explain what this does.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider.h:26: virtual
~ITunesDataProvider();
no virtual

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider.h:33: base::FilePath
library_path() const;
return const ref.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider.h:42: bool
GetTrackLocation(const ArtistName& artist, const AlbumName& album,
Just return an empty FilePath on failure?

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider.h:60: static Album
MakeUniqueTrackNames(const ITunesLibraryParser::Album& album);
Put this in the anonymous namespace instead?

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider.h:64: base::FilePath
library_path_;
const

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
File chrome/browser/media_galleries/imported_media_gallery_registry.cc (right):

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/imported_media_gallery_registry.cc:69:
base::Unretained(GetInstance()), database_path));
Since this is no longer static, use this instead of GetInstance() ?

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/imported_media_gallery_registry.cc:73:
DCHECK_EQ(picasa_database_path_, database_path);
If these were FilePaths, you need to compare their .value()s instead. Sigh.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/imported_media_gallery_registry.cc:81: const
base::FilePath& library_xml_path) {
DCHECK the path is not empty?

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/imported_media_gallery_registry.cc:91: return "";
FWIW, this can't actually be reached. If we reach this line, we'll just fail a
CHECK() in the current caller.

We should return std::string() BTW.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/imported_media_gallery_registry.cc:103:
DCHECK_EQ(itumes_xml_library_path_, library_xml_path);
typo: itumes

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/imported_media_gallery_registry.cc:117: if
(picasa_fsids_.erase(fsid) && !picasa_fsids_.size()) {
!foo_set.size() -> foo_set.empty()

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/imported_media_gallery_registry.cc:117: if
(picasa_fsids_.erase(fsid) && !picasa_fsids_.size()) {
If |picasa_fsids_| has 2 elements, and you erase 1, you shouldn't need to go on
and check |itunes_fsids_| below.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/imported_media_gallery_registry.cc:147:
ImportedMediaGalleryRegistry::itunes_data_provider() {
nit: These are not simple getters, and should not be named as such.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
File chrome/browser/media_galleries/imported_media_gallery_registry.h (right):

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/imported_media_gallery_registry.h:61:
scoped_ptr<picasa::PicasaDataProvider> picasa_data_provider_;
You should mention which variables are accessed on which thread.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/imported_media_gallery_registry.h:68: std::string
picasa_database_path_;
These need to be FilePaths to compile x-platform.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
File chrome/browser/media_galleries/media_file_system_registry.cc (right):

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/media_file_system_registry.cc:561: std::string
fs_name(extension_misc::kMediaFileSystemPathPart);
nit: you can move |fs_name| down into the else block.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
File chrome/browser/media_galleries/media_galleries_preferences.cc (right):

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/media_galleries_preferences.cc:269: //
TODO(vandebo): Amend any existing iTunes gallery instead of adding anew.
nit: Add more context. This is for the corner case of the user changing their
iTunes folder, right?

[Lei Zhang, 2013-06-06 09:58:52]

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.cc (right):

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.cc:32:
fileapi::DirectoryEntry MakeDirectoryEntryFilePathType(
Feels like DirectoryEntry needs a ctor.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.cc:111: return
MakeDirectoryFileInfo(file_info);
Don't you need to fill in the directory name for |file_info|? Ditto for cases 2
and 3.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.cc:193:
file_list->push_back(MakeDirectoryEntryUTF8(it->first, file_info.size,
nit: needs {} around this block.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.cc:234: void
ItunesFileUtil::GetFileInfoWithFreshDataProvider(
Do you even need this and ReadDirectoryWithFreshDataProvider?

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.cc:249:
chrome::ImportedMediaGalleryRegistry* imported_registry =
save this value instead of calling GetInstance() every time?

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/itunes_data_provider.cc (right):

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider.cc:20: const int64
kMaxLibraryFileSize = 50 * 1024 * 1024;
If you search, you will find users reporting having xml files bigger than this.
People love their music. I'd double it for now and add a TODO to add a UMA.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider.cc:59: if
(!needs_refresh_) {
If you write this the other way around, it's only 5 lines:

  if (needs_refresh_) {
    ParseLibrary();
    needs_refresh_ = false;
  }
  ready_callback.Run();

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider.cc:159: typedef
std::set<const ITunesLibraryParser::Track*> TrackRefs;
What about a std::multimap instead of these two structures? Code will instead
look roughly like:

typedef std::multimap<TrackName, const ITunesLibraryParser::Track*> AlbumInfo;
AlbumInfo info;
Album result;

ITunesLibraryParser::Album::const_iterator album_it;
for (album_it = album.begin(); album_it != album.end(); ++album_it) {
  const ITunesLibraryParser::Track& track = *album_it;
  std::string name = track.location.BaseName().AsUTF8Unsafe();
  info.insert(std::make_pair(name, &track);
}

for (AlbumInfo::iterator it = info.begin();
     it != info.end();
     it = info.upper_bound(it->first)) {
  if (info.count(it->first) == 1) {
    result[it->first] = it->second->location;
  } else {
    std::pair<AlbumInfo::iterator, AlbumInfo::iterator> dups =
      info.equal_range(it->first);
    for (AlbumInfo::iterator dup_it = dups.first;
         dup_it != dups.second;
         ++dup_it) {
      std::string id =
          base::StringPrintf(" (%" PRId64 ")", (*dup_it)->id);
      base::FilePath unique_name =
          (*dup_it)->location.BaseName().InsertBeforeExtensionASCII(id);
      result[unique_name.AsUTF8Unsafe()] = (*dup_it)->location;
    }
  }
}

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider.cc:166: for
(album_it = album.begin(); album_it != album.end(); ++album_it) {
a "const ITunesLibraryParser::Track& track = *album_it;" can help readability
and avoid "&*album_it"

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider.cc:174: if
(name_it->second.size() == 1) {
name_it->second can also use a const ref alias inside this loop for readability.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/itunes_data_provider.h (right):

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider.h:19: class
ITunesDataProvider {
On 2013/06/06 03:48:47, Lei Zhang wrote:
> Needs some comment to explain what this does.

Like, for instance, what thread does this run on? Can we have some DCHECKs to
verify that?

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider.h:46:
std::set<ArtistName> GetArtists() const;
How about GetArtistNames, GetAlbumNames, and GetAlbum? Make
ItunesDataProvider::Album public and use it where convenient instead of
std:map<...>

[vandebo (ex-Chrome), 2013-06-06 19:49:18]

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.cc (right):

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.cc:32:
fileapi::DirectoryEntry MakeDirectoryEntryFilePathType(
On 2013/06/06 09:58:54, Lei Zhang wrote:
> Feels like DirectoryEntry needs a ctor.

Indeed, I think it just moved out of base, I will suggest this.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.cc:111: return
MakeDirectoryFileInfo(file_info);
On 2013/06/06 09:58:54, Lei Zhang wrote:
> Don't you need to fill in the directory name for |file_info|? Ditto for cases
2
> and 3.

base::PlatformFileInfo doesn't have a name field.  The caller already knows the
virtual name (in |url|) and has looked up the real name (in |platform_path| by
calling GetLocalFilePath()

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.cc:193:
file_list->push_back(MakeDirectoryEntryUTF8(it->first, file_info.size,
On 2013/06/06 09:58:54, Lei Zhang wrote:
> nit: needs {} around this block.

Done.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.cc:234: void
ItunesFileUtil::GetFileInfoWithFreshDataProvider(
On 2013/06/06 09:58:54, Lei Zhang wrote:
> Do you even need this and ReadDirectoryWithFreshDataProvider?

Yes, bind wouldn't bind to a protected method of a super class - I tried that
first.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.cc:249:
chrome::ImportedMediaGalleryRegistry* imported_registry =
On 2013/06/06 09:58:54, Lei Zhang wrote:
> save this value instead of calling GetInstance() every time?

Done.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/itunes_data_provider.cc (right):

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider.cc:20: const int64
kMaxLibraryFileSize = 50 * 1024 * 1024;
On 2013/06/06 09:58:54, Lei Zhang wrote:
> If you search, you will find users reporting having xml files bigger than
this.
> People love their music. I'd double it for now and add a TODO to add a UMA.

Done.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider.cc:59: if
(!needs_refresh_) {
On 2013/06/06 09:58:54, Lei Zhang wrote:
> If you write this the other way around, it's only 5 lines:
> 
>   if (needs_refresh_) {
>     ParseLibrary();
>     needs_refresh_ = false;
>   }
>   ready_callback.Run();

Done.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider.cc:159: typedef
std::set<const ITunesLibraryParser::Track*> TrackRefs;
On 2013/06/06 09:58:54, Lei Zhang wrote:
> What about a std::multimap instead of these two structures? Code will instead
> look roughly like:
> 
> typedef std::multimap<TrackName, const ITunesLibraryParser::Track*> AlbumInfo;
> AlbumInfo info;
> Album result;
> 
> ITunesLibraryParser::Album::const_iterator album_it;
> for (album_it = album.begin(); album_it != album.end(); ++album_it) {
>   const ITunesLibraryParser::Track& track = *album_it;
>   std::string name = track.location.BaseName().AsUTF8Unsafe();
>   info.insert(std::make_pair(name, &track);
> }
> 
> for (AlbumInfo::iterator it = info.begin();
>      it != info.end();
>      it = info.upper_bound(it->first)) {
>   if (info.count(it->first) == 1) {
>     result[it->first] = it->second->location;
>   } else {
>     std::pair<AlbumInfo::iterator, AlbumInfo::iterator> dups =
>       info.equal_range(it->first);
>     for (AlbumInfo::iterator dup_it = dups.first;
>          dup_it != dups.second;
>          ++dup_it) {
>       std::string id =
>           base::StringPrintf(" (%" PRId64 ")", (*dup_it)->id);
>       base::FilePath unique_name =
>           (*dup_it)->location.BaseName().InsertBeforeExtensionASCII(id);
>       result[unique_name.AsUTF8Unsafe()] = (*dup_it)->location;
>     }
>   }
> }

multimap is an interesting idea, but to me, it looks like the existing code is
simpler.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider.cc:166: for
(album_it = album.begin(); album_it != album.end(); ++album_it) {
On 2013/06/06 09:58:54, Lei Zhang wrote:
> a "const ITunesLibraryParser::Track& track = *album_it;" can help readability
> and avoid "&*album_it"

Done.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider.cc:174: if
(name_it->second.size() == 1) {
On 2013/06/06 09:58:54, Lei Zhang wrote:
> name_it->second can also use a const ref alias inside this loop for
readability.

Done.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/itunes_data_provider.h (right):

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider.h:13: #include
"base/callback.h"
On 2013/06/06 03:48:47, Lei Zhang wrote:
> base/callback_forward.h instead?

Done.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider.h:19: class
ITunesDataProvider {
On 2013/06/06 03:48:47, Lei Zhang wrote:
> Needs some comment to explain what this does.

Done.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider.h:19: class
ITunesDataProvider {
On 2013/06/06 09:58:54, Lei Zhang wrote:
> On 2013/06/06 03:48:47, Lei Zhang wrote:
> > Needs some comment to explain what this does.
> 
> Like, for instance, what thread does this run on? Can we have some DCHECKs to
> verify that?

It doesn't really care which thread it's on.  There's a DCHECK that IO is
allowed.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider.h:26: virtual
~ITunesDataProvider();
On 2013/06/06 03:48:47, Lei Zhang wrote:
> no virtual

Done.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider.h:33: base::FilePath
library_path() const;
On 2013/06/06 03:48:47, Lei Zhang wrote:
> return const ref.

Done.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider.h:42: bool
GetTrackLocation(const ArtistName& artist, const AlbumName& album,
On 2013/06/06 03:48:47, Lei Zhang wrote:
> Just return an empty FilePath on failure?

Done.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider.h:46:
std::set<ArtistName> GetArtists() const;
On 2013/06/06 09:58:54, Lei Zhang wrote:
> How about GetArtistNames, GetAlbumNames, and GetAlbum? Make
> ItunesDataProvider::Album public and use it where convenient instead of
> std:map<...>

Done.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider.h:60: static Album
MakeUniqueTrackNames(const ITunesLibraryParser::Album& album);
On 2013/06/06 03:48:47, Lei Zhang wrote:
> Put this in the anonymous namespace instead?

I started there, but Album was not public and it made things a lot uglier. 
Done.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider.h:64: base::FilePath
library_path_;
On 2013/06/06 03:48:47, Lei Zhang wrote:
> const

Done.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
File chrome/browser/media_galleries/imported_media_gallery_registry.cc (right):

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/imported_media_gallery_registry.cc:69:
base::Unretained(GetInstance()), database_path));
On 2013/06/06 03:48:47, Lei Zhang wrote:
> Since this is no longer static, use this instead of GetInstance() ?

Done.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/imported_media_gallery_registry.cc:73:
DCHECK_EQ(picasa_database_path_, database_path);
On 2013/06/06 03:48:47, Lei Zhang wrote:
> If these were FilePaths, you need to compare their .value()s instead. Sigh.

Done.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/imported_media_gallery_registry.cc:81: const
base::FilePath& library_xml_path) {
On 2013/06/06 03:48:47, Lei Zhang wrote:
> DCHECK the path is not empty?

Done.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/imported_media_gallery_registry.cc:91: return "";
On 2013/06/06 03:48:47, Lei Zhang wrote:
> FWIW, this can't actually be reached. If we reach this line, we'll just fail a
> CHECK() in the current caller.

It can fail in IsolatedContext, so I fixed the callers.

> We should return std::string() BTW.

Done.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/imported_media_gallery_registry.cc:103:
DCHECK_EQ(itumes_xml_library_path_, library_xml_path);
On 2013/06/06 03:48:47, Lei Zhang wrote:
> typo: itumes

Done.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/imported_media_gallery_registry.cc:117: if
(picasa_fsids_.erase(fsid) && !picasa_fsids_.size()) {
On 2013/06/06 03:48:47, Lei Zhang wrote:
> !foo_set.size() -> foo_set.empty()

Done.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/imported_media_gallery_registry.cc:117: if
(picasa_fsids_.erase(fsid) && !picasa_fsids_.size()) {
On 2013/06/06 03:48:47, Lei Zhang wrote:
> If |picasa_fsids_| has 2 elements, and you erase 1, you shouldn't need to go
on
> and check |itunes_fsids_| below.

Done.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/imported_media_gallery_registry.cc:147:
ImportedMediaGalleryRegistry::itunes_data_provider() {
On 2013/06/06 03:48:47, Lei Zhang wrote:
> nit: These are not simple getters, and should not be named as such.

Done.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
File chrome/browser/media_galleries/imported_media_gallery_registry.h (right):

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/imported_media_gallery_registry.h:61:
scoped_ptr<picasa::PicasaDataProvider> picasa_data_provider_;
On 2013/06/06 03:48:47, Lei Zhang wrote:
> You should mention which variables are accessed on which thread.

Done.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/imported_media_gallery_registry.h:68: std::string
picasa_database_path_;
On 2013/06/06 03:48:47, Lei Zhang wrote:
> These need to be FilePaths to compile x-platform.

Done.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
File chrome/browser/media_galleries/media_file_system_registry.cc (right):

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/media_file_system_registry.cc:561: std::string
fs_name(extension_misc::kMediaFileSystemPathPart);
On 2013/06/06 03:48:47, Lei Zhang wrote:
> nit: you can move |fs_name| down into the else block.

Done.

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
File chrome/browser/media_galleries/media_galleries_preferences.cc (right):

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/media_galleries_preferences.cc:269: //
TODO(vandebo): Amend any existing iTunes gallery instead of adding anew.
On 2013/06/06 03:48:47, Lei Zhang wrote:
> nit: Add more context. This is for the corner case of the user changing their
> iTunes folder, right?

Done.

[vandebo (ex-Chrome), 2013-06-06 22:45:07]

+kinuko pleas review content/browser/fileapi/fileapi_message_filter.cc

[Lei Zhang, 2013-06-07 03:29:07]

lgtm

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
File chrome/browser/media_galleries/imported_media_gallery_registry.cc (right):

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/imported_media_gallery_registry.cc:91: return "";
On 2013/06/06 19:49:18, vandebo wrote:
> On 2013/06/06 03:48:47, Lei Zhang wrote:
> > FWIW, this can't actually be reached. If we reach this line, we'll just fail
a
> > CHECK() in the current caller.
> 
> It can fail in IsolatedContext, so I fixed the callers.

With the current implementation and the parameters we pass in, it can't fail.

I think there are multiple layers where we all have DCHECKS that the fsid is
non-empty, now that you removed the one CHECK.

https://codereview.chromium.org/16158004/diff/24002/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/itunes_data_provider.cc (right):

https://codereview.chromium.org/16158004/diff/24002/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes_data_provider.cc:62: std::string
name = album_it->location.BaseName().AsUTF8Unsafe();
nit: declare |track| first and put name = track.location...

https://codereview.chromium.org/16158004/diff/24002/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes_data_provider.cc:207: };
nit: extra ; at the end

[vandebo (ex-Chrome), 2013-06-07 03:46:52]

thanks

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
File chrome/browser/media_galleries/imported_media_gallery_registry.cc (right):

https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/imported_media_gallery_registry.cc:91: return "";
On 2013/06/07 03:29:07, Lei Zhang wrote:
> On 2013/06/06 19:49:18, vandebo wrote:
> > On 2013/06/06 03:48:47, Lei Zhang wrote:
> > > FWIW, this can't actually be reached. If we reach this line, we'll just
fail
> a
> > > CHECK() in the current caller.
> > 
> > It can fail in IsolatedContext, so I fixed the callers.
> 
> With the current implementation and the parameters we pass in, it can't fail.
> 
> I think there are multiple layers where we all have DCHECKS that the fsid is
> non-empty, now that you removed the one CHECK.

Are you saying that we should add a CHECK in IsolatedContext and not have the
failure case?  That seems fine to me.

https://codereview.chromium.org/16158004/diff/24002/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/itunes_data_provider.cc (right):

https://codereview.chromium.org/16158004/diff/24002/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes_data_provider.cc:62: std::string
name = album_it->location.BaseName().AsUTF8Unsafe();
On 2013/06/07 03:29:07, Lei Zhang wrote:
> nit: declare |track| first and put name = track.location...

Done.

https://codereview.chromium.org/16158004/diff/24002/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes_data_provider.cc:207: };
On 2013/06/07 03:29:07, Lei Zhang wrote:
> nit: extra ; at the end

Done.

[Lei Zhang, 2013-06-07 04:32:07]

On 2013/06/07 03:46:52, vandebo wrote:
> thanks
> 
>
https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
> File chrome/browser/media_galleries/imported_media_gallery_registry.cc
(right):
> 
>
https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galle...
> chrome/browser/media_galleries/imported_media_gallery_registry.cc:91: return
"";
> On 2013/06/07 03:29:07, Lei Zhang wrote:
> > On 2013/06/06 19:49:18, vandebo wrote:
> > > On 2013/06/06 03:48:47, Lei Zhang wrote:
> > > > FWIW, this can't actually be reached. If we reach this line, we'll just
> fail
> > a
> > > > CHECK() in the current caller.
> > > 
> > > It can fail in IsolatedContext, so I fixed the callers.
> > 
> > With the current implementation and the parameters we pass in, it can't
fail.
> > 
> > I think there are multiple layers where we all have DCHECKS that the fsid is
> > non-empty, now that you removed the one CHECK.
> 
> Are you saying that we should add a CHECK in IsolatedContext and not have the
> failure case?  That seems fine to me.
> 
>
https://codereview.chromium.org/16158004/diff/24002/chrome/browser/media_gall...
> File chrome/browser/media_galleries/fileapi/itunes_data_provider.cc (right):
> 
>
https://codereview.chromium.org/16158004/diff/24002/chrome/browser/media_gall...
> chrome/browser/media_galleries/fileapi/itunes_data_provider.cc:62: std::string
> name = album_it->location.BaseName().AsUTF8Unsafe();
> On 2013/06/07 03:29:07, Lei Zhang wrote:
> > nit: declare |track| first and put name = track.location...
> 
> Done.
> 
>
https://codereview.chromium.org/16158004/diff/24002/chrome/browser/media_gall...
> chrome/browser/media_galleries/fileapi/itunes_data_provider.cc:207: };
> On 2013/06/07 03:29:07, Lei Zhang wrote:
> > nit: extra ; at the end
> 
> Done.

[Lei Zhang, 2013-06-07 04:34:26]

On 2013/06/07 03:46:52, vandebo wrote:
> Are you saying that we should add a CHECK in IsolatedContext and not have the
> failure case?  That seems fine to me.

I didn't follow the entire callstack to see how the fsid is used elsewhere, and
how well it can handle an empty one. The CHECK that we had gave some assurances
that an empty fsid will be caught early with a crash, but now I worry in
production, if we ever had an empty fsid, it may end up blowing through all the
DCHECKs and result in weird failures.

[vandebo (ex-Chrome), 2013-06-07 04:45:18]

On 2013/06/07 04:34:26, Lei Zhang wrote:
> On 2013/06/07 03:46:52, vandebo wrote:
> > Are you saying that we should add a CHECK in IsolatedContext and not have
the
> > failure case?  That seems fine to me.
> 
> I didn't follow the entire callstack to see how the fsid is used elsewhere,
and
> how well it can handle an empty one. The CHECK that we had gave some
assurances
> that an empty fsid will be caught early with a crash, but now I worry in
> production, if we ever had an empty fsid, it may end up blowing through all
the
> DCHECKs and result in weird failures.

With this change the behavior of an empty fsid is that MediaFileSystemRegistry
will
skip that entry and not return a gallery for it.

[kinuko, 2013-06-07 05:45:13]

content/browser/fileapi/fileapi_message_filter lgtm (sorry for the delay)

https://codereview.chromium.org/16158004/diff/31015/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/16158004/diff/31015/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:765: url.type() ==
fileapi::kFileSystemTypeItunes);
Hmm, this part seems to need some cleanup...

[kinuko, 2013-06-07 05:50:59]

On Fri, Jun 7, 2013 at 2:45 PM, <kinuko@chromium.org> wrote:

> content/browser/fileapi/**fileapi_message_filter lgtm (sorry for the
> delay)
>
>
> https://codereview.chromium.**org/16158004/diff/31015/**
>
content/browser/fileapi/**fileapi_message_filter.cc<https://codereview.chromium.org/16158004/diff/31015/content/browser/fileapi/fileapi_message_filter.cc>
> File content/browser/fileapi/**fileapi_message_filter.cc (right):
>
> https://codereview.chromium.**org/16158004/diff/31015/**
>
content/browser/fileapi/**fileapi_message_filter.cc#**newcode765<https://codereview.chromium.org/16158004/diff/31015/content/browser/fileapi/fileapi_message_filter.cc#newcode765>
> content/browser/fileapi/**fileapi_message_filter.cc:765: url.type() ==
> fileapi::**kFileSystemTypeItunes);
> Hmm, this part seems to need some cleanup...
>

(This comment is not directed to you)


>
https://codereview.chromium.**org/16158004/<https://codereview.chromium.org/1...
>

[commit-bot: I haz the power, 2013-06-07 05:55:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vandebo@chromium.org/16158004/31015

[Lei Zhang, 2013-06-07 05:58:21]

On 2013/06/07 04:45:18, vandebo wrote:
> With this change the behavior of an empty fsid is that MediaFileSystemRegistry
> will
> skip that entry and not return a gallery for it.

Right, that's fine.

[commit-bot: I haz the power, 2013-06-07 14:03:01]

Change committed as 204824

[tommycli, 2013-06-10 17:33:24]

https://codereview.chromium.org/16158004/diff/31015/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.cc (right):

https://codereview.chromium.org/16158004/diff/31015/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.cc:32:
std::vector<std::string> GetComponents(const base::FilePath& path) {
Given that this method also appears in PicasaFileUtil, I think it's important
that we figure out a way to de-dupe them.

Maybe we should put this as a static function in NativeMediaFileUtil?

https://codereview.chromium.org/16158004/diff/31015/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.cc:49:
fileapi::DirectoryEntry MakeDirectoryEntry(
Ditto.

https://codereview.chromium.org/16158004/diff/31015/chrome/browser/media_gall...
File chrome/browser/media_galleries/imported_media_gallery_registry.h (right):

https://codereview.chromium.org/16158004/diff/31015/chrome/browser/media_gall...
chrome/browser/media_galleries/imported_media_gallery_registry.h:64: // The
remaining members are only accessed on the IO thread.
Nice. This is better.

[vandebo (ex-Chrome), 2013-06-11 05:24:37]

https://codereview.chromium.org/16158004/diff/31015/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.cc (right):

https://codereview.chromium.org/16158004/diff/31015/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.cc:32:
std::vector<std::string> GetComponents(const base::FilePath& path) {
On 2013/06/10 17:33:25, tommycli wrote:
> Given that this method also appears in PicasaFileUtil, I think it's important
> that we figure out a way to de-dupe them.
> 
> Maybe we should put this as a static function in NativeMediaFileUtil?

We should just create a file with helper functions.  static
NativeMediaFileUtil::IsMediaFile is also used across classes.