Fix to use FilePath in more unittests....

Fix to use FilePath in more unittests.

BUG=None
TEST=run unit_tests.exe, base_unittests.exe.

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

[tfarina (gmail-do not use), 2009-09-04 21:44:48]

Ping

[Paweł Hajdan Jr., 2009-09-04 21:52:46]

http://codereview.chromium.org/187005/diff/1/2
File app/gfx/icon_util_unittest.cc (right):

http://codereview.chromium.org/187005/diff/1/2#newcode16
Line 16: static const FilePath::CharType kSmallIconName[] = FILE_PATH_LITERAL(
While you're changing this code, please get rid of the file separator in the
constants.

http://codereview.chromium.org/187005/diff/1/5
File chrome/browser/browser_focus_uitest.cc (right):

http://codereview.chromium.org/187005/diff/1/5#newcode51
Line 51: const FilePath::CharType kTypicalPageName[] = FILE_PATH_LITERAL(
Should be char* and then AppendASCII should be used.

http://codereview.chromium.org/187005/diff/1/6
File chrome/browser/importer/importer_unittest.cc (right):

http://codereview.chromium.org/187005/diff/1/6#newcode40
Line 40: CreateDirectory(test_path_.value().c_str(), NULL);
Doesn't CreateDirectory work with FilePaths?

http://codereview.chromium.org/187005/diff/1/6#newcode42
Line 42: profile_path_ = profile_path_.AppendASCII("profile");
You can write test_path_.AppendASCII("profile") and get rid of the line above.

http://codereview.chromium.org/187005/diff/1/6#newcode43
Line 43: CreateDirectory(profile_path_.value().c_str(), NULL);
Same here.

http://codereview.chromium.org/187005/diff/1/6#newcode45
Line 45: app_path_ = app_path_.AppendASCII("app");
You can write test_path_.AppendASCII("app") and get rid of the line above.

http://codereview.chromium.org/187005/diff/1/6#newcode46
Line 46: CreateDirectory(app_path_.value().c_str(), NULL);
Same here.

http://codereview.chromium.org/187005/diff/1/6#newcode61
Line 61: data_path = data_path.AppendASCII(WideToASCII(profile_dir + L"\\*"));
Such mixing is even worse. Please convert everything involved to FilePaths.

http://codereview.chromium.org/187005/diff/1/6#newcode323
Line 323: CreateDirectory((path_value + L"\\SubFolder").c_str(), NULL);
Use FilePath and not a concatenation like here.

http://codereview.chromium.org/187005/diff/1/6#newcode341
Line 341: file_util::WriteFile(path_value + L"\\InvalidUrlFile.url", "x", 1);
Use FilePath's Append to concatenate.

http://codereview.chromium.org/187005/diff/1/6#newcode636
Line 636: data_path = data_path.AppendASCII("firefox2_profile\\*");
This \\* part being Appended to FilePath seems bad. Is there a better way?

http://codereview.chromium.org/187005/diff/1/10
File chrome/installer/setup/setup_util_unittest.cc (right):

http://codereview.chromium.org/187005/diff/1/10#newcode139
Line 139: chrome_dir = chrome_dir.AppendASCII("1.0.0.0");
Initialize chrome_dir with test_dir_.AppendASCII(...) and get rid of this line.

http://codereview.chromium.org/187005/diff/1/10#newcode140
Line 140: CreateDirectory(chrome_dir.value().c_str(), NULL);
Doesn't CreateDirectory accept FilePath?

http://codereview.chromium.org/187005/diff/1/10#newcode151
Line 151: chrome_dir = chrome_dir.AppendASCII("ABC");
Like above.

http://codereview.chromium.org/187005/diff/1/10#newcode157
Line 157: chrome_dir = chrome_dir.AppendASCII("2.3.4.5");
Like above.

[tfarina (gmail-do not use), 2009-09-04 23:01:24]

http://codereview.chromium.org/187005/diff/1/6
File chrome/browser/importer/importer_unittest.cc (right):

http://codereview.chromium.org/187005/diff/1/6#newcode40
Line 40: CreateDirectory(test_path_.value().c_str(), NULL);
On 2009/09/04 21:52:46, Paweł Hajdan Jr. wrote:
> Doesn't CreateDirectory work with FilePaths?
This functions is from windows(win32) api. So should I use
file_util::CreateDirectory instead?

[Paweł Hajdan Jr., 2009-09-04 23:03:01]

I think yes.

On Fri, Sep 4, 2009 at 16:01, <thiago.farina@gmail.com> wrote:

>
> http://codereview.chromium.org/187005/diff/1/6
> File chrome/browser/importer/importer_unittest.cc (right):
>
> http://codereview.chromium.org/187005/diff/1/6#newcode40
> Line 40: CreateDirectory(test_path_.value().c_str(), NULL);
> On 2009/09/04 21:52:46, Pawe=C5=82 Hajdan Jr. wrote:
>
>> Doesn't CreateDirectory work with FilePaths?
>>
> This functions is from windows(win32) api. So should I use
> file_util::CreateDirectory instead?
>
>
> http://codereview.chromium.org/187005
>

[tfarina (gmail-do not use), 2009-09-05 03:12:32]

http://codereview.chromium.org/187005/diff/1/5
File chrome/browser/browser_focus_uitest.cc (right):

http://codereview.chromium.org/187005/diff/1/5#newcode51
Line 51: const FilePath::CharType kTypicalPageName[] = FILE_PATH_LITERAL(
On 2009/09/04 21:52:46, Paweł Hajdan Jr. wrote:
> Should be char* and then AppendASCII should be used.

Done.

http://codereview.chromium.org/187005/diff/1/6
File chrome/browser/importer/importer_unittest.cc (right):

http://codereview.chromium.org/187005/diff/1/6#newcode42
Line 42: profile_path_ = profile_path_.AppendASCII("profile");
On 2009/09/04 21:52:46, Paweł Hajdan Jr. wrote:
> You can write test_path_.AppendASCII("profile") and get rid of the line above.

Done.

http://codereview.chromium.org/187005/diff/1/6#newcode43
Line 43: CreateDirectory(profile_path_.value().c_str(), NULL);
On 2009/09/04 21:52:46, Paweł Hajdan Jr. wrote:
> Same here.

Done.

http://codereview.chromium.org/187005/diff/1/6#newcode45
Line 45: app_path_ = app_path_.AppendASCII("app");
On 2009/09/04 21:52:46, Paweł Hajdan Jr. wrote:
> You can write test_path_.AppendASCII("app") and get rid of the line above.

Done.

http://codereview.chromium.org/187005/diff/1/6#newcode46
Line 46: CreateDirectory(app_path_.value().c_str(), NULL);
On 2009/09/04 21:52:46, Paweł Hajdan Jr. wrote:
> Same here.

Done.

http://codereview.chromium.org/187005/diff/1/6#newcode61
Line 61: data_path = data_path.AppendASCII(WideToASCII(profile_dir + L"\\*"));
On 2009/09/04 21:52:46, Paweł Hajdan Jr. wrote:
> Such mixing is even worse. Please convert everything involved to FilePaths.

Done.

http://codereview.chromium.org/187005/diff/1/6#newcode341
Line 341: file_util::WriteFile(path_value + L"\\InvalidUrlFile.url", "x", 1);
On 2009/09/04 21:52:46, Paweł Hajdan Jr. wrote:
> Use FilePath's Append to concatenate.

Done.

http://codereview.chromium.org/187005/diff/1/6#newcode636
Line 636: data_path = data_path.AppendASCII("firefox2_profile\\*");
On 2009/09/04 21:52:46, Paweł Hajdan Jr. wrote:
> This \\* part being Appended to FilePath seems bad. Is there a better way?

Done.

http://codereview.chromium.org/187005/diff/1/10
File chrome/installer/setup/setup_util_unittest.cc (right):

http://codereview.chromium.org/187005/diff/1/10#newcode139
Line 139: chrome_dir = chrome_dir.AppendASCII("1.0.0.0");
On 2009/09/04 21:52:46, Paweł Hajdan Jr. wrote:
> Initialize chrome_dir with test_dir_.AppendASCII(...) and get rid of this
line.

Done.

http://codereview.chromium.org/187005/diff/1/10#newcode140
Line 140: CreateDirectory(chrome_dir.value().c_str(), NULL);
On 2009/09/04 21:52:46, Paweł Hajdan Jr. wrote:
> Doesn't CreateDirectory accept FilePath?

Done.

http://codereview.chromium.org/187005/diff/1/10#newcode151
Line 151: chrome_dir = chrome_dir.AppendASCII("ABC");
On 2009/09/04 21:52:46, Paweł Hajdan Jr. wrote:
> Like above.

Done.

http://codereview.chromium.org/187005/diff/1/10#newcode157
Line 157: chrome_dir = chrome_dir.AppendASCII("2.3.4.5");
On 2009/09/04 21:52:46, Paweł Hajdan Jr. wrote:
> Like above.

Done.

[Paweł Hajdan Jr., 2009-09-08 16:19:43]

I think it may be better to extract the good changes to a separate patch. The
things I commented about here may be rather hard to fix without changing
FilePath and/or other functions a little bit.

http://codereview.chromium.org/187005/diff/5002/5003
File app/gfx/icon_util_unittest.cc (right):

http://codereview.chromium.org/187005/diff/5002/5003#newcode17
Line 17: const FilePath::StringType kPathSeparator =
FilePath::StringType().append(1,
nit: The line break should be rather after the "=".

And this is creating a global static object - that's bad.

http://codereview.chromium.org/187005/diff/5002/5003#newcode19
Line 19: static const FilePath::StringType kSmallIconName = FPL("icon_util") +
Would it be possible to use FilePath's Append here?

http://codereview.chromium.org/187005/diff/5002/5007
File chrome/browser/importer/importer_unittest.cc (right):

http://codereview.chromium.org/187005/diff/5002/5007#newcode328
Line 328: ASSERT_TRUE(CreateUrlFile(path_value + L"\\Google Home Page.url",
This should use FilePath's Append concatenation and not + L"\\"

http://codereview.chromium.org/187005/diff/5002/5007#newcode639
Line 639: data_path =
data_path.AppendASCII("firefox2_profile").AppendASCII("*");
AppendASCII("*") doesn't look correct to me.

[tfarina (gmail-do not use), 2009-09-08 22:03:56]

On 2009/09/08 16:19:43, Paweł Hajdan Jr. wrote:
> I think it may be better to extract the good changes to a separate patch. The
> things I commented about here may be rather hard to fix without changing
> FilePath and/or other functions a little bit.
If I understand correct, to keep the good changes here, I have to remove the
following files from this CL, no?

app/gfx/icon_util_unittest.cc 
chrome/browser/importer/importer_unittest.cc

And then make another CL with the changes necessary in these files, right?

[tfarina (gmail-do not use), 2009-09-12 18:59:37]

Ping

[Paweł Hajdan Jr., 2009-09-14 16:34:50]

LGTM, I'll handle landing this. Sent to try servers.

[tfarina (gmail-do not use), 2009-09-14 18:09:11]

On 2009/09/14 16:34:50, Paweł Hajdan Jr. wrote:
> LGTM, I'll handle landing this. Sent to try servers.
Thanks Pawel, fixed the compilation erros of linux/mac.

[Paweł Hajdan Jr., 2009-09-14 18:37:41]

Sent to the trybot again, good luck!

On Mon, Sep 14, 2009 at 11:09, <thiago.farina@gmail.com> wrote:

> On 2009/09/14 16:34:50, Pawe=C5=82 Hajdan Jr. wrote:
>
>> LGTM, I'll handle landing this. Sent to try servers.
>>
> Thanks Pawel, fixed the compilation erros of linux/mac.
>
>
>
> http://codereview.chromium.org/187005
>