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

Issue 11309014: File manager: support for zipping selected files. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 10 months ago by hshi1
Modified:
2 years, 9 months ago
CC:
chromium-reviews, nkostylev+watch_chromium.org, Aaron Boodman, rginda+watch_chromium.org, arv (Not doing code reviews), oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org
Visibility:
Public.

Description

File manager: support for zipping selected files. Adds a new command "Zip selection" in the file manager context menu that creates a zip file for the selected files and folders. TODO: zipping on Drive is not yet supported. Tracked under bug 158690. BUG=138359 TEST=manual, CQ Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=169212

Patch Set 1 #

Patch Set 2 : Hook up JS to C++ layer (fileBrowserPrivateApi). #

Patch Set 3 : Barely functional for one selected directory. TODO: handle array of mixed files/directories; move t… #

Patch Set 4 : Replace 'createZipFile' with 'zipSelection' as suggested by Dave. #

Patch Set 5 : Add destName as an argument to fileBrowserPrivate.zipSelection. #

Patch Set 6 : Keep the refcount on the delegate until reponse is received. Fixes the pure virtual function crash. #

Patch Set 7 : Now the zip file creation is functional in the utility process. #

Patch Set 8 : Disable the zip-selection command on Drive for the time being. #

Patch Set 9 : Let JS layer decide the directory path. Remove the hack that uses the 1st file entry. #

Patch Set 10 : Fix 'complex constructor has an inline body' style violation. #

Patch Set 11 : Add error handler and renaming logic. #

Patch Set 12 : Rebase @ SVN 165698. #

Patch Set 13 : Set the pending bytes for zip task to zero. TODO: need to implement per-entry progress update to tr… #

Total comments: 6

Patch Set 14 : Fix nit - indentation in file_copy_manager.js #

Patch Set 15 : Rebase @ SVN 166228. #

Patch Set 16 : Define zip::ZipFileList method to internalize details with handling file lists. #

Total comments: 2

Patch Set 17 : Remove the unused function (ZipSelectionFilter). #

Total comments: 14

Patch Set 18 : Fix lint warning of header guard in zip-file_creator.h #

Patch Set 19 : Partially address Toni's comments (still a few left todo). #

Patch Set 20 : Change the semantics of |src_file_list| to be relative paths to avoid redundant IsParent checks in … #

Patch Set 21 : Rebase SVN 167030; use the finalized zip::ZipFiles interface. #

Patch Set 22 : Still missing unit test for ZipFileCreator. #

Total comments: 12

Patch Set 23 : Rebase @ SVN 167212. #

Patch Set 24 : Address Toni's comments for Patch Sets #13, #17 and #22. #

Total comments: 4

Patch Set 25 : More comments on Patch Set #24. #

Patch Set 26 : Rebase @ SVN 167426. #

Total comments: 2

Patch Set 27 : Rebase @ SVN 168247 and resolve merge conflicts. #

Patch Set 28 : Rebase @ SVN 168842, pass dest file in fd. TODO: validate src paths. #

Patch Set 29 : Rebase @ 169091 (after the dest_fd change for zip::ZipFiles() landed). #

Patch Set 30 : Sanitize source relative paths to reject absolute and trick paths. #

Total comments: 2

Patch Set 31 : Nit: fix comment. #

Patch Set 32 : Fix compiler warning: declare base::FileDescriptor a struct, not a class. The struct is put after t… #

Unified diffs Side-by-side diffs Delta from patch set Stats (+607 lines, -6 lines) Patch
M chrome/app/generated_resources.grd View 2 chunks +22 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/extensions/file_browser_private_api.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 2 chunks +27 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/extensions/file_browser_private_api.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 5 chunks +106 lines, -0 lines 0 comments Download
A chrome/browser/chromeos/extensions/zip_file_creator.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +90 lines, -0 lines 0 comments Download
A chrome/browser/chromeos/extensions/zip_file_creator.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 1 chunk +117 lines, -0 lines 0 comments Download
M chrome/browser/extensions/extension_function_registry.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/resources/file_manager/js/butter_bar.js View 1 2 3 4 5 6 7 8 9 10 1 chunk +6 lines, -1 line 0 comments Download
M chrome/browser/resources/file_manager/js/file_copy_manager.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 6 chunks +115 lines, -2 lines 0 comments Download
M chrome/browser/resources/file_manager/js/file_copy_manager_wrapper.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/resources/file_manager/js/file_manager.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 2 chunks +5 lines, -2 lines 0 comments Download
M chrome/browser/resources/file_manager/js/file_manager_commands.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +17 lines, -0 lines 0 comments Download
M chrome/browser/resources/file_manager/main.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 2 chunks +3 lines, -0 lines 0 comments Download
M chrome/chrome_browser_chromeos.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 2 chunks +4 lines, -0 lines 0 comments Download
M chrome/common/chrome_utility_messages.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 3 chunks +17 lines, -0 lines 0 comments Download
M chrome/common/extensions/api/file_browser_private.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +34 lines, -0 lines 0 comments Download
M chrome/utility/chrome_content_utility_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 2 chunks +7 lines, -0 lines 0 comments Download
M chrome/utility/chrome_content_utility_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 5 chunks +35 lines, -1 line 0 comments Download
Project "None" does not have a commit queue.

Messages

Total messages: 51 (0 generated)
hshi1
Hi could you review this draft CL (particularly the JS / fileBrowserPrivateApi part)? It is ...
2 years, 10 months ago (2012-11-02 18:27:38 UTC) #1
hshi1
P.S. I have decided that this CL is becoming too large to get landed in ...
2 years, 10 months ago (2012-11-02 22:50:53 UTC) #2
Oleg Eterevsky
Changes to FileManager js code lgtm. http://codereview.chromium.org/11309014/diff/41009/chrome/browser/resources/file_manager/js/file_copy_manager.js File chrome/browser/resources/file_manager/js/file_copy_manager.js (right): http://codereview.chromium.org/11309014/diff/41009/chrome/browser/resources/file_manager/js/file_copy_manager.js#newcode954 chrome/browser/resources/file_manager/js/file_copy_manager.js:954: errorCallback) { Indentation.
2 years, 10 months ago (2012-11-06 10:07:42 UTC) #3
hshi1
Fixed indentation in file_copy_manager.js. I'll add other owners for review. http://codereview.chromium.org/11309014/diff/41009/chrome/browser/resources/file_manager/js/file_copy_manager.js File chrome/browser/resources/file_manager/js/file_copy_manager.js (right): http://codereview.chromium.org/11309014/diff/41009/chrome/browser/resources/file_manager/js/file_copy_manager.js#newcode954 ...
2 years, 9 months ago (2012-11-06 18:01:02 UTC) #4
hshi1
Adding Nico and James for the changes in the common/ and utility/: M chrome/common/chrome_utility_messages.h M ...
2 years, 9 months ago (2012-11-06 19:00:38 UTC) #5
tbarzic
http://codereview.chromium.org/11309014/diff/41009/chrome/browser/chromeos/extensions/zip_file_creator.h File chrome/browser/chromeos/extensions/zip_file_creator.h (right): http://codereview.chromium.org/11309014/diff/41009/chrome/browser/chromeos/extensions/zip_file_creator.h#newcode35 chrome/browser/chromeos/extensions/zip_file_creator.h:35: class Delegate { Looks like this class reacts on ...
2 years, 9 months ago (2012-11-06 19:41:25 UTC) #6
hshi1
Thanks Toni for the feedback. I've addressed a few easy ones. Will work on the ...
2 years, 9 months ago (2012-11-06 20:42:59 UTC) #7
satorux1
I'd suggest to split the changes in chrome/common/zip into a separate patch. I'd be happy ...
2 years, 9 months ago (2012-11-07 04:58:00 UTC) #8
hshi1
On 2012/11/07 04:58:00, satorux1 wrote: > I'd suggest to split the changes in chrome/common/zip into ...
2 years, 9 months ago (2012-11-07 05:13:17 UTC) #9
hshi1
http://codereview.chromium.org/11309014/diff/31030/chrome/browser/chromeos/extensions/zip_file_creator.cc File chrome/browser/chromeos/extensions/zip_file_creator.cc (right): http://codereview.chromium.org/11309014/diff/31030/chrome/browser/chromeos/extensions/zip_file_creator.cc#newcode43 chrome/browser/chromeos/extensions/zip_file_creator.cc:43: // We assume that we are started on the ...
2 years, 9 months ago (2012-11-10 02:29:11 UTC) #10
tbarzic
http://codereview.chromium.org/11309014/diff/31030/chrome/browser/chromeos/extensions/zip_file_creator.cc File chrome/browser/chromeos/extensions/zip_file_creator.cc (right): http://codereview.chromium.org/11309014/diff/31030/chrome/browser/chromeos/extensions/zip_file_creator.cc#newcode43 chrome/browser/chromeos/extensions/zip_file_creator.cc:43: // We assume that we are started on the ...
2 years, 9 months ago (2012-11-10 03:40:56 UTC) #11
hshi1
http://codereview.chromium.org/11309014/diff/41009/chrome/browser/chromeos/extensions/zip_file_creator.h File chrome/browser/chromeos/extensions/zip_file_creator.h (right): http://codereview.chromium.org/11309014/diff/41009/chrome/browser/chromeos/extensions/zip_file_creator.h#newcode35 chrome/browser/chromeos/extensions/zip_file_creator.h:35: class Delegate { On 2012/11/06 19:41:25, tbarzic wrote: > ...
2 years, 9 months ago (2012-11-12 20:12:11 UTC) #12
tbarzic
http://codereview.chromium.org/11309014/diff/31054/chrome/browser/chromeos/extensions/file_browser_private_api.cc File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right): http://codereview.chromium.org/11309014/diff/31054/chrome/browser/chromeos/extensions/file_browser_private_api.cc#newcode2995 chrome/browser/chromeos/extensions/file_browser_private_api.cc:2995: BrowserThread::FILE, FROM_HERE, it should be save to call zip_file_creator_->Start() ...
2 years, 9 months ago (2012-11-12 20:29:37 UTC) #13
hshi1
http://codereview.chromium.org/11309014/diff/31054/chrome/browser/chromeos/extensions/file_browser_private_api.cc File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right): http://codereview.chromium.org/11309014/diff/31054/chrome/browser/chromeos/extensions/file_browser_private_api.cc#newcode2995 chrome/browser/chromeos/extensions/file_browser_private_api.cc:2995: BrowserThread::FILE, FROM_HERE, On 2012/11/12 20:29:37, tbarzic wrote: > it ...
2 years, 9 months ago (2012-11-12 21:42:17 UTC) #14
tbarzic
lgtm
2 years, 9 months ago (2012-11-12 21:43:31 UTC) #15
hshi1
Ping again for review for changes under the chrome common directories. Adding jln@ and palmer@ ...
2 years, 9 months ago (2012-11-13 18:47:05 UTC) #16
James Hawkins
On 2012/11/13 18:47:05, hshi1 wrote: > Ping again for review for changes under the chrome ...
2 years, 9 months ago (2012-11-13 18:48:17 UTC) #17
hshi1
Need owner review for the following: mihaip@: chrome/browser/extensions/extension_function_registry.cc jhawkins@, thakis@: chrome/chrome_browser_chromeos.gypi chrome/common/extensions/api/file_browser_private.json chrome/utility/chrome_content_utility_client.h chrome/utility/chrome_content_utility_client.cc jln@, ...
2 years, 9 months ago (2012-11-13 18:53:55 UTC) #18
James Hawkins
On 2012/11/13 18:53:55, hshi1 wrote: > Need owner review for the following: > > mihaip@: ...
2 years, 9 months ago (2012-11-13 18:56:58 UTC) #19
hshi1
Sorry about that. Removing thakis@ and jln@ -- just keep one owner for each set ...
2 years, 9 months ago (2012-11-13 18:58:35 UTC) #20
Mihai Parparita -not on Chrome
s/mihaip/miket/
2 years, 9 months ago (2012-11-13 18:58:38 UTC) #21
James Hawkins
lgtm
2 years, 9 months ago (2012-11-13 19:05:07 UTC) #22
palmer
Re-adding jln for another pair of eyes. We need to make sure this IPC does ...
2 years, 9 months ago (2012-11-13 20:43:40 UTC) #23
Jorge Lucangeli Obes
https://codereview.chromium.org/11309014/diff/34058/chrome/browser/chromeos/extensions/zip_file_creator.h File chrome/browser/chromeos/extensions/zip_file_creator.h (right): https://codereview.chromium.org/11309014/diff/34058/chrome/browser/chromeos/extensions/zip_file_creator.h#newcode21 chrome/browser/chromeos/extensions/zip_file_creator.h:21: // from untrusted sources. How is the subprocess sandboxed? ...
2 years, 9 months ago (2012-11-13 21:03:35 UTC) #24
hshi1
>> The utility process is not sandboxed by default. For this case, is it necessary ...
2 years, 9 months ago (2012-11-13 22:07:49 UTC) #25
hshi1
Chris, I understand the part about path validation. But is it really necessary to change ...
2 years, 9 months ago (2012-11-13 22:18:08 UTC) #26
Jorge Lucangeli Obes
On 2012/11/13 22:07:49, hshi1 wrote: > >> The utility process is not sandboxed by > ...
2 years, 9 months ago (2012-11-13 22:20:34 UTC) #27
miket_OOO
chrome/browser/extensions/extension_function_registry.cc LGTM
2 years, 9 months ago (2012-11-13 22:37:09 UTC) #28
hshi1
I see. I guess the question is whether it is acceptable for the zip-file utility ...
2 years, 9 months ago (2012-11-13 22:39:19 UTC) #29
hshi1
Sorry for the typo. I meant "for the zip-file utility process to be not sandboxed". ...
2 years, 9 months ago (2012-11-13 22:40:06 UTC) #30
Jorge Lucangeli Obes
On 2012/11/13 22:18:08, hshi1 wrote: > Chris, > > I understand the part about path ...
2 years, 9 months ago (2012-11-13 22:42:00 UTC) #31
Jorge Lucangeli Obes
On 2012/11/13 22:39:19, hshi1 wrote: > I see. I guess the question is whether it ...
2 years, 9 months ago (2012-11-13 22:43:39 UTC) #32
jln (slow on Chromium)
On Tue, Nov 13, 2012 at 2:42 PM, <jorgelo@chromium.org> wrote: > On 2012/11/13 22:18:08, hshi1 ...
2 years, 9 months ago (2012-11-13 22:45:09 UTC) #33
hshi1
Jorge, would it be ok if I just clarify in the comments that the utility ...
2 years, 9 months ago (2012-11-13 22:56:16 UTC) #34
palmer
> I understand the part about path validation. But is it really necessary to > ...
2 years, 9 months ago (2012-11-13 22:59:55 UTC) #35
hshi1
Jorge, Chris -- thanks for all the feedback. I agree that it would be the ...
2 years, 9 months ago (2012-11-13 23:16:05 UTC) #36
Jorge Lucangeli Obes
On 2012/11/13 22:56:16, hshi1 wrote: > Jorge, would it be ok if I just clarify ...
2 years, 9 months ago (2012-11-13 23:18:22 UTC) #37
hshi1
>> As a minimum, the dest file should be passed as an fd to the ...
2 years, 9 months ago (2012-11-13 23:20:29 UTC) #38
Jorge Lucangeli Obes
On 2012/11/13 23:20:29, hshi1 wrote: > >> As a minimum, the dest file should be ...
2 years, 9 months ago (2012-11-13 23:28:09 UTC) #39
hshi1
>> for rel_src_file in src_files: check(rel_src_file) // i.e. they should not have .. or . ...
2 years, 9 months ago (2012-11-15 18:21:54 UTC) #40
Jorge Lucangeli Obes
On 2012/11/15 18:21:54, hshi1 wrote: > >> for rel_src_file in src_files: > check(rel_src_file) // i.e. ...
2 years, 9 months ago (2012-11-15 20:08:33 UTC) #41
hshi1
>> is it problematic to plumb the fd's? Hi Jorge, sorry for the delayed reply. ...
2 years, 9 months ago (2012-11-20 19:10:07 UTC) #42
Jorge Lucangeli Obes
On 2012/11/20 19:10:07, hshi1 wrote: > >> is it problematic to plumb the fd's? > ...
2 years, 9 months ago (2012-11-20 19:23:10 UTC) #43
palmer
> So for now I think we can settle on passing an fd for the ...
2 years, 9 months ago (2012-11-20 19:30:02 UTC) #44
hshi1
Jorge/Chris, please review Patch Set #30. What I did in patches 29,30: (1) use base::PlatformFile ...
2 years, 9 months ago (2012-11-21 23:04:54 UTC) #45
palmer
LGTM. Thank you!
2 years, 9 months ago (2012-11-22 00:27:21 UTC) #46
Jorge Lucangeli Obes
lgtm, but please fix the comment if you can. Chris P needs to ACK for ...
2 years, 9 months ago (2012-11-22 00:28:20 UTC) #47
hshi1
https://codereview.chromium.org/11309014/diff/57028/chrome/utility/chrome_content_utility_client.cc File chrome/utility/chrome_content_utility_client.cc (right): https://codereview.chromium.org/11309014/diff/57028/chrome/utility/chrome_content_utility_client.cc#newcode202 chrome/utility/chrome_content_utility_client.cc:202: // Check sanity for source relative paths. Reject if ...
2 years, 9 months ago (2012-11-22 00:35:45 UTC) #48
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hshi@chromium.org/11309014/57033
2 years, 9 months ago (2012-11-22 00:40:55 UTC) #49
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hshi@chromium.org/11309014/61014
2 years, 9 months ago (2012-11-22 01:02:30 UTC) #50
commit-bot: I haz the power
2 years, 9 months ago (2012-11-22 03:49:36 UTC) #51
Change committed as 169212
Sign in to reply to this message.

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