Extract track information from iTunes library xml file.

Extract track information from iTunes library xml file.

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

[wrong vandebo, 2013-05-30 23:14:44]

I'm going to add some more tests, but if you can give this a once over, we can
move along a big faster.

I'm going to try out a macro to generate a basic track XML to make the tests a
bit shorter.

[Lei Zhang, 2013-05-31 04:34:35]

https://codereview.chromium.org/16231016/diff/1/chrome/browser/media_gallerie...
File chrome/browser/media_galleries/fileapi/itunes_library_parser.cc (right):

https://codereview.chromium.org/16231016/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/itunes_library_parser.cc:99: // true if
it was all found, false otherwise.  In either case, the curser is
typo - cursor

https://codereview.chromium.org/16231016/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/itunes_library_parser.cc:102:
DCHECK(result != NULL);
nit: DCHECK(result);

https://codereview.chromium.org/16231016/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/itunes_library_parser.cc:103:
DCHECK_EQ("dict", reader->NodeName());
Can there be a malformed XML file that fails this DCHECK?

https://codereview.chromium.org/16231016/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/itunes_library_parser.cc:105: int
dict_content_depth = reader->Depth() + 1;
Can someone create a big, malicious XML file that causes |dict_content_depth| to
overflow?

https://codereview.chromium.org/16231016/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/itunes_library_parser.cc:132: break;
Are you breaking out of the while loop because it is an error to have 2
locations? Same comment applies to elsewhere in this loop.

https://codereview.chromium.org/16231016/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/itunes_library_parser.cc:170: std::string
value;
Explain why this value is read into and then ignored. Is it just to cause the
reader to skip the element?

https://codereview.chromium.org/16231016/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/itunes_library_parser.cc:233: no_errors =
false;
why not just return track_found at this point?

https://codereview.chromium.org/16231016/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/itunes_library_parser.cc:242:
base::StringToUint(key, &id);
Check return result.

https://codereview.chromium.org/16231016/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/itunes_library_parser.cc:251:
library_[track_info.artist][track_info.album].insert(track);
Do you care if there already exists a track with the same id?

https://codereview.chromium.org/16231016/diff/1/chrome/browser/media_gallerie...
File chrome/browser/media_galleries/fileapi/itunes_library_parser.h (right):

https://codereview.chromium.org/16231016/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/itunes_library_parser.h:18:
Track(uint32_t id, const base::FilePath& location);
You may need uint32 instead to be x-platform? Try building it on Windows?

https://codereview.chromium.org/16231016/diff/1/chrome/browser/media_gallerie...
File chrome/browser/media_galleries/fileapi/itunes_library_parser_unittest.cc
(right):

https://codereview.chromium.org/16231016/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/itunes_library_parser_unittest.cc:82:
ITunesLibraryParser::Library expected_tracks_;
expected_library_ ?

[vandebo (ex-Chrome), 2013-05-31 21:41:12]

Added some more tests, happy to add any others you think might be useful.

https://codereview.chromium.org/16231016/diff/1/chrome/browser/media_gallerie...
File chrome/browser/media_galleries/fileapi/itunes_library_parser.cc (right):

https://codereview.chromium.org/16231016/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/itunes_library_parser.cc:99: // true if
it was all found, false otherwise.  In either case, the curser is
On 2013/05/31 04:34:35, Lei Zhang wrote:
> typo - cursor

Done.

https://codereview.chromium.org/16231016/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/itunes_library_parser.cc:102:
DCHECK(result != NULL);
On 2013/05/31 04:34:35, Lei Zhang wrote:
> nit: DCHECK(result);

Done.

https://codereview.chromium.org/16231016/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/itunes_library_parser.cc:103:
DCHECK_EQ("dict", reader->NodeName());
On 2013/05/31 04:34:35, Lei Zhang wrote:
> Can there be a malformed XML file that fails this DCHECK?

Done.

https://codereview.chromium.org/16231016/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/itunes_library_parser.cc:105: int
dict_content_depth = reader->Depth() + 1;
On 2013/05/31 04:34:35, Lei Zhang wrote:
> Can someone create a big, malicious XML file that causes |dict_content_depth|
to
> overflow?

Probably, but to what end?  If they control the XML, they control what entries
we get.  I don't see anything bad that would happen for any given value, aside
from messing up the parsing.

https://codereview.chromium.org/16231016/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/itunes_library_parser.cc:132: break;
On 2013/05/31 04:34:35, Lei Zhang wrote:
> Are you breaking out of the while loop because it is an error to have 2
> locations? Same comment applies to elsewhere in this loop.

Yes. We expect one entry for each these fields. If there are multiple then it's
a record that we don't understand and we should just skip it.

https://codereview.chromium.org/16231016/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/itunes_library_parser.cc:170: std::string
value;
On 2013/05/31 04:34:35, Lei Zhang wrote:
> Explain why this value is read into and then ignored. Is it just to cause the
> reader to skip the element?

Done.

https://codereview.chromium.org/16231016/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/itunes_library_parser.cc:233: no_errors =
false;
On 2013/05/31 04:34:35, Lei Zhang wrote:
> why not just return track_found at this point?

Done.

https://codereview.chromium.org/16231016/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/itunes_library_parser.cc:242:
base::StringToUint(key, &id);
On 2013/05/31 04:34:35, Lei Zhang wrote:
> Check return result.

Done.

https://codereview.chromium.org/16231016/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/itunes_library_parser.cc:251:
library_[track_info.artist][track_info.album].insert(track);
On 2013/05/31 04:34:35, Lei Zhang wrote:
> Do you care if there already exists a track with the same id?

ids should be unique.  If that's not the case, I'm ok letting the last one win
until someone complains.

https://codereview.chromium.org/16231016/diff/1/chrome/browser/media_gallerie...
File chrome/browser/media_galleries/fileapi/itunes_library_parser.h (right):

https://codereview.chromium.org/16231016/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/itunes_library_parser.h:18:
Track(uint32_t id, const base::FilePath& location);
On 2013/05/31 04:34:35, Lei Zhang wrote:
> You may need uint32 instead to be x-platform? Try building it on Windows?

Fixed.  Changed to uint64 (plist integers can be 64bit)

https://codereview.chromium.org/16231016/diff/1/chrome/browser/media_gallerie...
File chrome/browser/media_galleries/fileapi/itunes_library_parser_unittest.cc
(right):

https://codereview.chromium.org/16231016/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/itunes_library_parser_unittest.cc:82:
ITunesLibraryParser::Library expected_tracks_;
On 2013/05/31 04:34:35, Lei Zhang wrote:
> expected_library_ ?

Done.

[Lei Zhang, 2013-05-31 22:06:53]

lgtm, though I didn't check the libxml usage.

https://codereview.chromium.org/16231016/diff/1/chrome/browser/media_gallerie...
File chrome/browser/media_galleries/fileapi/itunes_library_parser.cc (right):

https://codereview.chromium.org/16231016/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/itunes_library_parser.cc:105: int
dict_content_depth = reader->Depth() + 1;
On 2013/05/31 21:41:12, vandebo wrote:
> On 2013/05/31 04:34:35, Lei Zhang wrote:
> > Can someone create a big, malicious XML file that causes
|dict_content_depth|
> to
> > overflow?
> 
> Probably, but to what end?  If they control the XML, they control what entries
> we get.  I don't see anything bad that would happen for any given value, aside
> from messing up the parsing.

With the way it's used, I just want to make sure we don't end up in an infinite
loop.

[vandebo (ex-Chrome), 2013-05-31 22:21:12]

On 2013/05/31 22:06:53, Lei Zhang wrote:
> lgtm, though I didn't check the libxml usage.

Check in what sense? Would you like me to get another review?

https://codereview.chromium.org/16231016/diff/1/chrome/browser/media_gallerie...
File chrome/browser/media_galleries/fileapi/itunes_library_parser.cc (right):

https://codereview.chromium.org/16231016/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/fileapi/itunes_library_parser.cc:105: int
dict_content_depth = reader->Depth() + 1;
On 2013/05/31 22:06:53, Lei Zhang wrote:
> On 2013/05/31 21:41:12, vandebo wrote:
> > On 2013/05/31 04:34:35, Lei Zhang wrote:
> > > Can someone create a big, malicious XML file that causes
> |dict_content_depth|
> > to
> > > overflow?
> > 
> > Probably, but to what end?  If they control the XML, they control what
entries
> > we get.  I don't see anything bad that would happen for any given value,
aside
> > from messing up the parsing.
> 
> With the way it's used, I just want to make sure we don't end up in an
infinite
> loop.

In infinite loop isn't possible because we'll always eventually hit the end of
the file.

[Lei Zhang, 2013-05-31 22:24:16]

On 2013/05/31 22:21:12, vandebo wrote:
> On 2013/05/31 22:06:53, Lei Zhang wrote:
> > lgtm, though I didn't check the libxml usage.
> 
> Check in what sense? Would you like me to get another review?

Yes, like the previous CL involving xml parsing.

[vandebo (ex-Chrome), 2013-05-31 22:28:52]

Bryan, could you take a look?

[bryeung, 2013-06-03 14:30:27]

nice unittest: it's very clearly written.

A couple of comments RE: code duplication mostly.

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

https://codereview.chromium.org/16231016/diff/6001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_library_parser.cc:31: bool
SeekToNodeAtCurrentDepth(XmlReader* reader, const std::string& name) {
this looks like a direct copy of the code in
chrome/browser/media_galleries/fileapi/itunes_finder_win.cc: can you please move
this to somewhere common?

https://codereview.chromium.org/16231016/diff/6001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_library_parser.cc:48: // Search
within the dict for |key|.
ditto: this needs to go somewhere common

https://codereview.chromium.org/16231016/diff/6001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_library_parser.cc:74: if
(!reader->SkipToElement()) {
seems like this little block could be it's own method as well, with a good name
to make things a little more clear

https://codereview.chromium.org/16231016/diff/6001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_library_parser.cc:165: if
(!reader->SkipToElement()) {
ah, yes, another use.  Now I'm sure this needs to be it's own method :-)

https://codereview.chromium.org/16231016/diff/6001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_library_parser.cc:173: if
(!reader->ReadElementContent(&value))
why bother reading it?  can't we just skip past it?

https://codereview.chromium.org/16231016/diff/6001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_library_parser.cc:179: while
(reader->Depth() >= dict_content_depth) {
nit: no braces

https://codereview.chromium.org/16231016/diff/6001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_library_parser.cc:229: // Once
parsing has gotten this far, return what ever is found, even if
nit: whatever

[vandebo (ex-Chrome), 2013-06-03 17:28:48]

Thanks

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

https://codereview.chromium.org/16231016/diff/6001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_library_parser.cc:31: bool
SeekToNodeAtCurrentDepth(XmlReader* reader, const std::string& name) {
On 2013/06/03 14:30:27, bryeung wrote:
> this looks like a direct copy of the code in
> chrome/browser/media_galleries/fileapi/itunes_finder_win.cc: can you please
move
> this to somewhere common?

Done.

https://codereview.chromium.org/16231016/diff/6001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_library_parser.cc:48: // Search
within the dict for |key|.
On 2013/06/03 14:30:27, bryeung wrote:
> ditto: this needs to go somewhere common

Done.

https://codereview.chromium.org/16231016/diff/6001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_library_parser.cc:74: if
(!reader->SkipToElement()) {
On 2013/06/03 14:30:27, bryeung wrote:
> seems like this little block could be it's own method as well, with a good
name
> to make things a little more clear

Done.

https://codereview.chromium.org/16231016/diff/6001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_library_parser.cc:173: if
(!reader->ReadElementContent(&value))
On 2013/06/03 14:30:27, bryeung wrote:
> why bother reading it?  can't we just skip past it?

Done.

https://codereview.chromium.org/16231016/diff/6001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_library_parser.cc:179: while
(reader->Depth() >= dict_content_depth) {
On 2013/06/03 14:30:27, bryeung wrote:
> nit: no braces

Done.

https://codereview.chromium.org/16231016/diff/6001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/itunes_library_parser.cc:229: // Once
parsing has gotten this far, return what ever is found, even if
On 2013/06/03 14:30:27, bryeung wrote:
> nit: whatever

Done.

[bryeung, 2013-06-03 17:34:16]

lgtm

https://codereview.chromium.org/16231016/diff/17001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/itunes_xml_utils.h (right):

https://codereview.chromium.org/16231016/diff/17001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes_xml_utils.h:22: // Search within
the dict for |key|.
A comment about pre-conditions would be nice here (i.e. that reader must be on a
"dict" tag).  And maybe what a "dict" means in the iTunes XML context.

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

https://codereview.chromium.org/16231016/diff/17001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/itunes_xml_utils.h (right):

https://codereview.chromium.org/16231016/diff/17001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes_xml_utils.h:22: // Search within
the dict for |key|.
On 2013/06/03 17:34:16, bryeung wrote:
> A comment about pre-conditions would be nice here (i.e. that reader must be on
a
> "dict" tag).  And maybe what a "dict" means in the iTunes XML context.

Done.

It's a plist XML, so I didn't really describe the format. Any real instance of
these XML files contains the DTD: http://www.apple.com/DTDs/PropertyList-1.0.dtd

[commit-bot: I haz the power, 2013-06-03 17:47:10]

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

[bryeung, 2013-06-03 17:49:22]

https://codereview.chromium.org/16231016/diff/17001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/itunes_xml_utils.h (right):

https://codereview.chromium.org/16231016/diff/17001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes_xml_utils.h:22: // Search within
the dict for |key|.
On 2013/06/03 17:46:52, vandebo wrote:
> On 2013/06/03 17:34:16, bryeung wrote:
> > A comment about pre-conditions would be nice here (i.e. that reader must be
on
> a
> > "dict" tag).  And maybe what a "dict" means in the iTunes XML context.
> 
> Done.
> 
> It's a plist XML, so I didn't really describe the format. Any real instance of
> these XML files contains the DTD:
http://www.apple.com/DTDs/PropertyList-1.0.dtd

sgtm, thanks

[commit-bot: I haz the power, 2013-06-03 18:11:28]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
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-06-03 18:31:12]

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

[commit-bot: I haz the power, 2013-06-03 18:52:19]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
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-06-03 18:59:03]

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

[commit-bot: I haz the power, 2013-06-03 20:02:46]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win7_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
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-06-03 20:07:40]

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

[commit-bot: I haz the power, 2013-06-03 21:34:27]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
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-06-03 21:41:24]

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

[commit-bot: I haz the power, 2013-06-03 22:49:22]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-06-04 05:41:23]

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

[commit-bot: I haz the power, 2013-06-04 13:51:05]

Change committed as 203958