Fix to use FilePath version of PathService::Get.

Fix to use FilePath version of PathService::Get.

BUG=None
TEST=None

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

[Paweł Hajdan Jr., 2009-09-02 15:37:47]

Thanks. Just a few nits.

http://codereview.chromium.org/184006/diff/1/2
File app/resource_bundle_win.cc (right):

http://codereview.chromium.org/184006/diff/1/2#newcode83
Line 83: GetDataDllLoadFlags());
nit: Align this line with the one above.

http://codereview.chromium.org/184006/diff/1/9
File chrome/browser/blocked_popup_container_interactive_uitest.cc (right):

http://codereview.chromium.org/184006/diff/1/9#newcode40
Line 40: void NavigateMainTabTo(const FilePath::StringType& file_name) {
nit: I'd prefer to see std::string here, and AppendASCII later. Simplifies the
usage of this function.

http://codereview.chromium.org/184006/diff/1/14
File chrome/browser/search_engines/template_url_parser_unittest.cc (right):

http://codereview.chromium.org/184006/diff/1/14#newcode36
Line 36: void ParseFile(const FilePath::StringType& file_name,
nit: Better to use std::string here, and AppendASCII. Less FILE_PATH_LITERALs
later.

[tfarina (gmail-do not use), 2009-09-02 17:39:46]

Thanks for the suggestion about AppendASCII, much better than have to write
FILE_PATH_LITERAL every time that function needs to be called.

http://codereview.chromium.org/184006/diff/1/2
File app/resource_bundle_win.cc (right):

http://codereview.chromium.org/184006/diff/1/2#newcode83
Line 83: GetDataDllLoadFlags());
On 2009/09/02 15:37:48, Paweł Hajdan Jr. wrote:
> nit: Align this line with the one above.

Done.

http://codereview.chromium.org/184006/diff/1/9
File chrome/browser/blocked_popup_container_interactive_uitest.cc (right):

http://codereview.chromium.org/184006/diff/1/9#newcode40
Line 40: void NavigateMainTabTo(const FilePath::StringType& file_name) {
On 2009/09/02 15:37:48, Paweł Hajdan Jr. wrote:
> nit: I'd prefer to see std::string here, and AppendASCII later. Simplifies the
> usage of this function.

Done.

http://codereview.chromium.org/184006/diff/1/14
File chrome/browser/search_engines/template_url_parser_unittest.cc (right):

http://codereview.chromium.org/184006/diff/1/14#newcode36
Line 36: void ParseFile(const FilePath::StringType& file_name,
On 2009/09/02 15:37:48, Paweł Hajdan Jr. wrote:
> nit: Better to use std::string here, and AppendASCII. Less FILE_PATH_LITERALs
> later.

Done.

[Paweł Hajdan Jr., 2009-09-08 16:30:49]

LGTM.

thestig, could you take a look too?

[Lei Zhang, 2009-09-08 18:29:18]

Mostly LGTM, some nits below.

http://codereview.chromium.org/184006/diff/4001/3006
File base/file_version_info_unittest.cc (right):

http://codereview.chromium.org/184006/diff/4001/3006#newcode30
Line 30: FILE_PATH_LITERAL("FileVersionInfoTest1.dll")
This is inside #ifdef OS_WIN, FilePath::CharType will always be wstring. You can
either (a) not make this change and use Append() below, or (b) make this ASCII
and use AppendASCII() below.

http://codereview.chromium.org/184006/diff/4001/3006#newcode83
Line 83: FILE_PATH_LITERAL("FileVersionInfoTest1.dll"),
ditto, see above comment.

http://codereview.chromium.org/184006/diff/4001/3006#newcode109
Line 109: dll_path =
dll_path.Append(FILE_PATH_LITERAL("FileVersionInfoTest1.dll"));
Easier to write AppendASCII("foo") ?

http://codereview.chromium.org/184006/diff/4001/3010
File chrome/browser/importer/safari_importer_unittest.mm (right):

http://codereview.chromium.org/184006/diff/4001/3010#newcode24
Line 24: test_dir = test_dir.AppendASCII("safari_import");
This is in a .mm file, This change is unnecessary, but ok.

http://codereview.chromium.org/184006/diff/4001/3015
File chrome/common/json_value_serializer_perftest.cc (right):

http://codereview.chromium.org/184006/diff/4001/3015#newcode21
Line 21: static const FilePath::CharType* const kTestFilenames[] = {
I think it's easier to change this to char* and use AppendASCII() below.

[tfarina (gmail-do not use), 2009-09-08 21:11:07]

http://codereview.chromium.org/184006/diff/4001/3006
File base/file_version_info_unittest.cc (right):

http://codereview.chromium.org/184006/diff/4001/3006#newcode30
Line 30: FILE_PATH_LITERAL("FileVersionInfoTest1.dll")
On 2009/09/08 18:29:18, Lei Zhang wrote:
> This is inside #ifdef OS_WIN, FilePath::CharType will always be wstring. You
can
> either (a) not make this change and use Append() below, or (b) make this ASCII
> and use AppendASCII() below.
Right, I stayed with wchar_t* in this case and then Append().

http://codereview.chromium.org/184006/diff/4001/3006#newcode83
Line 83: FILE_PATH_LITERAL("FileVersionInfoTest1.dll"),
On 2009/09/08 18:29:18, Lei Zhang wrote:
> ditto, see above comment.
Same as above.

http://codereview.chromium.org/184006/diff/4001/3006#newcode109
Line 109: dll_path =
dll_path.Append(FILE_PATH_LITERAL("FileVersionInfoTest1.dll"));
On 2009/09/08 18:29:18, Lei Zhang wrote:
> Easier to write AppendASCII("foo") ?
Yeah

http://codereview.chromium.org/184006/diff/4001/3015
File chrome/common/json_value_serializer_perftest.cc (right):

http://codereview.chromium.org/184006/diff/4001/3015#newcode21
Line 21: static const FilePath::CharType* const kTestFilenames[] = {
On 2009/09/08 18:29:18, Lei Zhang wrote:
> I think it's easier to change this to char* and use AppendASCII() below.
Thanks, fixed.

[Lei Zhang, 2009-09-08 21:49:07]

LGTM