[MediaGalleries] iPhoto: Add original file field to the parser. Dedupe album names.

[MediaGalleries] iPhoto: Add original file field to the parser. Dedupe album names.

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

[Greg Billock, 2013-10-31 14:51:11]

ready for a look

[vandebo (ex-Chrome), 2013-10-31 20:53:43]

https://codereview.chromium.org/52093003/diff/40001/chrome/utility/media_gall...
File chrome/utility/media_galleries/iphoto_library_parser.cc (right):

https://codereview.chromium.org/52093003/diff/40001/chrome/utility/media_gall...
chrome/utility/media_galleries/iphoto_library_parser.cc:20: struct PhotoInfo {
I think we should remove everything we don't use from this.  I think that's
guid, time, and type.

https://codereview.chromium.org/52093003/diff/40001/chrome/utility/media_gall...
chrome/utility/media_galleries/iphoto_library_parser.cc:301:
library_.albums[name] = album;
So first in the xml file wins?  Maybe we should try to do better.

https://codereview.chromium.org/52093003/diff/40001/chrome/utility/media_gall...
chrome/utility/media_galleries/iphoto_library_parser.cc:303:
library_.albums[name+"("+base::Uint64ToString(album_info.id)+")"] =
Date might be a more useful uniquifier, is there a field for album date?

[Greg Billock, 2013-10-31 22:34:23]

https://codereview.chromium.org/52093003/diff/40001/chrome/utility/media_gall...
File chrome/utility/media_galleries/iphoto_library_parser.cc (right):

https://codereview.chromium.org/52093003/diff/40001/chrome/utility/media_gall...
chrome/utility/media_galleries/iphoto_library_parser.cc:20: struct PhotoInfo {
On 2013/10/31 20:53:43, vandebo wrote:
> I think we should remove everything we don't use from this.  I think that's
> guid, time, and type.

ok

https://codereview.chromium.org/52093003/diff/40001/chrome/utility/media_gall...
chrome/utility/media_galleries/iphoto_library_parser.cc:301:
library_.albums[name] = album;
On 2013/10/31 20:53:43, vandebo wrote:
> So first in the xml file wins?  Maybe we should try to do better.

Sounds good.

https://codereview.chromium.org/52093003/diff/40001/chrome/utility/media_gall...
chrome/utility/media_galleries/iphoto_library_parser.cc:303:
library_.albums[name+"("+base::Uint64ToString(album_info.id)+")"] =
On 2013/10/31 20:53:43, vandebo wrote:
> Date might be a more useful uniquifier, is there a field for album date?

No, there's no real good signal. There's a GUID, a SongPath, which I'm guessing
is often just the default and not much use, and then the list of IDs. I thought
about counting the number of photos in the album, but then we'd need a dedupe
for our dedupe. Let's just go with this and if we think of something better
it'll be easy to change.

[vandebo (ex-Chrome), 2013-11-01 18:50:33]

LGTM with suggestions.

https://codereview.chromium.org/52093003/diff/40001/chrome/utility/media_gall...
File chrome/utility/media_galleries/iphoto_library_parser.cc (right):

https://codereview.chromium.org/52093003/diff/40001/chrome/utility/media_gall...
chrome/utility/media_galleries/iphoto_library_parser.cc:301:
library_.albums[name] = album;
On 2013/10/31 22:34:24, Greg Billock wrote:
> On 2013/10/31 20:53:43, vandebo wrote:
> > So first in the xml file wins?  Maybe we should try to do better.
> 
> Sounds good.

Should we keep a separate list of dup album names basenames and add the album id
to all of them?  We could just use 1-n in that case.

https://codereview.chromium.org/52093003/diff/160001/chrome/utility/media_gal...
File chrome/utility/media_galleries/iphoto_library_parser.cc (left):

https://codereview.chromium.org/52093003/diff/160001/chrome/utility/media_gal...
chrome/utility/media_galleries/iphoto_library_parser.cc:50: !(found_guid &&
found_location && found_type)) {
If you like, add this logic back in - if both location and original location
have been found, we're done.

[commit-bot: I haz the power, 2013-11-01 22:14:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/52093003/160001

[commit-bot: I haz the power, 2013-11-02 02:31:04]

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

[commit-bot: I haz the power, 2013-11-02 05:06:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/52093003/160001

[commit-bot: I haz the power, 2013-11-02 05:06:03]

Failed to apply patch for
chrome/utility/media_galleries/iphoto_library_parser_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/utility/media_galleries/iphoto_library_parser_unittest.cc
  Hunk #1 FAILED at 42.
  Hunk #2 succeeded at 99 (offset 32 lines).
  Hunk #3 succeeded at 153 (offset 32 lines).
  Hunk #4 succeeded at 290 (offset 87 lines).
  1 out of 4 hunks FAILED -- saving rejects to file
chrome/utility/media_galleries/iphoto_library_parser_unittest.cc.rej

Patch:       chrome/utility/media_galleries/iphoto_library_parser_unittest.cc
Index: chrome/utility/media_galleries/iphoto_library_parser_unittest.cc
diff --git a/chrome/utility/media_galleries/iphoto_library_parser_unittest.cc
b/chrome/utility/media_galleries/iphoto_library_parser_unittest.cc
index
68ad6b8f79ab66ab2375ccc51e778a1c981c7b1e..afefee6a81b66065bd27291f00abdfaaa8e9d5ee
100644
--- a/chrome/utility/media_galleries/iphoto_library_parser_unittest.cc
+++ b/chrome/utility/media_galleries/iphoto_library_parser_unittest.cc
@@ -42,24 +42,33 @@
     "   </dict>"
 
 #define SIMPLE_PHOTO(id, guid, path, caption) \
-    "  <key>" #id "</key>"             \
-    "  <dict>"                         \
-    "    <key>MediaType</key>"         \
-    "    <string>Image</string>"       \
-    "    <key>Caption</key>"           \
-    "    <string>" caption "</string>" \
-    "    <key>GUID</key>"              \
-    "    <string>" #guid "</string>"   \
-    "    <key>ImagePath</key>"         \
-    "    <string>" path "</string>"    \
-    "    <key>ThumbPath</key>"         \
-    "    <string>" path "</string>"    \
+    "  <key>" #id "</key>"                    \
+    "  <dict>"                                \
+    "    <key>MediaType</key>"                \
+    "    <string>Image</string>"              \
+    "    <key>Caption</key>"                  \
+    "    <string>" caption "</string>"        \
+    "    <key>GUID</key>"                     \
+    "    <string>" #guid "</string>"          \
+    "    <key>ModDateAsTimerInterval</key>"   \
+    "    <string>386221543.0000</string>"     \
+    "    <key>DateAsTimerInterval</key>"      \
+    "    <string>386221543.0000</string>"     \
+    "    <key>DateAsTimerIntervalGMT</key>"   \
+    "    <string>385123456.00</string>"       \
+    "    <key>ImagePath</key>"                \
+    "    <string>" path "</string>"           \
+    "    <key>OriginalPath</key>"             \
+    "    <string>/original" path "</string>"  \
+    "    <key>ThumbPath</key>"                \
+    "    <string>" path "</string>"           \
     "  </dict>"
 
 #define SIMPLE_FOOTER()  \
     "  </dict>"          \
     "</plist>"
 
+
 namespace iphoto {
 
 namespace {
@@ -67,6 +76,7 @@ namespace {
 void ComparePhoto(const parser::Photo& a, const parser::Photo& b) {
   EXPECT_EQ(a.id, b.id);
   EXPECT_EQ(a.location.value(), b.location.value());
+  EXPECT_EQ(a.original_location.value(), b.original_location.value());
 }
 
 void CompareAlbum(const parser::Album& a, const parser::Album& b) {
@@ -120,9 +130,11 @@ class IPhotoLibraryParserTest : public testing::Test {
     CompareLibrary(expected_library_, parser.library());
   }
 
-  void AddExpectedPhoto(uint32 id, const std::string& location,
+  void AddExpectedPhoto(uint32 id,
+                        const std::string& location,
                         const std::string& album) {
-    parser::Photo photo(id, base::FilePath::FromUTF8Unsafe(location));
+    parser::Photo photo(id, base::FilePath::FromUTF8Unsafe(location),
+                        base::FilePath::FromUTF8Unsafe("/original" +
location));
     if (!album.empty())
       expected_library_.albums[album].insert(id);
     expected_library_.all_photos.insert(photo);
@@ -200,6 +212,27 @@ TEST_F(IPhotoLibraryParserTest, Albums) {
       SIMPLE_FOOTER());
 }
 
+TEST_F(IPhotoLibraryParserTest, DuplicateAlbumNames) {
+  AddExpectedPhoto(1, "/dir/PhotoA1.jpg", "Album 1");
+  AddExpectedPhoto(2, "/dir/PhotoA2.jpg", "Album 1");
+  AddExpectedPhoto(3, "/dir/PhotoA3.jpg", "Album 1(11)");
+  AddExpectedPhoto(4, "/dir/PhotoB1.jpg", "Album 1(11)");
+  TestParser(
+      true,
+      SIMPLE_HEADER()
+      ALBUMS_HEADER()
+      SIMPLE_ALBUM(10, "Album 1", 1, 2)
+      SIMPLE_ALBUM(11, "Album 1", 3, 4)
+      ALBUMS_FOOTER()
+      IMAGE_LIST_HEADER()
+      SIMPLE_PHOTO(1, 1, "/dir/PhotoA1.jpg", "Photo 1")
+      SIMPLE_PHOTO(2, 2, "/dir/PhotoA2.jpg", "Photo 2")
+      SIMPLE_PHOTO(3, 3, "/dir/PhotoA3.jpg", "Photo 3")
+      SIMPLE_PHOTO(4, 4, "/dir/PhotoB1.jpg", "Photo 4")
+      IMAGE_LIST_FOOTER()
+      SIMPLE_FOOTER());
+}
+
 }  // namespace
 
 }  // namespace iphoto

[commit-bot: I haz the power, 2013-11-04 17:43:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/52093003/410001

[commit-bot: I haz the power, 2013-11-04 21:14:43]

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

[commit-bot: I haz the power, 2013-11-06 18:20:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/52093003/410001

[commit-bot: I haz the power, 2013-11-06 20:19:59]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2013-11-06 20:31:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/52093003/410001

[commit-bot: I haz the power, 2013-11-06 21:12:11]

Change committed as 233368