Port some more parts of browser/...

Port some more parts of browser/

This first needs http://codereview.chromium.org/6492
(porting base/process_util).

BUG=2333

[please use my chromium address, 2008-10-06 11:44:35]

sgk: In one browser/SConscript there is one more delete than add. This is
because safe_browsing_database.cc is already in the "upper" list.

[please use my chromium address, 2008-10-08 10:31:52]

Tree is now open. Please take a look.

[sgk, 2008-10-08 16:14:38]

SCons lgtm

[Evan Martin, 2008-10-14 23:08:45]

http://codereview.chromium.org/6268/diff/1/10
File chrome/browser/automation/url_request_failed_dns_job.cc (right):

http://codereview.chromium.org/6268/diff/1/10#newcode15
Line 15: L"http://url.handled.by.fake.dns/";
This can be made an 8-bit string to avoid conversions below.

http://codereview.chromium.org/6268/diff/1/9
File chrome/browser/automation/url_request_mock_http_job.cc (right):

http://codereview.chromium.org/6268/diff/1/9#newcode35
Line 35: net::FileURLToFilePath(GURL(WideToUTF8(file_url)), &file_path);
From talking with Brett I think the proper change here is to not use wstring for
URLs in the first place.  That also lets you avoid UTF8ToWide a few lines up.

http://codereview.chromium.org/6268/diff/1/9#newcode57
Line 57: return GURL(WideToUTF8(url));
Similar comment here -- a plain std::string would be better.

http://codereview.chromium.org/6268/diff/1/4
File chrome/browser/safe_browsing/safe_browsing_database_impl.cc (right):

http://codereview.chromium.org/6268/diff/1/4#newcode1180
Line 1180: hash_cache_.erase(it++);
It it safe to erase from something you're iterating through?

http://codereview.chromium.org/6268/diff/1/3
File chrome/browser/safe_browsing/safe_browsing_database_impl_unittest.cc
(right):

http://codereview.chromium.org/6268/diff/1/3#newcode91
Line 91: file_util::Delete(filename, false);  // In case it existed from a
previous run.
It appears this deletes all files within a directory.  Maybe that function
should be renamed...

[please use my chromium address, 2008-10-20 10:32:15]

http://codereview.chromium.org/6268/diff/1/10
File chrome/browser/automation/url_request_failed_dns_job.cc (right):

http://codereview.chromium.org/6268/diff/1/10#newcode15
Line 15: L"http://url.handled.by.fake.dns/";
On 2008/10/14 23:08:45, Evan Martin wrote:
> This can be made an 8-bit string to avoid conversions below.

Done.

http://codereview.chromium.org/6268/diff/1/9
File chrome/browser/automation/url_request_mock_http_job.cc (right):

http://codereview.chromium.org/6268/diff/1/9#newcode35
Line 35: net::FileURLToFilePath(GURL(WideToUTF8(file_url)), &file_path);
On 2008/10/14 23:08:45, Evan Martin wrote:
> From talking with Brett I think the proper change here is to not use wstring
for
> URLs in the first place.  That also lets you avoid UTF8ToWide a few lines up.

In this case it's not so simple, because base_path_ is wstring too (of course),
and it gets initialized from source that returns a wstring... It would be harder
to change, much more like a small refactoring.

http://codereview.chromium.org/6268/diff/1/9#newcode57
Line 57: return GURL(WideToUTF8(url));
On 2008/10/14 23:08:45, Evan Martin wrote:
> Similar comment here -- a plain std::string would be better.

Done. Please note that path is still a wstring.

http://codereview.chromium.org/6268/diff/1/4
File chrome/browser/safe_browsing/safe_browsing_database_impl.cc (right):

http://codereview.chromium.org/6268/diff/1/4#newcode1180
Line 1180: hash_cache_.erase(it++);
On 2008/10/14 23:08:45, Evan Martin wrote:
> It it safe to erase from something you're iterating through?

AFAIK in this case yes. Iterator gets incremented before the entry is erased.
And it is a hash_map.

One unit test touches this code (it gets executed) and doesn't crash. Not a
proof at all, but at least passes a "smoke test".

http://codereview.chromium.org/6268/diff/1/3
File chrome/browser/safe_browsing/safe_browsing_database_impl_unittest.cc
(right):

http://codereview.chromium.org/6268/diff/1/3#newcode91
Line 91: file_util::Delete(filename, false);  // In case it existed from a
previous run.
On 2008/10/14 23:08:45, Evan Martin wrote:
> It appears this deletes all files within a directory.  Maybe that function
> should be renamed...

Yes, if passed a directory file_util::Delete will delete it. Of course it
deletes files too. Do you mean a file_util::DeleteFile function?

[Evan Martin, 2008-10-20 19:43:32]

http://codereview.chromium.org/6268/diff/421/46
File chrome/browser/automation/url_request_failed_dns_job.cc (right):

http://codereview.chromium.org/6268/diff/421/46#newcode8
Line 8: #include "base/string_util.h"
Is this needed anymore?

http://codereview.chromium.org/6268/diff/421/41
File chrome/browser/safe_browsing/safe_browsing_database_unittest.cc (right):

http://codereview.chromium.org/6268/diff/421/41#newcode66
Line 66: file_util::Delete(filename, false);  // In case it existed from a
previous run.
Can you wrap this comment to 80 columns?

http://codereview.chromium.org/6268/diff/421/41#newcode499
Line 499: file_util::Delete(filename, false);  // In case it existed from a
previous run.
80 cols here too.  :\

http://codereview.chromium.org/6268/diff/421/41#newcode526
Line 526: after.ReadTransferCount - before.ReadTransferCount).c_str();
I'm surprised .c_str() is necessary here... ?

http://codereview.chromium.org/6268/diff/421/41#newcode615
Line 615: const wchar_t* GetOldSafeBrowsingPath() {
These file-level functions should be in the anonymous namespace.

You could also switch it to use FilePath (see base/file_path.h) but that's fine
to do in a different change (the rest of the file should use FilePath as
well...).

http://codereview.chromium.org/6268/diff/421/41#newcode642
Line 642: //
[1004/201323:FATAL:browser/safe_browsing/safe_browsing_database_impl.cc(712)]
I think the ifdef isn't necessary if they're failing at run-time, because the
DISABLED in the test name means they're disabled on Windows as well.  Is that
disabling not working on Linux?

http://codereview.chromium.org/6268/diff/421/41#newcode664
Line 664: #if defined(OS_WIN)
same here for the disabling

[please use my chromium address, 2008-10-21 05:29:37]

http://codereview.chromium.org/6268/diff/421/46
File chrome/browser/automation/url_request_failed_dns_job.cc (right):

http://codereview.chromium.org/6268/diff/421/46#newcode8
Line 8: #include "base/string_util.h"
On 2008/10/20 19:43:32, Evan Martin wrote:
> Is this needed anymore?

It seems that not. Removed.

http://codereview.chromium.org/6268/diff/421/41
File chrome/browser/safe_browsing/safe_browsing_database_unittest.cc (right):

http://codereview.chromium.org/6268/diff/421/41#newcode66
Line 66: file_util::Delete(filename, false);  // In case it existed from a
previous run.
On 2008/10/20 19:43:32, Evan Martin wrote:
> Can you wrap this comment to 80 columns?

Done.

http://codereview.chromium.org/6268/diff/421/41#newcode499
Line 499: file_util::Delete(filename, false);  // In case it existed from a
previous run.
On 2008/10/20 19:43:32, Evan Martin wrote:
> 80 cols here too.  :\

Done.

http://codereview.chromium.org/6268/diff/421/41#newcode526
Line 526: after.ReadTransferCount - before.ReadTransferCount).c_str();
On 2008/10/20 19:43:32, Evan Martin wrote:
> I'm surprised .c_str() is necessary here... ?

Indeed, can be removed. Done.

http://codereview.chromium.org/6268/diff/421/41#newcode642
Line 642: //
[1004/201323:FATAL:browser/safe_browsing/safe_browsing_database_impl.cc(712)]
On 2008/10/20 19:43:32, Evan Martin wrote:
> I think the ifdef isn't necessary if they're failing at run-time, because the
> DISABLED in the test name means they're disabled on Windows as well.  Is that
> disabling not working on Linux?

Disabling is working, I just wanted to emphasize the difference between working
and non-working tests. Now removed the #if defines.

http://codereview.chromium.org/6268/diff/421/41#newcode664
Line 664: #if defined(OS_WIN)
On 2008/10/20 19:43:32, Evan Martin wrote:
> same here for the disabling

Done.

[please use my chromium address, 2008-10-22 16:48:40]

*ping* - Please check whether everything's ok.

[Evan Martin, 2008-10-22 22:30:18]

looking now

[Evan Martin, 2008-10-23 21:32:16]

r3858 (sorry for the delay, having computer issues as usual)