Add a test for ITunesDataProvider (try two).

Add a test for ITunesDataProvider (try two).

It's a browser test instead of a unit test because the utility process gets invoked.
Patch set one is what was reviewed and committed in https://codereview.chromium.org/17616006/

BUG=NONE

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

[Greg Billock, 2013-07-03 22:41:19]

lgtm

[vandebo (ex-Chrome), 2013-07-03 22:42:14]

lei: please stamp - the gypi part did not change.

[Lei Zhang, 2013-07-03 23:01:39]

lgtm

[commit-bot: I haz the power, 2013-07-03 23:05:48]

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

[commit-bot: I haz the power, 2013-07-04 00:14:29]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-07-04 00:18:39]

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

[commit-bot: I haz the power, 2013-07-04 01:10:59]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-07-08 14:53:54]

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

[commit-bot: I haz the power, 2013-07-08 15:53:49]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[Lei Zhang, 2013-07-16 22:03:24]

https://codereview.chromium.org/18348019/diff/25001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc
(right):

https://codereview.chromium.org/18348019/diff/25001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:109:
"  <key>Album Artist</key><string>%s</string>\n"
Don't forget to update this as a result of the Artist vs Album Artist
discussion.

[vandebo (ex-Chrome), 2013-07-16 23:07:03]

Lei: I finally got this test working.  There are non-trivial changes.  Can you
give it another look?

https://codereview.chromium.org/18348019/diff/25001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc
(right):

https://codereview.chromium.org/18348019/diff/25001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:109:
"  <key>Album Artist</key><string>%s</string>\n"
On 2013/07/16 22:03:25, Lei Zhang wrote:
> Don't forget to update this as a result of the Artist vs Album Artist
> discussion.

Done.

[Lei Zhang, 2013-07-17 02:21:35]

https://codereview.chromium.org/18348019/diff/46001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/itunes_data_provider.h (right):

https://codereview.chromium.org/18348019/diff/46001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes_data_provider.h:66: friend class
itunes::TestITunesDataProvider;
nit: no need for itunes::

https://codereview.chromium.org/18348019/diff/46001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc
(right):

https://codereview.chromium.org/18348019/diff/46001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:66:
public:
None of this actually needs to be public.

https://codereview.chromium.org/18348019/diff/46001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:81:
void RunTestOnMediaTaskRunner() {
How about just RunTest(). RunTestOnMediaTaskRunner makes it sound like it's
suppose to be running on the MTR.

https://codereview.chromium.org/18348019/diff/46001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:107:
base::FilePath library_dir() const {
This can return a const base::FilePath&

https://codereview.chromium.org/18348019/diff/46001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:126:
void TestDone() {
make sure you are on the MTR.

https://codereview.chromium.org/18348019/diff/46001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:135:
void StartTestOnMediaTaskRunner() {
Check you are on the MTR.

https://codereview.chromium.org/18348019/diff/46001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:147:
void OnLibraryChanged(void) {
nit: We usually don't write (void). Same on line 290.

https://codereview.chromium.org/18348019/diff/46001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:149:
MediaFileSystemBackend::CurrentlyOnMediaTaskRunnerThread());
nit: fits on previous line.

https://codereview.chromium.org/18348019/diff/46001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:162:
// Visual studio doesn't like %zd, so cast to int instead.
Look for PRIuS in base/format_macros.h.

https://codereview.chromium.org/18348019/diff/46001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:176:
file_util::WriteFile(XmlFile(), xml.c_str(), xml.size());
Make sure we wrote the right # of bytes. Ditto for line 347.

https://codereview.chromium.org/18348019/diff/46001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:229:
EXPECT_EQ(1U, artists.size());
This should be an ASSERT_EQ(), otherwise the next line might have a NULL deref.
Ditto below.

https://codereview.chromium.org/18348019/diff/46001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:230:
EXPECT_EQ(std::string("Artist"), *artists.begin());
do you need the explicit std::string()? Ditto below.

https://codereview.chromium.org/18348019/diff/46001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:309:
void ExpectTrackLocation(const std::string& artist, const std::string& album,
If you put this and ExpectNoTrack() in ITunesDataProviderTest, you can use it in
other tests as well.

[vandebo (ex-Chrome), 2013-07-17 04:22:17]

https://codereview.chromium.org/18348019/diff/46001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/itunes_data_provider.h (right):

https://codereview.chromium.org/18348019/diff/46001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes_data_provider.h:66: friend class
itunes::TestITunesDataProvider;
On 2013/07/17 02:21:35, Lei Zhang wrote:
> nit: no need for itunes::

Done.

https://codereview.chromium.org/18348019/diff/46001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc
(right):

https://codereview.chromium.org/18348019/diff/46001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:66:
public:
On 2013/07/17 02:21:35, Lei Zhang wrote:
> None of this actually needs to be public.

Done.

https://codereview.chromium.org/18348019/diff/46001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:81:
void RunTestOnMediaTaskRunner() {
On 2013/07/17 02:21:35, Lei Zhang wrote:
> How about just RunTest(). RunTestOnMediaTaskRunner makes it sound like it's
> suppose to be running on the MTR.

Done.

https://codereview.chromium.org/18348019/diff/46001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:107:
base::FilePath library_dir() const {
On 2013/07/17 02:21:35, Lei Zhang wrote:
> This can return a const base::FilePath&

Done.

https://codereview.chromium.org/18348019/diff/46001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:126:
void TestDone() {
On 2013/07/17 02:21:35, Lei Zhang wrote:
> make sure you are on the MTR.

Done.

https://codereview.chromium.org/18348019/diff/46001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:135:
void StartTestOnMediaTaskRunner() {
On 2013/07/17 02:21:35, Lei Zhang wrote:
> Check you are on the MTR.

Done.

https://codereview.chromium.org/18348019/diff/46001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:147:
void OnLibraryChanged(void) {
On 2013/07/17 02:21:35, Lei Zhang wrote:
> nit: We usually don't write (void). Same on line 290.

Done.

https://codereview.chromium.org/18348019/diff/46001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:149:
MediaFileSystemBackend::CurrentlyOnMediaTaskRunnerThread());
On 2013/07/17 02:21:35, Lei Zhang wrote:
> nit: fits on previous line.

Done.

https://codereview.chromium.org/18348019/diff/46001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:162:
// Visual studio doesn't like %zd, so cast to int instead.
On 2013/07/17 02:21:35, Lei Zhang wrote:
> Look for PRIuS in base/format_macros.h.

Done.

https://codereview.chromium.org/18348019/diff/46001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:176:
file_util::WriteFile(XmlFile(), xml.c_str(), xml.size());
On 2013/07/17 02:21:35, Lei Zhang wrote:
> Make sure we wrote the right # of bytes. Ditto for line 347.

Done.

https://codereview.chromium.org/18348019/diff/46001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:229:
EXPECT_EQ(1U, artists.size());
On 2013/07/17 02:21:35, Lei Zhang wrote:
> This should be an ASSERT_EQ(), otherwise the next line might have a NULL
deref.
> Ditto below.

Done.

https://codereview.chromium.org/18348019/diff/46001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:230:
EXPECT_EQ(std::string("Artist"), *artists.begin());
On 2013/07/17 02:21:35, Lei Zhang wrote:
> do you need the explicit std::string()? Ditto below.

Done.

https://codereview.chromium.org/18348019/diff/46001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:309:
void ExpectTrackLocation(const std::string& artist, const std::string& album,
On 2013/07/17 02:21:35, Lei Zhang wrote:
> If you put this and ExpectNoTrack() in ITunesDataProviderTest, you can use it
in
> other tests as well.

Done.

[Lei Zhang, 2013-07-17 19:59:40]

lgtm

[commit-bot: I haz the power, 2013-07-18 15:48:18]

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

[commit-bot: I haz the power, 2013-07-18 19:02:00]

Change committed as 212384