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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 months, 2 weeks ago by vandebo
Modified:
10 months, 1 week ago
Reviewers:
Lei Zhang, kinuko, tommycli
CC:
chromium-reviews_chromium.org, 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) Lint 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 0 errors 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 ? errors Download
A chrome/browser/media_galleries/fileapi/itunes_data_provider.h View 1 2 3 4 1 chunk +78 lines, -0 lines 0 comments 1 errors 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 2 errors Download
M chrome/browser/media_galleries/fileapi/native_media_file_util.h View 3 chunks +39 lines, -38 lines 0 comments ? errors Download
M chrome/browser/media_galleries/fileapi/native_media_file_util.cc View 1 6 7 6 chunks +235 lines, -235 lines 0 comments ? errors 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 0 errors Download
M chrome/browser/media_galleries/imported_media_gallery_registry.h View 1 2 3 4 4 chunks +26 lines, -4 lines 1 comment ? errors 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 ? errors Download
M chrome/browser/media_galleries/media_file_system_registry.cc View 1 2 3 4 3 chunks +14 lines, -6 lines 0 comments 0 errors Download
M chrome/browser/media_galleries/media_galleries_preferences.cc View 1 2 3 4 2 chunks +4 lines, -5 lines 0 comments 0 errors Download
M chrome/chrome_browser.gypi View 1 1 chunk +2 lines, -0 lines 0 comments ? errors Download
M content/browser/fileapi/fileapi_message_filter.cc View 1 2 3 4 5 1 chunk +3 lines, -1 line 1 comment ? errors Download
Commit:

Messages

Total messages: 20
vandebo
Checkpoint - compiles
10 months, 2 weeks ago #1
vandebo
Basic bug fixes
10 months, 2 weeks ago #2
vandebo
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 ...
10 months, 2 weeks ago #3
vandebo
+cc tommycli Tommy, since you've been working with this code a lot, feel free to ...
10 months, 2 weeks ago #4
Lei Zhang
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, ...
10 months, 2 weeks ago #5
Lei Zhang
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 ...
10 months, 2 weeks ago #6
vandebo
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: > ...
10 months, 2 weeks ago #7
vandebo
+kinuko pleas review content/browser/fileapi/fileapi_message_filter.cc
10 months, 2 weeks ago #8
Lei Zhang
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: > ...
10 months, 2 weeks ago #9
vandebo
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: ...
10 months, 2 weeks ago #10
Lei Zhang
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): > ...
10 months, 2 weeks ago #11
Lei Zhang
On 2013/06/07 03:46:52, vandebo wrote: > Are you saying that we should add a CHECK ...
10 months, 2 weeks ago #12
vandebo
On 2013/06/07 04:34:26, Lei Zhang wrote: > On 2013/06/07 03:46:52, vandebo wrote: > > Are ...
10 months, 2 weeks ago #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); ...
10 months, 2 weeks ago #14
kinuko
On Fri, Jun 7, 2013 at 2:45 PM, <kinuko@chromium.org> wrote: > content/browser/fileapi/**fileapi_message_filter lgtm (sorry for ...
10 months, 2 weeks ago #15
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vandebo@chromium.org/16158004/31015
10 months, 2 weeks ago #16
Lei Zhang
On 2013/06/07 04:45:18, vandebo wrote: > With this change the behavior of an empty fsid ...
10 months, 2 weeks ago #17
I haz the power (commit-bot)
Change committed as 204824
10 months, 2 weeks ago #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 ...
10 months, 1 week ago #19
vandebo
10 months, 1 week ago #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 1275:d14800f88434