iTunes file util and data provider for media galleries

chrome/browser/media_galleries/fileapi/itunes_data_provider.h

+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_MEDIA_GALLERIES_FILEAPI_ITUNES_DATA_PROVIDER_H_
+#define CHROME_BROWSER_MEDIA_GALLERIES_FILEAPI_ITUNES_DATA_PROVIDER_H_
+
+#include <map>
+#include <set>
+#include <string>
+
+#include "base/basictypes.h"
+#include "base/callback.h"

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

base/callback_forward.h instead?

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

Done.

+#include "base/files/file_path.h"
+#include "chrome/browser/media_galleries/fileapi/itunes_library_parser.h"
+
+namespace itunes {
+
+class ITunesDataProvider {

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

Needs some comment to explain what this does.

[Lei Zhang, 2013/06/06 09:58:54]

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

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

Done.

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

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

+ public:
+  typedef std::string TrackName;
+  typedef std::string AlbumName;
+  typedef std::string ArtistName;
+
+  explicit ITunesDataProvider(const base::FilePath& library_path);
+  virtual ~ITunesDataProvider();

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

no virtual

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

Done.

+
+  // Ask the data provider to refresh the data if necessary. |ready_callback|
+  // will be called when the data is up to date.
+  void RefreshData(const base::Closure& ready_callback);
+
+  // Get the platform path for the library xml file.
+  base::FilePath library_path() const;

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

return const ref.

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

Done.

+
+  // Returns true if |artist| exists in the library.
+  bool KnownArtist(const ArtistName& artist) const;
+
+  // Returns true if |artist| has an album by the name |album| in the library.
+  bool KnownAlbum(const ArtistName& artist, const AlbumName& album) const;
+
+  // Get the track named (filename basename) |track| in |album| by |artist|.
+  bool GetTrackLocation(const ArtistName& artist, const AlbumName& album,

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

Just return an empty FilePath on failure?

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

Done.

+                        const TrackName& track, base::FilePath* result) const;
+
+  // Get the set of artists.
+  std::set<ArtistName> GetArtists() const;

[Lei Zhang, 2013/06/06 09:58:54]

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]

Done.

+
+  // Get the set of albums for |artist|.
+  std::set<AlbumName> GetAlbums(const ArtistName& artist) const;
+
+  // Get the tracks for the |album| by |artist|.
+  std::map<TrackName, base::FilePath> GetTracks(const ArtistName& artist,
+                                                const AlbumName& album) const;
+
+ private:
+  typedef std::map<TrackName, base::FilePath> Album;
+  typedef std::map<AlbumName, Album> Artist;
+  typedef std::map<ArtistName, Artist> Library;
+
+  static Album MakeUniqueTrackNames(const ITunesLibraryParser::Album& album);

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

Put this in the anonymous namespace instead?

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

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

+
+  void ParseLibrary();
+
+  base::FilePath library_path_;

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

const

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

Done.

+
+  Library library_;
+
+  bool needs_refresh_;
+
+  DISALLOW_COPY_AND_ASSIGN(ITunesDataProvider);
+};
+
+}  // namespace itunes
+
+#endif  // CHROME_BROWSER_MEDIA_GALLERIES_FILEAPI_ITUNES_DATA_PROVIDER_H_