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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 5 months ago by hshi1
Modified:
1 year, 4 months ago
CC:
chromium-reviews_chromium.org, nkostylev+watch_chromium.org, Aaron Boodman, rginda+watch_chromium.org, arv, 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) Lint Patch
M chrome/app/generated_resources.grd View 2 chunks +22 lines, -0 lines 0 comments ? errors 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 ? errors 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 ? errors 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 2 errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors Download
Commit:

Messages

Total messages: 51
hshi1
Hi could you review this draft CL (particularly the JS / fileBrowserPrivateApi part)? It is ...
1 year, 5 months ago #1
hshi1
P.S. I have decided that this CL is becoming too large to get landed in ...
1 year, 5 months ago #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.
1 year, 5 months ago #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 ...
1 year, 5 months ago #4
hshi1
Adding Nico and James for the changes in the common/ and utility/: M chrome/common/chrome_utility_messages.h M ...
1 year, 5 months ago #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 ...
1 year, 5 months ago #6
hshi1
Thanks Toni for the feedback. I've addressed a few easy ones. Will work on the ...
1 year, 5 months ago #7
satorux1
I'd suggest to split the changes in chrome/common/zip into a separate patch. I'd be happy ...
1 year, 5 months ago #8
hshi1
On 2012/11/07 04:58:00, satorux1 wrote: > I'd suggest to split the changes in chrome/common/zip into ...
1 year, 5 months ago #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 ...
1 year, 5 months ago #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 ...
1 year, 5 months ago #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: > ...
1 year, 5 months ago #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() ...
1 year, 5 months ago #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 ...
1 year, 5 months ago #14
tbarzic
lgtm
1 year, 5 months ago #15
hshi1
Ping again for review for changes under the chrome common directories. Adding jln@ and palmer@ ...
1 year, 5 months ago #16
James Hawkins
On 2012/11/13 18:47:05, hshi1 wrote: > Ping again for review for changes under the chrome ...
1 year, 5 months ago #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@, ...
1 year, 5 months ago #18
James Hawkins
On 2012/11/13 18:53:55, hshi1 wrote: > Need owner review for the following: > > mihaip@: ...
1 year, 5 months ago #19
hshi1
Sorry about that. Removing thakis@ and jln@ -- just keep one owner for each set ...
1 year, 5 months ago #20
Mihai Parparita -not on Chrome
s/mihaip/miket/
1 year, 5 months ago #21
James Hawkins
lgtm
1 year, 5 months ago #22
Chromium Palmer
Re-adding jln for another pair of eyes. We need to make sure this IPC does ...
1 year, 5 months ago #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? ...
1 year, 5 months ago #24
hshi1
>> The utility process is not sandboxed by default. For this case, is it necessary ...
1 year, 5 months ago #25
hshi1
Chris, I understand the part about path validation. But is it really necessary to change ...
1 year, 5 months ago #26
Jorge Lucangeli Obes
On 2012/11/13 22:07:49, hshi1 wrote: > >> The utility process is not sandboxed by > ...
1 year, 5 months ago #27
miket
chrome/browser/extensions/extension_function_registry.cc LGTM
1 year, 5 months ago #28
hshi1
I see. I guess the question is whether it is acceptable for the zip-file utility ...
1 year, 5 months ago #29
hshi1
Sorry for the typo. I meant "for the zip-file utility process to be not sandboxed". ...
1 year, 5 months ago #30
Jorge Lucangeli Obes
On 2012/11/13 22:18:08, hshi1 wrote: > Chris, > > I understand the part about path ...
1 year, 5 months ago #31
Jorge Lucangeli Obes
On 2012/11/13 22:39:19, hshi1 wrote: > I see. I guess the question is whether it ...
1 year, 5 months ago #32
jln
On Tue, Nov 13, 2012 at 2:42 PM, <jorgelo@chromium.org> wrote: > On 2012/11/13 22:18:08, hshi1 ...
1 year, 5 months ago #33
hshi1
Jorge, would it be ok if I just clarify in the comments that the utility ...
1 year, 5 months ago #34
Chromium Palmer
> I understand the part about path validation. But is it really necessary to > ...
1 year, 5 months ago #35
hshi1
Jorge, Chris -- thanks for all the feedback. I agree that it would be the ...
1 year, 5 months ago #36
Jorge Lucangeli Obes
On 2012/11/13 22:56:16, hshi1 wrote: > Jorge, would it be ok if I just clarify ...
1 year, 5 months ago #37
hshi1
>> As a minimum, the dest file should be passed as an fd to the ...
1 year, 5 months ago #38
Jorge Lucangeli Obes
On 2012/11/13 23:20:29, hshi1 wrote: > >> As a minimum, the dest file should be ...
1 year, 5 months ago #39
hshi1
>> for rel_src_file in src_files: check(rel_src_file) // i.e. they should not have .. or . ...
1 year, 5 months ago #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. ...
1 year, 5 months ago #41
hshi1
>> is it problematic to plumb the fd's? Hi Jorge, sorry for the delayed reply. ...
1 year, 4 months ago #42
Jorge Lucangeli Obes
On 2012/11/20 19:10:07, hshi1 wrote: > >> is it problematic to plumb the fd's? > ...
1 year, 4 months ago #43
Chromium Palmer
> So for now I think we can settle on passing an fd for the ...
1 year, 4 months ago #44
hshi1
Jorge/Chris, please review Patch Set #30. What I did in patches 29,30: (1) use base::PlatformFile ...
1 year, 4 months ago #45
Chromium Palmer
LGTM. Thank you!
1 year, 4 months ago #46
Jorge Lucangeli Obes
lgtm, but please fix the comment if you can. Chris P needs to ACK for ...
1 year, 4 months ago #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 ...
1 year, 4 months ago #48
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hshi@chromium.org/11309014/57033
1 year, 4 months ago #49
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hshi@chromium.org/11309014/61014
1 year, 4 months ago #50
I haz the power (commit-bot)
1 year, 4 months ago #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 1280:2d3e6564b7b6