Add a test for ITunesDataProvider.

Add a test for ITunesDataProvider.

It's a browser test instead of a unit test because the utility process gets invoked.

BUG=NONE

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

[Greg Billock, 2013-07-02 20:29:56]

lgtm. a few comments, but seems fine.

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

https://codereview.chromium.org/17616006/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:77:
int id = static_cast<int>(i) + 1;
ids of zero bad? How about making i=1; i <= entries.size() in the loop?

https://codereview.chromium.org/17616006/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:92:
library_dir_.path().AppendASCII("library.xml");
use XmlFile() here.

https://codereview.chromium.org/17616006/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:96:
void RefreshData(const ITunesDataProvider::ReadyCallback& callback) {
Could this be made synchronous with a waitable event or internal RunLoop or
something? That would let each of the tests just execute straight through.

https://codereview.chromium.org/17616006/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:147:
EXPECT_TRUE(data_provider()->KnownArtist("Artist"));
I don't see why these APIs exist. Shouldn't we just do GetArtists, GetAlbums,
GetAlbum?

https://codereview.chromium.org/17616006/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:268:
artist, album, track_name).NormalizePathSeparators().value());
Add << track_name or something to help debug if this goes bad. and below.

https://codereview.chromium.org/17616006/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:302:
EXPECT_EQ(track.value(),
Maybe add a comment about how dupe names are built up and presented to the
client?

[vandebo (ex-Chrome), 2013-07-02 21:00:31]

+thestig for gypi changes.

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

https://codereview.chromium.org/17616006/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:77:
int id = static_cast<int>(i) + 1;
On 2013/07/02 20:29:57, Greg Billock wrote:
> ids of zero bad? How about making i=1; i <= entries.size() in the loop?

Not bad, just edgecase-y. I can't change the loop because the vector is zero
indexed.  I still need to cast it to int so it wouldn't save any lines even if I
could.

https://codereview.chromium.org/17616006/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:92:
library_dir_.path().AppendASCII("library.xml");
On 2013/07/02 20:29:57, Greg Billock wrote:
> use XmlFile() here.

Oops, done.

https://codereview.chromium.org/17616006/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:96:
void RefreshData(const ITunesDataProvider::ReadyCallback& callback) {
On 2013/07/02 20:29:57, Greg Billock wrote:
> Could this be made synchronous with a waitable event or internal RunLoop or
> something? That would let each of the tests just execute straight through.

I started with a RunLoop, but that doesn't work on a SequencedTaskRunner (i.e.
the media task runner thread).  It can't use a waitable event because the
message loop wouldn't get pumped.

https://codereview.chromium.org/17616006/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:147:
EXPECT_TRUE(data_provider()->KnownArtist("Artist"));
On 2013/07/02 20:29:57, Greg Billock wrote:
> I don't see why these APIs exist. Shouldn't we just do GetArtists, GetAlbums,
> GetAlbum?

If you look at where ITunesDataProvider is used, i.e. ITunesFileUtil, there are
places where we need to ensure that a particular artist or album is valid
without knowing the complete list.  These methods make that more efficient.

https://codereview.chromium.org/17616006/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:268:
artist, album, track_name).NormalizePathSeparators().value());
On 2013/07/02 20:29:57, Greg Billock wrote:
> Add << track_name or something to help debug if this goes bad. and below.

If the comparison fails, it will print .value() of both args, so I don't see any
value in adding additional out here.

Updated ExpectNoTrack()

https://codereview.chromium.org/17616006/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:302:
EXPECT_EQ(track.value(),
On 2013/07/02 20:29:57, Greg Billock wrote:
> Maybe add a comment about how dupe names are built up and presented to the
> client?

Done.

https://codereview.chromium.org/17616006/diff/13001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/17616006/diff/13001/chrome/chrome_tests.gypi#...
chrome/chrome_tests.gypi:2038:
'browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc',
Moved this to a mac or win only section.

[Lei Zhang, 2013-07-02 21:25:50]

gypi file lgtm

[commit-bot: I haz the power, 2013-07-02 21:26:34]

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

[vandebo (ex-Chrome), 2013-07-02 22:27:26]

Compile fix

[commit-bot: I haz the power, 2013-07-02 22:27:50]

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

[commit-bot: I haz the power, 2013-07-03 03:44:41]

Change committed as 209868