Refactor BaseFile methods to return a DownloadInterruptReason instead of a net::Error.

Refactor BaseFile methods to return a DownloadInterruptReason instead of a net::Error.
Also split the implementation into platform specific files.

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

[Randy Smith (Not in Mondays), 2012-10-23 20:18:13]

http://codereview.chromium.org/11238044/diff/5001/content/browser/download/ba...
File content/browser/download/base_file.cc (right):

http://codereview.chromium.org/11238044/diff/5001/content/browser/download/ba...
content/browser/download/base_file.cc:169: if (new_path != full_path_) {
Why reverse the logic?  I'm used to "if (X) { Y; return; } Z" patterns to prefer
the uncommon pathway in the Y branch, and it seems like this would be the common
pathway.  (Another metric would be to put the fewer lines of code in Y, which
this doesn't follow either.)

http://codereview.chromium.org/11238044/diff/5001/content/browser/download/ba...
content/browser/download/base_file.cc:185:
bound_net_log_.EndEvent(net::NetLog::TYPE_DOWNLOAD_FILE_RENAMED);
Why remove the "if (!saved_in_progres) Close()" path from this branch?)

http://codereview.chromium.org/11238044/diff/5001/content/browser/download/ba...
content/browser/download/base_file.cc:335: bool BaseFile::SetHashState(const
std::string& hash_state_bytes) {
Suggestion (up to you):  This is only called from the constructor, and isn't
much code--would it make sense to pull it up into the constructor?

http://codereview.chromium.org/11238044/diff/5001/content/browser/download/ba...
File content/browser/download/base_file.h (right):

http://codereview.chromium.org/11238044/diff/5001/content/browser/download/ba...
content/browser/download/base_file.h:77: content::DownloadInterruptReason
AnnotateWithSourceInformation();
I don't object, but why?  Nothing appears to return an error.  If for a future
CL, I think it's better to have the change to returning an error code in that
CL.

http://codereview.chromium.org/11238044/diff/5001/content/browser/download/ba...
content/browser/download/base_file.h:136: const char* operation, int os_error,
Why associate the os_error logging with the DownloadInterruptReason logging and
not net::Error logging?

http://codereview.chromium.org/11238044/diff/5001/content/browser/download/ba...
File content/browser/download/base_file_posix.cc (right):

http://codereview.chromium.org/11238044/diff/5001/content/browser/download/ba...
content/browser/download/base_file_posix.cc:26: LogNetError("stat",
net::MapSystemError(errno));
Why not logging the OS error directly here?  Given that we can just avoid doing
the extra translation?

[asanka, 2012-10-23 21:53:34]

https://codereview.chromium.org/11238044/diff/5001/content/browser/download/b...
File content/browser/download/base_file.cc (right):

https://codereview.chromium.org/11238044/diff/5001/content/browser/download/b...
content/browser/download/base_file.cc:169: if (new_path != full_path_) {
On 2012/10/23 20:18:13, rdsmith wrote:
> Why reverse the logic?  I'm used to "if (X) { Y; return; } Z" patterns to
prefer
> the uncommon pathway in the Y branch, and it seems like this would be the
common
> pathway.  (Another metric would be to put the fewer lines of code in Y, which
> this doesn't follow either.)

This is to avoid having multiple return points since we have a
bound_net_log_.EndEvent() at the end. I moved the code around to skip the rename
entirely if the paths are the same.

Are the conventions you mention stated somewhere?

https://codereview.chromium.org/11238044/diff/5001/content/browser/download/b...
content/browser/download/base_file.cc:185:
bound_net_log_.EndEvent(net::NetLog::TYPE_DOWNLOAD_FILE_RENAMED);
On 2012/10/23 20:18:13, rdsmith wrote:
> Why remove the "if (!saved_in_progres) Close()" path from this branch?)

If saved_in_progress is false, then the file_stream_ was NULL to begin with.
Calling Close() would be a no-op.

https://codereview.chromium.org/11238044/diff/5001/content/browser/download/b...
content/browser/download/base_file.cc:335: bool BaseFile::SetHashState(const
std::string& hash_state_bytes) {
On 2012/10/23 20:18:13, rdsmith wrote:
> Suggestion (up to you):  This is only called from the constructor, and isn't
> much code--would it make sense to pull it up into the constructor?

Done.

https://codereview.chromium.org/11238044/diff/5001/content/browser/download/b...
File content/browser/download/base_file.h (right):

https://codereview.chromium.org/11238044/diff/5001/content/browser/download/b...
content/browser/download/base_file.h:77: content::DownloadInterruptReason
AnnotateWithSourceInformation();
On 2012/10/23 20:18:13, rdsmith wrote:
> I don't object, but why?  Nothing appears to return an error.  If for a future
> CL, I think it's better to have the change to returning an error code in that
> CL.

Done.

https://codereview.chromium.org/11238044/diff/5001/content/browser/download/b...
content/browser/download/base_file.h:136: const char* operation, int os_error,
On 2012/10/23 20:18:13, rdsmith wrote:
> Why associate the os_error logging with the DownloadInterruptReason logging
and
> not net::Error logging?

It's based on usage. When we have a net::Error we don't have an os_error.

I added a LogSystemError() method that's used when we only have a system error.

https://codereview.chromium.org/11238044/diff/5001/content/browser/download/b...
File content/browser/download/base_file_posix.cc (right):

https://codereview.chromium.org/11238044/diff/5001/content/browser/download/b...
content/browser/download/base_file_posix.cc:26: LogNetError("stat",
net::MapSystemError(errno));
On 2012/10/23 20:18:13, rdsmith wrote:
> Why not logging the OS error directly here?  Given that we can just avoid
doing
> the extra translation?
> 

We are logging the OS error as well now, although we still have an extra
conversion.

[Randy Smith (Not in Mondays), 2012-10-24 18:10:29]

LGTM.

http://codereview.chromium.org/11238044/diff/5001/content/browser/download/ba...
File content/browser/download/base_file.cc (right):

http://codereview.chromium.org/11238044/diff/5001/content/browser/download/ba...
content/browser/download/base_file.cc:169: if (new_path != full_path_) {
On 2012/10/23 21:53:34, asanka wrote:
> On 2012/10/23 20:18:13, rdsmith wrote:
> > Why reverse the logic?  I'm used to "if (X) { Y; return; } Z" patterns to
> prefer
> > the uncommon pathway in the Y branch, and it seems like this would be the
> common
> > pathway.  (Another metric would be to put the fewer lines of code in Y,
which
> > this doesn't follow either.)
> 
> This is to avoid having multiple return points since we have a
> bound_net_log_.EndEvent() at the end. 

Ah, that makes sense. 

> I moved the code around to skip the rename
> entirely if the paths are the same.

Cool; thanks.

> Are the conventions you mention stated somewhere?

I don't think so :-}.  It's my attempt to articulate my preferences, though I
think I've seen reviews where pkasting pushes similar preferences.

[asanka, 2012-10-24 20:30:13]

+jam for content/content_browser.gypi changes

[jam, 2012-10-24 20:49:00]

gypi lgtm, but a comment that i happened to see:

https://codereview.chromium.org/11238044/diff/8001/content/browser/download/b...
File content/browser/download/base_file_win.cc (right):

https://codereview.chromium.org/11238044/diff/8001/content/browser/download/b...
content/browser/download/base_file_win.cc:30: #define SHFILE_TO_REASON(symbol,
value, mapping)                        \
the style guide says to avoid using macros to generate code, unless absolutely
necessary. seems like this can be done without macros without a loss in
readability?

[asanka, 2012-10-24 21:40:44]

Thanks!

http://codereview.chromium.org/11238044/diff/8001/content/browser/download/ba...
File content/browser/download/base_file_win.cc (right):

http://codereview.chromium.org/11238044/diff/8001/content/browser/download/ba...
content/browser/download/base_file_win.cc:30: #define SHFILE_TO_REASON(symbol,
value, mapping)                        \
On 2012/10/24 20:49:00, John Abd-El-Malek wrote:
> the style guide says to avoid using macros to generate code, unless absolutely
> necessary. seems like this can be done without macros without a loss in
> readability?

Done.

[commit-bot: I haz the power, 2012-10-24 22:52:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asanka@chromium.org/11238044/23001

[commit-bot: I haz the power, 2012-10-25 01:23:52]

Change committed as 163983