[filebrowser] Fix the browser_tests using invalid mount point.

[filebrowser] Fix the browser_tests using invalid mount point.

BUG=chromium-os:23809
TEST=SelectFileDialogExtensionBrowserTest.*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113701

[dgozman, 2011-12-06 16:09:51]

Please, take a look.

[zel, 2011-12-06 16:14:19]

http://codereview.chromium.org/8819025/diff/1/chrome/browser/ui/views/select_...
File chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc
(right):

http://codereview.chromium.org/8819025/diff/1/chrome/browser/ui/views/select_...
chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc:222:
file_util::CreateDirectory(tmp_dir);
You should use ScopedTempDir class for this purpose.


http://codesearch.google.com/#OAMlx_jo-ck/src/base/scoped_temp_dir.h

[SeRya, 2011-12-06 16:18:31]

LGTM with comment

http://codereview.chromium.org/8819025/diff/1/webkit/chromeos/fileapi/cros_mo...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

http://codereview.chromium.org/8819025/diff/1/webkit/chromeos/fileapi/cros_mo...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:137:
mount_point_map_.insert(std::pair<std::string, FilePath>(
This two line are equal to:

mount_point_map_[mount_point.BaseName().value()] = mount_point.DirName();

[dgozman, 2011-12-06 17:11:54]

http://codereview.chromium.org/8819025/diff/1/chrome/browser/ui/views/select_...
File chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc
(right):

http://codereview.chromium.org/8819025/diff/1/chrome/browser/ui/views/select_...
chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc:222:
file_util::CreateDirectory(tmp_dir);
On 2011/12/06 16:14:19, zel wrote:
> You should use ScopedTempDir class for this purpose.
> 
> 
> http://codesearch.google.com/#OAMlx_jo-ck/src/base/scoped_temp_dir.h

Done.

[commit-bot: I haz the power, 2011-12-06 17:12:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgozman@chromium.org/8819025/4001

[commit-bot: I haz the power, 2011-12-06 17:12:30]

Presubmit check for 8819025-4001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
webkit/chromeos/fileapi/cros_mount_point_provider.cc

[dgozman, 2011-12-06 17:18:21]

Darin, Scott,

May you have a look and approve?
You are listed in OWNERS.

Thanks,
Dmitry

[sky, 2011-12-06 17:23:59]

http://codereview.chromium.org/8819025/diff/4001/chrome/browser/ui/views/sele...
File chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc
(right):

http://codereview.chromium.org/8819025/diff/4001/chrome/browser/ui/views/sele...
chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc:251:
FilePath tmp_dir("/tmp/Downloads");
You want PathService::Get(base::DIR_TEMP, &tmp_path);

[zel, 2011-12-06 17:29:29]

http://codereview.chromium.org/8819025/diff/4001/chrome/browser/ui/views/sele...
File chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc
(right):

http://codereview.chromium.org/8819025/diff/4001/chrome/browser/ui/views/sele...
chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc:223:
ASSERT_TRUE(downloads_dir.Set(tmp_dir));
I suggest something like this instead:

ScopedTempDir downloads_dir;
ASSERT_TRUE(downloads_dir.CreateUniqueTempDirUnderPath("/tmp"));
FilePath dir_path = downloads_dir.Take();    // get path
ASSERT_TRUE(downloads_dir.Set(dir_path));    // Reclaim ownership
ASSERT_TRUE(file_util::CreateDirectory(dir_path.Append("Downloads")));   //
create Downloads sub dir

This will make your code more resilient to random stuff that might still reside
in /tmp/Downloads after test runs.

[zel, 2011-12-06 17:42:01]

http://codereview.chromium.org/8819025/diff/4001/chrome/browser/ui/views/sele...
File chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc
(right):

http://codereview.chromium.org/8819025/diff/4001/chrome/browser/ui/views/sele...
chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc:223:
ASSERT_TRUE(downloads_dir.Set(tmp_dir));
On 2011/12/06 17:29:29, zel wrote:
> I suggest something like this instead:
> 
> ScopedTempDir downloads_dir;
> ASSERT_TRUE(downloads_dir.CreateUniqueTempDirUnderPath("/tmp"));
> FilePath dir_path = downloads_dir.Take();    // get path
> ASSERT_TRUE(downloads_dir.Set(dir_path));    // Reclaim ownership
> ASSERT_TRUE(file_util::CreateDirectory(dir_path.Append("Downloads")));   //
> create Downloads sub dir
> 
> This will make your code more resilient to random stuff that might still
reside
> in /tmp/Downloads after test runs.
> 

Shorter version:

ScopedTempDir downloads_dir;
ASSERT_TRUE(downloads_dir.CreateUniqueTempDirUnderPath("/tmp"));
ASSERT_TRUE(file_util::CreateDirectory(downloads_dir.path().Append("Downloads")));

[dgozman, 2011-12-07 11:20:41]

http://codereview.chromium.org/8819025/diff/4001/chrome/browser/ui/views/sele...
File chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc
(right):

http://codereview.chromium.org/8819025/diff/4001/chrome/browser/ui/views/sele...
chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc:223:
ASSERT_TRUE(downloads_dir.Set(tmp_dir));
On 2011/12/06 17:42:02, zel wrote:
> On 2011/12/06 17:29:29, zel wrote:
> > I suggest something like this instead:
> > 
> > ScopedTempDir downloads_dir;
> > ASSERT_TRUE(downloads_dir.CreateUniqueTempDirUnderPath("/tmp"));
> > FilePath dir_path = downloads_dir.Take();    // get path
> > ASSERT_TRUE(downloads_dir.Set(dir_path));    // Reclaim ownership
> > ASSERT_TRUE(file_util::CreateDirectory(dir_path.Append("Downloads")));   //
> > create Downloads sub dir
> > 
> > This will make your code more resilient to random stuff that might still
> reside
> > in /tmp/Downloads after test runs.
> > 
> 
> Shorter version:
> 
> ScopedTempDir downloads_dir;
> ASSERT_TRUE(downloads_dir.CreateUniqueTempDirUnderPath("/tmp"));
>
ASSERT_TRUE(file_util::CreateDirectory(downloads_dir.path().Append("Downloads")));

Done.

http://codereview.chromium.org/8819025/diff/4001/chrome/browser/ui/views/sele...
chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc:251:
FilePath tmp_dir("/tmp/Downloads");
On 2011/12/06 17:23:59, sky wrote:
> You want PathService::Get(base::DIR_TEMP, &tmp_path);

Done.

[sky, 2011-12-07 16:39:16]

http://codereview.chromium.org/8819025/diff/8001/chrome/browser/ui/views/sele...
File chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc
(right):

http://codereview.chromium.org/8819025/diff/8001/chrome/browser/ui/views/sele...
chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc:254:
FilePath tmp_dir;
How about a little refactoring so this code isn't duplicated so many times? If
all tests need it, put it in Setup.

[sky, 2011-12-07 16:54:02]

LGTM with the understanding you're going to come back to this soon and refactor
it.

[commit-bot: I haz the power, 2011-12-07 17:03:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgozman@chromium.org/8819025/8001

[commit-bot: I haz the power, 2011-12-07 17:03:32]

Presubmit check for 8819025-8001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
webkit/chromeos/fileapi/cros_mount_point_provider.cc

Presubmit checks took 1.3s to calculate.

[zel, 2011-12-07 19:37:55]

LGTM

http://codereview.chromium.org/8819025/diff/8001/chrome/browser/ui/views/sele...
File chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc
(right):

http://codereview.chromium.org/8819025/diff/8001/chrome/browser/ui/views/sele...
chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc:254:
FilePath tmp_dir;
On 2011/12/07 16:39:16, sky wrote:
> How about a little refactoring so this code isn't duplicated so many times? If
> all tests need it, put it in Setup.

Right, please move this to the test fixture instead since all tests need it.

[dgozman, 2011-12-08 04:51:36]

Darin, Tony,

Please, approve this CL (you are in OWNERS for webkit).

Thanks,
Dmitry

[commit-bot: I haz the power, 2011-12-08 04:54:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgozman@chromium.org/8819025/8001

[commit-bot: I haz the power, 2011-12-08 04:54:44]

Presubmit check for 8819025-8001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
webkit/chromeos/fileapi/cros_mount_point_provider.cc

Presubmit checks took 1.4s to calculate.

[tbarzic, 2011-12-08 05:03:00]

On 2011/12/08 04:54:44, I haz the power (commit-bot) wrote:
> Presubmit check for 8819025-8001 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for:
> webkit/chromeos/fileapi/cros_mount_point_provider.cc
> 
> Presubmit checks took 1.4s to calculate.

You probably ment to add tony@chromium.org to approve, not me :)

[dgozman, 2011-12-08 07:01:36]

Tony, please approve this CL (you are in OWNERS for webkit). Thanks!

[tony, 2011-12-08 17:49:25]

LGTM.  You probably want to add an OWNERS file to src/webkit/chromeos/fileapi.

[commit-bot: I haz the power, 2011-12-08 21:08:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgozman@chromium.org/8819025/8001

[zel, 2011-12-08 23:05:47]

yep, I am planning to do this right now


On Thu, Dec 8, 2011 at 9:49 AM, <tony@chromium.org> wrote:

> LGTM.  You probably want to add an OWNERS file to
> src/webkit/chromeos/fileapi.
>
>
http://codereview.chromium.**org/8819025/<http://codereview.chromium.org/8819...
>

[commit-bot: I haz the power, 2011-12-08 23:56:39]

Change committed as 113701