mac: Zip packages when they are selected by the file opener.

mac: Zip packages when they are selected by the file opener.

When a package is selected in the file opener, it is automatically zipped, and
the zipped file is passed to the render view host.

This CL fixes 2 unrelated bugs in file_select_helper:
- If the render_view_host_ was destroyed before the user finished with the
file_select_helper, then Release() would never be called and the
file_select_helper would leak.

- If the render_view_host_ was destroyed before the user finished with the
file_select_helper, the default directory would be updated if the user was
opening multiple files, but it would not be updated if the user was opening a
single file. Now both update the default directory.

Note: When a large package is being zipped, the UI thread and renderer are
still responsive, but display no indication of the ongoing work.

BUG=33920
Committed: https://crrev.com/22de64a3653834acad2580ded12857385e3d4d65
Cr-Commit-Position: refs/heads/master@{#299143}

[erikchen, 2014-10-07 23:14:13]

Patchset #1 (id:1) has been deleted

[erikchen, 2014-10-07 23:14:17]

Patchset #1 (id:20001) has been deleted

[erikchen, 2014-10-07 23:14:23]

Patchset #1 (id:40001) has been deleted

[erikchen, 2014-10-07 23:14:29]

Patchset #1 (id:60001) has been deleted

[erikchen, 2014-10-07 23:49:04]

Patchset #1 (id:80001) has been deleted

[erikchen, 2014-10-07 23:49:09]

Patchset #1 (id:100001) has been deleted

[erikchen, 2014-10-07 23:49:17]

Patchset #1 (id:120001) has been deleted

[erikchen, 2014-10-07 23:49:46]

erikchen@chromium.org changed reviewers:
+ avi@chromium.org

[erikchen, 2014-10-07 23:49:47]

avi: Please review.

[Avi (use Gerrit), 2014-10-08 00:17:32]

https://codereview.chromium.org/634833003/diff/140001/chrome/browser/file_sel...
File chrome/browser/file_select_helper.cc (right):

https://codereview.chromium.org/634833003/diff/140001/chrome/browser/file_sel...
chrome/browser/file_select_helper.cc:65: for (base::FilePath file_path : paths)
auto& perhaps?

https://codereview.chromium.org/634833003/diff/140001/chrome/browser/file_sel...
chrome/browser/file_select_helper.cc:175: NotifyRenderViewHostAndEnd(files);
Might as well put this into an #else.

https://codereview.chromium.org/634833003/diff/140001/chrome/browser/file_sel...
chrome/browser/file_select_helper.cc:501: }
So if there was zipping that happened, those temporary files will stick around
for as long as the tab does? Can we do better?

https://codereview.chromium.org/634833003/diff/140001/chrome/browser/file_sel...
File chrome/browser/file_select_helper_mac.mm (right):

https://codereview.chromium.org/634833003/diff/140001/chrome/browser/file_sel...
chrome/browser/file_select_helper_mac.mm:90: int dest_fd =
open(dest.value().c_str(), O_WRONLY | O_CREAT);
Please use base::File from base/files/file.h. Create it here, use
GetPlatformFile() to get the fd to pass to ZipFiles, etc.

https://codereview.chromium.org/634833003/diff/140001/chrome/browser/file_sel...
chrome/browser/file_select_helper_mac.mm:117: ++it) {
Why not for( : ) syntax here? You should be able to declare file_info right here
in the loop.

https://codereview.chromium.org/634833003/diff/140001/chrome/browser/file_sel...
chrome/browser/file_select_helper_mac.mm:120: [NSString
stringWithUTF8String:file_info.local_path.value().c_str()];
FilePathToNSString from foundation_util.

[erikchen, 2014-10-08 20:14:44]

avi: PTAL

https://codereview.chromium.org/634833003/diff/140001/chrome/browser/file_sel...
File chrome/browser/file_select_helper.cc (right):

https://codereview.chromium.org/634833003/diff/140001/chrome/browser/file_sel...
chrome/browser/file_select_helper.cc:65: for (base::FilePath file_path : paths)
On 2014/10/08 00:17:32, Avi wrote:
> auto& perhaps?

Done.

https://codereview.chromium.org/634833003/diff/140001/chrome/browser/file_sel...
chrome/browser/file_select_helper.cc:175: NotifyRenderViewHostAndEnd(files);
On 2014/10/08 00:17:32, Avi wrote:
> Might as well put this into an #else.

Done.

https://codereview.chromium.org/634833003/diff/140001/chrome/browser/file_sel...
chrome/browser/file_select_helper.cc:501: }
On 2014/10/08 00:17:32, Avi wrote:
> So if there was zipping that happened, those temporary files will stick around
> for as long as the tab does? Can we do better?

Sure. In the updated CL, the notification NOTIFICATION_RENDER_VIEW_HOST_CHANGED
will also kill the temporary files.

https://codereview.chromium.org/634833003/diff/140001/chrome/browser/file_sel...
File chrome/browser/file_select_helper_mac.mm (right):

https://codereview.chromium.org/634833003/diff/140001/chrome/browser/file_sel...
chrome/browser/file_select_helper_mac.mm:90: int dest_fd =
open(dest.value().c_str(), O_WRONLY | O_CREAT);
On 2014/10/08 00:17:32, Avi wrote:
> Please use base::File from base/files/file.h. Create it here, use
> GetPlatformFile() to get the fd to pass to ZipFiles, etc.

Done. I didn't see an alternative to fchmod, so I'm still using that.

https://codereview.chromium.org/634833003/diff/140001/chrome/browser/file_sel...
chrome/browser/file_select_helper_mac.mm:117: ++it) {
On 2014/10/08 00:17:32, Avi wrote:
> Why not for( : ) syntax here? You should be able to declare file_info right
here
> in the loop.

I didn't realize that the C++11 style discussions had already finished. My
previous use of for( : ) syntax was an oversight. I've changed the CL to use ':'
and auto.

https://codereview.chromium.org/634833003/diff/140001/chrome/browser/file_sel...
chrome/browser/file_select_helper_mac.mm:120: [NSString
stringWithUTF8String:file_info.local_path.value().c_str()];
On 2014/10/08 00:17:32, Avi wrote:
> FilePathToNSString from foundation_util.

Done.

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
File chrome/browser/file_select_helper_unittest.cc (right):

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
chrome/browser/file_select_helper_unittest.cc:47: TEST_F(FileSelectHelperTest,
ZipPackage) {
I changed the unit test to zip and unzip the package, and then check that
several specific files did not change. This means that the test is not longer
dependent on the specific zlib compression implementation.

[Avi (use Gerrit), 2014-10-08 20:28:18]

LGTM; if ifdefing the variable causes too much pain then don't bother.

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
File chrome/browser/file_select_helper.h (right):

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
chrome/browser/file_select_helper.h:194: std::vector<base::FilePath>
temporary_files_;
Don't say it in the comment, say it with an #if defined(OS_MACOSX)

[erikchen, 2014-10-08 20:33:29]

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
File chrome/browser/file_select_helper.h (right):

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
chrome/browser/file_select_helper.h:194: std::vector<base::FilePath>
temporary_files_;
On 2014/10/08 20:28:18, Avi wrote:
> Don't say it in the comment, say it with an #if defined(OS_MACOSX)

Doing so litters the implementation file with a lot more #ifs, so I'm going to
leave it in the comment.

[erikchen, 2014-10-08 20:33:34]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-10-08 20:37:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/634833003/160001

[commit-bot: I haz the power, 2014-10-08 22:11:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-08 22:11:12]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32 on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[erikchen, 2014-10-08 22:15:55]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-10-08 22:22:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/634833003/160001

[commit-bot: I haz the power, 2014-10-09 03:13:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-09 03:13:43]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[erikchen, 2014-10-09 17:10:44]

erikchen@chromium.org changed reviewers:
+ thestig@chromium.org

[erikchen, 2014-10-09 17:10:45]

thestig: Looking for an OWNER review of chrome/browser/*

[Lei Zhang, 2014-10-09 18:41:55]

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
File chrome/browser/file_select_helper.cc (right):

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
chrome/browser/file_select_helper.cc:66: base::DeleteFile(file_path, true);
Recursive deletion is scary. Do you really need to? It looks like you have a
list of files already.

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
File chrome/browser/file_select_helper_mac.mm (right):

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
chrome/browser/file_select_helper_mac.mm:48: int count =
base_dir_components.size();
size_t

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
chrome/browser/file_select_helper_mac.mm:59: for (base::FilePath path =
file_enumerator.Next(); !path.value().empty();
nit: !path.empty()

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
chrome/browser/file_select_helper_mac.mm:65: // Ignore the first |count|
components to create the relative path.
Are you reinventing FilePath::AppendRelativePath() here?

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
chrome/browser/file_select_helper_mac.mm:87: bool success =
base::CreateDirectory(dest.DirName());
nit: you don't really need |success|.

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
chrome/browser/file_select_helper_mac.mm:101: result =
fchmod(file.GetPlatformFile(), S_IRUSR);
I'm surprised the destination file don't sane permissions by default.

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
chrome/browser/file_select_helper_mac.mm:150: // Typically, |temporary_files|
are deleted after web_contents_ is
nit: |web_contents_|

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
File chrome/browser/file_select_helper_unittest.cc (right):

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
chrome/browser/file_select_helper_unittest.cc:20: virtual void SetUp() OVERRIDE
{
nit: we're switching to "override"

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
chrome/browser/file_select_helper_unittest.cc:23:
ASSERT_TRUE(base::PathExists(data_dir_));
Who creates this?

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
chrome/browser/file_select_helper_unittest.cc:49: const char* app_name =
"CalculatorFake.app";
const char app_name[], otherwise you can potentially change the pointer.

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
chrome/browser/file_select_helper_unittest.cc:52: EXPECT_FALSE(dest.empty());
Assert here, otherwise you'd pass an empty path to unzip.

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
chrome/browser/file_select_helper_unittest.cc:56:
EXPECT_TRUE(temp_dir.CreateUniqueTempDir());
You should assert true. Otherwise what happens if you pass an empty path to
unzip below?

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
chrome/browser/file_select_helper_unittest.cc:70: unsigned long file_count =
use arraysize() ?

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
chrome/browser/file_select_helper_unittest.cc:72: for (unsigned long i = 0; i <
file_count; i++) {
and size_t ?

[erikchen, 2014-10-09 23:33:36]

thestig: PTAL

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
File chrome/browser/file_select_helper.cc (right):

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
chrome/browser/file_select_helper.cc:66: base::DeleteFile(file_path, true);
On 2014/10/09 18:41:54, Lei Zhang wrote:
> Recursive deletion is scary. Do you really need to? It looks like you have a
> list of files already.

Got rid of the recursive deletion.

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
File chrome/browser/file_select_helper_mac.mm (right):

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
chrome/browser/file_select_helper_mac.mm:48: int count =
base_dir_components.size();
On 2014/10/09 18:41:54, Lei Zhang wrote:
> size_t

Done.

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
chrome/browser/file_select_helper_mac.mm:59: for (base::FilePath path =
file_enumerator.Next(); !path.value().empty();
On 2014/10/09 18:41:54, Lei Zhang wrote:
> nit: !path.empty()

Done.

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
chrome/browser/file_select_helper_mac.mm:65: // Ignore the first |count|
components to create the relative path.
On 2014/10/09 18:41:54, Lei Zhang wrote:
> Are you reinventing FilePath::AppendRelativePath() here?

Alas...no.

zip::ZipFiles() requires a list of relative paths. (Don't get me started on the
other functions in zip:: They're basically unusable).
FileEnumerator returns a list of absolute paths. This logic converts absolute
paths to relative paths. If you know of utility methods to do that, let me
know...

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
chrome/browser/file_select_helper_mac.mm:87: bool success =
base::CreateDirectory(dest.DirName());
On 2014/10/09 18:41:54, Lei Zhang wrote:
> nit: you don't really need |success|.

I removed this instance of success, but I kept the one at line 97. I dislike
putting too much text into conditionals, because I find it more difficult to
read.

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
chrome/browser/file_select_helper_mac.mm:101: result =
fchmod(file.GetPlatformFile(), S_IRUSR);
On 2014/10/09 18:41:54, Lei Zhang wrote:
> I'm surprised the destination file don't sane permissions by default.

Me too.

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
chrome/browser/file_select_helper_mac.mm:150: // Typically, |temporary_files|
are deleted after web_contents_ is
On 2014/10/09 18:41:54, Lei Zhang wrote:
> nit: |web_contents_|

Done.

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
File chrome/browser/file_select_helper_unittest.cc (right):

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
chrome/browser/file_select_helper_unittest.cc:20: virtual void SetUp() OVERRIDE
{
On 2014/10/09 18:41:55, Lei Zhang wrote:
> nit: we're switching to "override"

Done.

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
chrome/browser/file_select_helper_unittest.cc:23:
ASSERT_TRUE(base::PathExists(data_dir_));
On 2014/10/09 18:41:55, Lei Zhang wrote:
> Who creates this?

it's already there. it's a test directory that got added in this CL.

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
chrome/browser/file_select_helper_unittest.cc:49: const char* app_name =
"CalculatorFake.app";
On 2014/10/09 18:41:55, Lei Zhang wrote:
> const char app_name[], otherwise you can potentially change the pointer.

Done.

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
chrome/browser/file_select_helper_unittest.cc:52: EXPECT_FALSE(dest.empty());
On 2014/10/09 18:41:55, Lei Zhang wrote:
> Assert here, otherwise you'd pass an empty path to unzip.

Done. Asserted the line after this as well, since the rest of this test doesn't
make sense of there's no directory at "dest".

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
chrome/browser/file_select_helper_unittest.cc:56:
EXPECT_TRUE(temp_dir.CreateUniqueTempDir());
On 2014/10/09 18:41:55, Lei Zhang wrote:
> You should assert true. Otherwise what happens if you pass an empty path to
> unzip below?

Done.

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
chrome/browser/file_select_helper_unittest.cc:70: unsigned long file_count =
On 2014/10/09 18:41:55, Lei Zhang wrote:
> use arraysize() ?

Done.

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
chrome/browser/file_select_helper_unittest.cc:72: for (unsigned long i = 0; i <
file_count; i++) {
On 2014/10/09 18:41:54, Lei Zhang wrote:
> and size_t ?

Done.

[Lei Zhang, 2014-10-10 01:32:28]

lgtm, but please give FilePath::AppendRelativePath() a shot and see if it will
simplify your code.

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
File chrome/browser/file_select_helper_mac.mm (right):

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
chrome/browser/file_select_helper_mac.mm:65: // Ignore the first |count|
components to create the relative path.
On 2014/10/09 23:33:35, erikchen wrote:
> On 2014/10/09 18:41:54, Lei Zhang wrote:
> > Are you reinventing FilePath::AppendRelativePath() here?
> 
> Alas...no.
> 
> zip::ZipFiles() requires a list of relative paths. (Don't get me started on
the
> other functions in zip:: They're basically unusable).
> FileEnumerator returns a list of absolute paths. This logic converts absolute
> paths to relative paths. If you know of utility methods to do that, let me
> know...

I still suspect base_dir.AppendRelativePath(path, &relative_path); may do
exactly what you want here.

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
File chrome/browser/file_select_helper_unittest.cc (right):

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
chrome/browser/file_select_helper_unittest.cc:23:
ASSERT_TRUE(base::PathExists(data_dir_));
On 2014/10/09 23:33:36, erikchen wrote:
> On 2014/10/09 18:41:55, Lei Zhang wrote:
> > Who creates this?
> 
> it's already there. it's a test directory that got added in this CL.

Ah, that's what I get for ignoring the rest of the CL.

[erikchen, 2014-10-10 17:21:39]

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
File chrome/browser/file_select_helper_mac.mm (right):

https://codereview.chromium.org/634833003/diff/160001/chrome/browser/file_sel...
chrome/browser/file_select_helper_mac.mm:65: // Ignore the first |count|
components to create the relative path.
On 2014/10/10 01:32:28, Lei Zhang wrote:
> On 2014/10/09 23:33:35, erikchen wrote:
> > On 2014/10/09 18:41:54, Lei Zhang wrote:
> > > Are you reinventing FilePath::AppendRelativePath() here?
> > 
> > Alas...no.
> > 
> > zip::ZipFiles() requires a list of relative paths. (Don't get me started on
> the
> > other functions in zip:: They're basically unusable).
> > FileEnumerator returns a list of absolute paths. This logic converts
absolute
> > paths to relative paths. If you know of utility methods to do that, let me
> > know...
> 
> I still suspect base_dir.AppendRelativePath(path, &relative_path); may do
> exactly what you want here.

You're right! The name of the method primed me to think that it wouldn't do the
right thing, and the comment was accurate, but reinforced the priming.

[erikchen, 2014-10-10 17:21:44]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-10-10 17:22:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/634833003/300001

[commit-bot: I haz the power, 2014-10-10 18:27:59]

Committed patchset #4 (id:300001)

[commit-bot: I haz the power, 2014-10-10 18:28:36]

Patchset 4 (id:??) landed as
https://crrev.com/22de64a3653834acad2580ded12857385e3d4d65
Cr-Commit-Position: refs/heads/master@{#299143}