First step of porting VisitedLinkMaster to POSIX:

First step of porting VisitedLinkMaster to POSIX:
* Use POSIX file access APIs rather than HANDLEs.
* Add stubs so that VisitedLinkMaster compiles on POSIX.

Still to be done:
* Bring up Surrounding infrastructure to turn on unit tests.

[agl, 2009-01-23 00:22:39]

http://codereview.chromium.org/18530/diff/201/203
File base/scoped_file.h (right):

http://codereview.chromium.org/18530/diff/201/203#newcode22
Line 22: typedef scoped_ptr_malloc<FILE, ScopedFILEClose> ScopedFILE;
This is neat, but I worry that any error messages will confuse the heck out of
people. However, I'd be temped to leave it anyway. Just say'n.

http://codereview.chromium.org/18530/diff/201/205
File chrome/browser/visitedlink_master.cc (right):

http://codereview.chromium.org/18530/diff/201/205#newcode108
Line 108: int32 offset,
off_t here?

http://codereview.chromium.org/18530/diff/201/205#newcode115
Line 115: return  num_written == data_len;
extra space after return

http://codereview.chromium.org/18530/diff/201/205#newcode120
Line 120: FILE *file_;
star on the left

http://codereview.chromium.org/18530/diff/201/205#newcode141
Line 141: FILE *file_;
star on the left (SOTL)

http://codereview.chromium.org/18530/diff/201/205#newcode156
Line 156: FILE *file_;
SOTL

http://codereview.chromium.org/18530/diff/201/205#newcode556
Line 556: #if defined(OS_WIN)
TODO(port): make a header in base to get platform specific stuff right in a
generic way

http://codereview.chromium.org/18530/diff/201/205#newcode664
Line 664: size_t file_size = ftell(file);
Don't you want to seek back to the location which the file was at before?

http://codereview.chromium.org/18530/diff/201/205#newcode1002
Line 1002: int32 offset,
Again, maybe off_t here?

http://codereview.chromium.org/18530/diff/201/206
File chrome/browser/visitedlink_master.h (right):

http://codereview.chromium.org/18530/diff/201/206#newcode18
Line 18: #if !defined(OS_POSIX)
OS_WIN is clearer here, it's easy to miss the !

http://codereview.chromium.org/18530/diff/201/206#newcode340
Line 340: FILE *file_;
FILE* (star on the left)

[jeremy, 2009-01-23 17:38:06]

all fixed, new version uploaded.

http://codereview.chromium.org/18530/diff/201/203
File base/scoped_file.h (right):

http://codereview.chromium.org/18530/diff/201/203#newcode22
Line 22: typedef scoped_ptr_malloc<FILE, ScopedFILEClose> ScopedFILE;
I think the advantages outweigh any confusion here.

I want to create another one that wraps close() and redo some of the IPC code,
should make it much less error prone.

http://codereview.chromium.org/18530/diff/201/205
File chrome/browser/visitedlink_master.cc (right):

http://codereview.chromium.org/18530/diff/201/205#newcode108
Line 108: int32 offset,
On 2009/01/23 00:22:40, agl wrote:
> off_t here?

Done.

http://codereview.chromium.org/18530/diff/201/205#newcode115
Line 115: return  num_written == data_len;
On 2009/01/23 00:22:40, agl wrote:
> extra space after return

Done.

http://codereview.chromium.org/18530/diff/201/205#newcode120
Line 120: FILE *file_;
On 2009/01/23 00:22:40, agl wrote:
> star on the left

Done.

http://codereview.chromium.org/18530/diff/201/205#newcode141
Line 141: FILE *file_;
On 2009/01/23 00:22:40, agl wrote:
> star on the left (SOTL)

Done.

http://codereview.chromium.org/18530/diff/201/205#newcode156
Line 156: FILE *file_;
On 2009/01/23 00:22:40, agl wrote:
> SOTL

Done.

http://codereview.chromium.org/18530/diff/201/205#newcode556
Line 556: #if defined(OS_WIN)
On 2009/01/23 00:22:40, agl wrote:
> TODO(port): make a header in base to get platform specific stuff right in a
> generic way

Done.

http://codereview.chromium.org/18530/diff/201/205#newcode664
Line 664: size_t file_size = ftell(file);
Added explanatory comment.

http://codereview.chromium.org/18530/diff/201/205#newcode1002
Line 1002: int32 offset,
On 2009/01/23 00:22:40, agl wrote:
> Again, maybe off_t here?

Done.

http://codereview.chromium.org/18530/diff/201/206
File chrome/browser/visitedlink_master.h (right):

http://codereview.chromium.org/18530/diff/201/206#newcode18
Line 18: #if !defined(OS_POSIX)
On 2009/01/23 00:22:40, agl wrote:
> OS_WIN is clearer here, it's easy to miss the !

Done.

http://codereview.chromium.org/18530/diff/201/206#newcode340
Line 340: FILE *file_;
On 2009/01/23 00:22:40, agl wrote:
> FILE* (star on the left)

Done.

[agl, 2009-01-23 17:42:13]

LGTM. Sorry about the "star on the left" comments; Evan gets me with these every
time and I just want to pass the pain around ;)

[brettw, 2009-01-23 19:03:25]

http://codereview.chromium.org/18530/diff/212/214
File base/scoped_file.h (right):

http://codereview.chromium.org/18530/diff/212/214#newcode22
Line 22: typedef scoped_ptr_malloc<FILE, ScopedFILEClose> ScopedFILE;
Would this make sense in file_util.h?

http://codereview.chromium.org/18530/diff/212/216
File chrome/browser/visitedlink_master.cc (right):

http://codereview.chromium.org/18530/diff/212/216#newcode27
Line 27: // TODO(port): remove
It's not clear what this means. Remove the history includes or remove the if
!defined? I assume you mean the latter? Should this be OS_WIN instead?

http://codereview.chromium.org/18530/diff/212/216#newcode70
Line 70: void TruncateFile(FILE* file) {
It seems like this would fit well in file_util.

http://codereview.chromium.org/18530/diff/212/216#newcode114
Line 114: return  num_written == data_len;
Remove extra space.

http://codereview.chromium.org/18530/diff/212/216#newcode557
Line 557: fd = _wfopen(filename.value().c_str(), L"wb+");
Use OpenFile in file_util to avoid the ifdefs.

http://codereview.chromium.org/18530/diff/212/216#newcode604
Line 604: fd = _wfopen(filename.value().c_str(), L"rb+");
Ditto above with OpenFile.

[jeremy, 2009-01-23 22:31:06]

All fixed.

http://codereview.chromium.org/18530/diff/212/214
File base/scoped_file.h (right):

http://codereview.chromium.org/18530/diff/212/214#newcode22
Line 22: typedef scoped_ptr_malloc<FILE, ScopedFILEClose> ScopedFILE;
On 2009/01/23 19:03:25, brettw wrote:
> Would this make sense in file_util.h?

Done.

http://codereview.chromium.org/18530/diff/212/214#newcode22
Line 22: typedef scoped_ptr_malloc<FILE, ScopedFILEClose> ScopedFILE;
On 2009/01/23 19:03:25, brettw wrote:
> Would this make sense in file_util.h?

Done.

http://codereview.chromium.org/18530/diff/212/216
File chrome/browser/visitedlink_master.cc (right):

http://codereview.chromium.org/18530/diff/212/216#newcode27
Line 27: // TODO(port): remove
On 2009/01/23 19:03:25, brettw wrote:
> It's not clear what this means. Remove the history includes or remove the if
> !defined? I assume you mean the latter? Should this be OS_WIN instead?

Done.

http://codereview.chromium.org/18530/diff/212/216#newcode70
Line 70: void TruncateFile(FILE* file) {
On 2009/01/23 19:03:25, brettw wrote:
> It seems like this would fit well in file_util.

Done.

http://codereview.chromium.org/18530/diff/212/216#newcode114
Line 114: return  num_written == data_len;
On 2009/01/23 19:03:25, brettw wrote:
> Remove extra space.

Done.

http://codereview.chromium.org/18530/diff/212/216#newcode557
Line 557: fd = _wfopen(filename.value().c_str(), L"wb+");
Neat, thanks for the pointer!

http://codereview.chromium.org/18530/diff/212/216#newcode604
Line 604: fd = _wfopen(filename.value().c_str(), L"rb+");
On 2009/01/23 19:03:25, brettw wrote:
> Ditto above with OpenFile.

Done.

[brettw, 2009-01-23 22:53:44]

LGTM

[John Grabowski, 2009-01-26 19:31:08]

http://codereview.chromium.org/18530/diff/226/229
File base/file_util.h (right):

http://codereview.chromium.org/18530/diff/226/229#newcode324
Line 324: bool TruncateFile(FILE* file);
This might be misnamed.  For example:
  FILE *fp = fopen("/tmp/foo.txt", "w");
  fseek(fp, 500, SEEK_SET);
  file_util::TruncateFile(fp);
Will cause the file to be EXTENDED, not truncated.  
Perhaps that doesn't matter because the POSIX call, ftruncate(), is also
misnamed.