Cross-platform equivalent of fopen, _wfopen_s etc....

Cross-platform equivalent of fopen, _wfopen_s etc.

This is refactoring needed for http://codereview.chromium.org/4247
(porting in chrome/browser/ ).

BUG=2333

[please use my chromium address, 2008-09-30 09:25:52]

Please check if anyone else should review this. As with rest of my issues, I
couldn't test on Windows. :-/

If I can make it easier to review by better splitting or something else, I will
gladly do it.

[Dean McNamee, 2008-09-30 13:32:19]

I didn't look for anything to nit for now, just a few design thoughts.  Overall
I think this is pretty good, thanks.  I am worried about the difference in modes
on platforms, maybe we can just filter our "b", or maybe it's fine to pass it
and it is ignored?

http://codereview.chromium.org/6005/diff/1/4
File base/file_util.h (right):

http://codereview.chromium.org/6005/diff/1/4#newcode252
Line 252: bool OpenFile(FILE** file, const std::string& filename, const char*
mode);
Why return a bool, and not just a FILE*, and have the caller NULL check it?

http://codereview.chromium.org/6005/diff/1/4#newcode253
Line 253: bool OpenFile(FILE** file, const std::wstring& filename, const char*
mode);
I think it's weird to mix OpenFile with fclose.  We should make CloseFile pass
through to fclose, just to make everything look consistent.

http://codereview.chromium.org/6005/diff/1/6
File base/file_util_posix.cc (right):

http://codereview.chromium.org/6005/diff/1/6#newcode315
Line 315: return *file;
In general I think maybe it's a good habit not to overwrite arguments on
failure. So this would be better as:

FILE* res = fopen(...);
if (res) {
  *file = res;
  return true;
}
return false;

However, I think we should just return the FILE* ?

http://codereview.chromium.org/6005/diff/1/2
File base/file_util_win.cc (right):

http://codereview.chromium.org/6005/diff/1/2#newcode452
Line 452: return fopen_s(file, filename.c_str(), w_mode.c_str()) == 0;
The Windows API is stupid.  We should just return NULL, even though they do the
return success business.

http://codereview.chromium.org/6005/diff/1/5
File base/logging.cc (right):

http://codereview.chromium.org/6005/diff/1/5#newcode477
Line 477: fclose(log_file);
file_util::CloseFile

http://codereview.chromium.org/6005/diff/1/16
File chrome/browser/download/download_file.cc (right):

http://codereview.chromium.org/6005/diff/1/16#newcode110
Line 110: if (!Open("a+b"))
Does Posix have the "b" binary flag?  I didn't think so, so does it just ignored
it, or?

[please use my chromium address, 2008-09-30 14:33:50]

On 2008/09/30 13:32:19, Dean McNamee wrote:
> I didn't look for anything to nit for now, just a few design thoughts. 
Overall
> I think this is pretty good, thanks.  I am worried about the difference in
modes
> on platforms, maybe we can just filter our "b", or maybe it's fine to pass it
> and it is ignored?

Fortunately it's ignored:

The mode string can also include the letter 'b' either as a last character or as
a character between the  characters  in any of the two-character strings
described above.  This is strictly for compatibility with C89 and has no effect;
the 'b' is ignored on all  POSIX  conforming  systems,  including Linux.

> http://codereview.chromium.org/6005/diff/1/4
> File base/file_util.h (right):
> 
> http://codereview.chromium.org/6005/diff/1/4#newcode252
> Line 252: bool OpenFile(FILE** file, const std::string& filename, const char*
> mode);
> Why return a bool, and not just a FILE*, and have the caller NULL check it?

Probably no difference. Changed to returning FILE*.

> http://codereview.chromium.org/6005/diff/1/4#newcode253
> Line 253: bool OpenFile(FILE** file, const std::wstring& filename, const char*
> mode);
> I think it's weird to mix OpenFile with fclose.  We should make CloseFile pass
> through to fclose, just to make everything look consistent.

Done.

Rest of issues fixed as well.

[Dean McNamee, 2008-09-30 14:42:34]

Overall looks good.  I am concerned that maybe in logging.cc and trace_event.cc,
there was some reasons for avoiding FILE* and doing things low level.  Adding
Darin and Erik to comment on those.

[brettw, 2008-09-30 15:25:30]

http://codereview.chromium.org/6005/diff/226/29
File base/logging.cc (right):

http://codereview.chromium.org/6005/diff/226/29#newcode166
Line 166: log_file = file_util::OpenFile(*log_file_name, "a");
I'm a little worried about sharing. We should be able to have the file open in
an editor while it's being logged to. The editor should be able to open the file
if the process is writing it, and the process should be able to open the file if
the editor has it opened for writing. It doesn't look like this happens now from
what I can tell in the implementation of wfopen.

[Mark Mentovai, 2008-09-30 15:29:44]

This looks mostly good.

http://codereview.chromium.org/6005/diff/226/33
File base/event_recorder.cc (right):

http://codereview.chromium.org/6005/diff/226/33#newcode10
Line 10: #include "base/file_util.h"
Sort

http://codereview.chromium.org/6005/diff/226/31
File base/file_util.cc (right):

http://codereview.chromium.org/6005/diff/226/31#newcode8
Line 8: #include <stdio.h>
C headers come before C++ headers.

http://codereview.chromium.org/6005/diff/226/31#newcode273
Line 273: FILE* file = OpenFile(path, "r");
The existing Windows implementation used "rbS" as the mode.  S is a hint and so
if it's unportable we can ignore it, but b is significant on Windows.

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

http://codereview.chromium.org/6005/diff/226/28#newcode252
Line 252: FILE* OpenFile(const std::wstring& filename, const char* mode);
I don't think we should do the overload.  Let's just support the wstring one for
now, and when I finish PathString this will obviously move to that.

http://codereview.chromium.org/6005/diff/226/29
File base/logging.cc (left):

http://codereview.chromium.org/6005/diff/226/29#oldcode184
Line 184: log_file = CreateFile(log_file_name->c_str(), GENERIC_WRITE,
I'm not sure about moving from win32 to stdio calls here either, definitely ask
around.

At the very least, relying on file_util in logging means that we can't use
logging or anything else that uses logging in at least chunks of file_util. 
Chasing the dependencies to ensure that everything is safe can be cumbersome.

http://codereview.chromium.org/6005/diff/226/37
File chrome/browser/history/thumbnail_database.cc (right):

http://codereview.chromium.org/6005/diff/226/37#newcode108
Line 108: FILE* f = file_util::OpenFile(std::string(filename), "wb");
Should this just use file_util's WriteFile?

http://codereview.chromium.org/6005/diff/226/36
File chrome/browser/printing/printing_layout_uitest.cc (right):

http://codereview.chromium.org/6005/diff/226/36#newcode62
Line 62: FILE* f = file_util::OpenFile(filename, "wb");
file_util's WriteFile here too?  And maybe other cases I didn't catch?

[please use my chromium address, 2008-09-30 15:30:32]

On 2008/09/30 15:25:30, brettw wrote:
> http://codereview.chromium.org/6005/diff/226/29
> File base/logging.cc (right):
> 
> http://codereview.chromium.org/6005/diff/226/29#newcode166
> Line 166: log_file = file_util::OpenFile(*log_file_name, "a");
> I'm a little worried about sharing. We should be able to have the file open in
> an editor while it's being logged to. The editor should be able to open the
file
> if the process is writing it, and the process should be able to open the file
if
> the editor has it opened for writing. It doesn't look like this happens now
from
> what I can tell in the implementation of wfopen.

Should I leave the #ifdefs then and only change the POSIX side? Or do something
else?

[please use my chromium address, 2008-09-30 16:22:07]

On 2008/09/30 15:29:44, Mark Mentovai wrote:
> http://codereview.chromium.org/6005/diff/226/28#newcode252
> Line 252: FILE* OpenFile(const std::wstring& filename, const char* mode);
> I don't think we should do the overload.  Let's just support the wstring one
for
> now, and when I finish PathString this will obviously move to that.

This is a bit problematic, at least for me. There are some places where
std::string is used as filename type. There are significant differences in how
it's handled on Windows (wstring vs. string). I don't know ho to do that well
for now other than have two versions of OpenFile. If you have any tip, I would
gladly implement it.

> http://codereview.chromium.org/6005/diff/226/37
> File chrome/browser/history/thumbnail_database.cc (right):
> 
> http://codereview.chromium.org/6005/diff/226/37#newcode108
> Line 108: FILE* f = file_util::OpenFile(std::string(filename), "wb");
> Should this just use file_util's WriteFile?

Sure. Done.

> http://codereview.chromium.org/6005/diff/226/36
> File chrome/browser/printing/printing_layout_uitest.cc (right):
> 
> http://codereview.chromium.org/6005/diff/226/36#newcode62
> Line 62: FILE* f = file_util::OpenFile(filename, "wb");
> file_util's WriteFile here too?  And maybe other cases I didn't catch?

Done. I found at least one other case when WriteFile could be used, but it had
std::string instead of std::wstring...

Rest of issues fixed too; no changes in logging.cc per brettw's request (it even
makes things easier!).

[Dean McNamee, 2008-09-30 16:22:18]

Looks good enough for me.  I am fine with the OpenFile overloading until we have
a path class, but I'll let Mark decide on that one.

Thanks

http://codereview.chromium.org/6005/diff/275/88
File chrome/browser/printing/printing_layout_uitest.cc (right):

http://codereview.chromium.org/6005/diff/275/88#newcode63
Line 63: compressed.begin(),
compressed.data()

[please use my chromium address, 2008-09-30 16:40:55]

On 2008/09/30 16:22:18, Dean McNamee wrote:
> Looks good enough for me.  I am fine with the OpenFile overloading until we
have
> a path class, but I'll let Mark decide on that one.
> 
> Thanks
> 
> http://codereview.chromium.org/6005/diff/275/88
> File chrome/browser/printing/printing_layout_uitest.cc (right):
> 
> http://codereview.chromium.org/6005/diff/275/88#newcode63
> Line 63: compressed.begin(),
> compressed.data()

Done.

[please use my chromium address, 2008-10-01 15:43:31]

*ping* - please take a look.

[Dean McNamee, 2008-10-01 16:27:17]

You already have my LGTM.  I will leave it up to Mark, and he can commit the
patch.  Thanks!

On 2008/10/01 15:43:31, phajdan.jr wrote:
> *ping* - please take a look.

[Mark Mentovai, 2008-10-01 17:40:55]

Committed r2760 with the changes listed.  Please have a look.

http://codereview.chromium.org/6005/diff/279/119
File base/file_util_win.cc (right):

http://codereview.chromium.org/6005/diff/279/119#newcode451
Line 451: std::wstring w_mode = ASCIIToWide(std::string(mode));
Not for fopen_s, you can just pass mode directly.

http://codereview.chromium.org/6005/diff/279/124
File base/trace_event.cc (right):

http://codereview.chromium.org/6005/diff/279/124#newcode52
Line 52: Log("var raw_trace_events = [\r\n");
Followup patch to get rid of the \r or only emit it on Windows?

(Might be worth looking over the codebase for other \rs.)

http://codereview.chromium.org/6005/diff/279/124#newcode81
Line 81: file_util::CloseFile(log_file_);
log_file_ is declared as a FileHandle, which is not FILE* on Windows.  Fixing
that in trace_event.h.

http://codereview.chromium.org/6005/diff/279/128
File chrome/browser/printing/printing_layout_uitest.cc (right):

http://codereview.chromium.org/6005/diff/279/128#newcode63
Line 63: compressed.data(),
std::vector<> has no data(), I think you want &*compressed.begin() like what was
here before.

Also, because WriteFile takes a const char* parameter as opposed to fwrite's
void*, the vector's type is significant.  We need a cast somewhere because
PNGEncoder::Encode expects an unsigned char*, so I've casted the one in
WriteFile to char*.

[please use my chromium address, 2008-10-01 17:57:55]

On 2008/10/01 17:40:55, Mark Mentovai wrote:
> Committed r2760 with the changes listed.  Please have a look.

Thanks for fixing these last issues and commiting. Of course compiles fine and
passes tests here. After closing this review I will update
http://codereview.chromium.org/4247.

> http://codereview.chromium.org/6005/diff/279/124#newcode52
> Line 52: Log("var raw_trace_events = [\r\n");
> Followup patch to get rid of the \r or only emit it on Windows?
> 
> (Might be worth looking over the codebase for other \rs.)

I submitted http://code.google.com/p/chromium/issues/detail?id=3034 to handle
it. I can produce the patch, just have a few questions/doubts.