Chromium Code Reviews
Help | Chromium Project | Sign in
(10)

Issue 16158004: iTunes file util and data provider for media galleries (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years ago by vandebo (ex-Chrome)
Modified:
2 years ago
CC:
chromium-reviews, tzik+watch_chromium.org, kinuko+watch, tommycli
Visibility:
Public.

Description

iTunes file util and data provider for media galleries BUG=234837 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=204824

Patch Set 1 #

Patch Set 2 : Checkpoint - compiles #

Patch Set 3 : Basic bug fixes #

Total comments: 4

Patch Set 4 : nit #

Total comments: 64

Patch Set 5 : comments #

Patch Set 6 : A small bug fixes #

Total comments: 4

Patch Set 7 : nits #

Patch Set 8 : Undo incorrect fix #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+964 lines, -327 lines) Patch
M chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.h View 1 2 3 4 2 chunks +31 lines, -1 line 0 comments Download
M chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.cc View 1 2 3 4 3 chunks +229 lines, -9 lines 3 comments Download
A chrome/browser/media_galleries/fileapi/itunes_data_provider.h View 1 2 3 4 1 chunk +78 lines, -0 lines 0 comments Download
A chrome/browser/media_galleries/fileapi/itunes_data_provider.cc View 1 2 3 4 5 6 1 chunk +209 lines, -0 lines 0 comments Download
M chrome/browser/media_galleries/fileapi/native_media_file_util.h View 3 chunks +39 lines, -38 lines 0 comments Download
M chrome/browser/media_galleries/fileapi/native_media_file_util.cc View 1 6 7 6 chunks +235 lines, -235 lines 0 comments Download
M chrome/browser/media_galleries/fileapi/picasa/picasa_file_util.cc View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/media_galleries/imported_media_gallery_registry.h View 1 2 3 4 4 chunks +26 lines, -4 lines 1 comment Download
M chrome/browser/media_galleries/imported_media_gallery_registry.cc View 1 2 3 4 5 3 chunks +93 lines, -27 lines 0 comments Download
M chrome/browser/media_galleries/media_file_system_registry.cc View 1 2 3 4 3 chunks +14 lines, -6 lines 0 comments Download
M chrome/browser/media_galleries/media_galleries_preferences.cc View 1 2 3 4 2 chunks +4 lines, -5 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 1 chunk +2 lines, -0 lines 0 comments Download
M content/browser/fileapi/fileapi_message_filter.cc View 1 2 3 4 5 1 chunk +3 lines, -1 line 1 comment Download
Commit: CQ not working?

Messages

Total messages: 20 (0 generated)
vandebo (ex-Chrome)
Checkpoint - compiles
2 years ago (2013-06-05 20:54:25 UTC) #1
vandebo (ex-Chrome)
Basic bug fixes
2 years ago (2013-06-05 22:18:04 UTC) #2
vandebo (ex-Chrome)
https://codereview.chromium.org/16158004/diff/6001/chrome/browser/media_galleries/fileapi/native_media_file_util.cc File chrome/browser/media_galleries/fileapi/native_media_file_util.cc (right): https://codereview.chromium.org/16158004/diff/6001/chrome/browser/media_galleries/fileapi/native_media_file_util.cc#newcode101 chrome/browser/media_galleries/fileapi/native_media_file_util.cc:101: // static Same here - just reordering methods to ...
2 years ago (2013-06-05 22:49:32 UTC) #3
vandebo (ex-Chrome)
+cc tommycli Tommy, since you've been working with this code a lot, feel free to ...
2 years ago (2013-06-06 03:45:41 UTC) #4
Lei Zhang (OOO)
Here's one incomplete pass. https://codereview.chromium.org/16158004/diff/6001/chrome/browser/media_galleries/fileapi/native_media_file_util.cc File chrome/browser/media_galleries/fileapi/native_media_file_util.cc (right): https://codereview.chromium.org/16158004/diff/6001/chrome/browser/media_galleries/fileapi/native_media_file_util.cc#newcode101 chrome/browser/media_galleries/fileapi/native_media_file_util.cc:101: // static On 2013/06/05 22:49:32, ...
2 years ago (2013-06-06 03:48:47 UTC) #5
Lei Zhang (OOO)
https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.cc File chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.cc (right): https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.cc#newcode32 chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.cc:32: fileapi::DirectoryEntry MakeDirectoryEntryFilePathType( Feels like DirectoryEntry needs a ctor. https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.cc#newcode111 ...
2 years ago (2013-06-06 09:58:52 UTC) #6
vandebo (ex-Chrome)
https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.cc File chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.cc (right): https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.cc#newcode32 chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.cc:32: fileapi::DirectoryEntry MakeDirectoryEntryFilePathType( On 2013/06/06 09:58:54, Lei Zhang wrote: > ...
2 years ago (2013-06-06 19:49:18 UTC) #7
vandebo (ex-Chrome)
+kinuko pleas review content/browser/fileapi/fileapi_message_filter.cc
2 years ago (2013-06-06 22:45:07 UTC) #8
Lei Zhang (OOO)
lgtm https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galleries/imported_media_gallery_registry.cc File chrome/browser/media_galleries/imported_media_gallery_registry.cc (right): https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galleries/imported_media_gallery_registry.cc#newcode91 chrome/browser/media_galleries/imported_media_gallery_registry.cc:91: return ""; On 2013/06/06 19:49:18, vandebo wrote: > ...
2 years ago (2013-06-07 03:29:07 UTC) #9
vandebo (ex-Chrome)
thanks https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galleries/imported_media_gallery_registry.cc File chrome/browser/media_galleries/imported_media_gallery_registry.cc (right): https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galleries/imported_media_gallery_registry.cc#newcode91 chrome/browser/media_galleries/imported_media_gallery_registry.cc:91: return ""; On 2013/06/07 03:29:07, Lei Zhang wrote: ...
2 years ago (2013-06-07 03:46:52 UTC) #10
Lei Zhang (OOO)
On 2013/06/07 03:46:52, vandebo wrote: > thanks > > https://codereview.chromium.org/16158004/diff/9001/chrome/browser/media_galleries/imported_media_gallery_registry.cc > File chrome/browser/media_galleries/imported_media_gallery_registry.cc (right): > ...
2 years ago (2013-06-07 04:32:07 UTC) #11
Lei Zhang (OOO)
On 2013/06/07 03:46:52, vandebo wrote: > Are you saying that we should add a CHECK ...
2 years ago (2013-06-07 04:34:26 UTC) #12
vandebo (ex-Chrome)
On 2013/06/07 04:34:26, Lei Zhang wrote: > On 2013/06/07 03:46:52, vandebo wrote: > > Are ...
2 years ago (2013-06-07 04:45:18 UTC) #13
kinuko
content/browser/fileapi/fileapi_message_filter lgtm (sorry for the delay) https://codereview.chromium.org/16158004/diff/31015/content/browser/fileapi/fileapi_message_filter.cc File content/browser/fileapi/fileapi_message_filter.cc (right): https://codereview.chromium.org/16158004/diff/31015/content/browser/fileapi/fileapi_message_filter.cc#newcode765 content/browser/fileapi/fileapi_message_filter.cc:765: url.type() == fileapi::kFileSystemTypeItunes); ...
2 years ago (2013-06-07 05:45:13 UTC) #14
kinuko
On Fri, Jun 7, 2013 at 2:45 PM, <kinuko@chromium.org> wrote: > content/browser/fileapi/**fileapi_message_filter lgtm (sorry for ...
2 years ago (2013-06-07 05:50:59 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vandebo@chromium.org/16158004/31015
2 years ago (2013-06-07 05:55:50 UTC) #16
Lei Zhang (OOO)
On 2013/06/07 04:45:18, vandebo wrote: > With this change the behavior of an empty fsid ...
2 years ago (2013-06-07 05:58:21 UTC) #17
commit-bot: I haz the power
Change committed as 204824
2 years ago (2013-06-07 14:03:01 UTC) #18
tommycli
https://codereview.chromium.org/16158004/diff/31015/chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.cc File chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.cc (right): https://codereview.chromium.org/16158004/diff/31015/chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.cc#newcode32 chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.cc:32: std::vector<std::string> GetComponents(const base::FilePath& path) { Given that this method ...
2 years ago (2013-06-10 17:33:24 UTC) #19
vandebo (ex-Chrome)
2 years ago (2013-06-11 05:24:37 UTC) #20
Message was sent while issue was closed.
https://codereview.chromium.org/16158004/diff/31015/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.cc (right):

https://codereview.chromium.org/16158004/diff/31015/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.cc:32:
std::vector<std::string> GetComponents(const base::FilePath& path) {
On 2013/06/10 17:33:25, tommycli wrote:
> Given that this method also appears in PicasaFileUtil, I think it's important
> that we figure out a way to de-dupe them.
> 
> Maybe we should put this as a static function in NativeMediaFileUtil?

We should just create a file with helper functions.  static
NativeMediaFileUtil::IsMediaFile is also used across classes.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1f9106d