Refactor AppendToFile and WriteFileDescriptor

Refactor AppendToFile and WriteFileDescriptor

- Unify the behavior of the windows and posix implementations of these
  functions.
- Simplify the interface by having them just indicate success or failure
  instead of making callers deal with partial writes.

BUG=418837
Signed-off-by: Chirantan Ekbote <chirantan@chromium.org>;

Committed: https://crrev.com/75ea2fdb5c87f133a8e1b8da16f6091fb7d5321e
Cr-Commit-Position: refs/heads/master@{#298604}

[Chirantan Ekbote, 2014-10-01 17:49:20]

chirantan@chromium.org changed reviewers:
+ avi@chromium.org, brettw@chromium.org, jochen@chromium.org,
rvargas@chromium.org, tbarzic@chromium.org, wolenetz@chromium.org

[Chirantan Ekbote, 2014-10-01 17:49:21]

This is my first attempt at cleaning up the file writing in base.  Please take a
look.

rvargas: Please review base/files
brettw: Please review the rest of base/ and third_party/zlib
avi: Please review content/ and ui/
jochen: Please review chrome/ and components/
tbarzic: Please review chromeos/
wolenetz: Please review media/

Thanks :-)

[tbarzic, 2014-10-01 17:53:04]

On 2014/10/01 17:49:21, chirantan wrote:
> This is my first attempt at cleaning up the file writing in base.  Please take
a
> look.
> 
> rvargas: Please review base/files
> brettw: Please review the rest of base/ and third_party/zlib
> avi: Please review content/ and ui/
> jochen: Please review chrome/ and components/
> tbarzic: Please review chromeos/
> wolenetz: Please review media/
> 
> Thanks :-)

chromeos/process_proxy lgtm

[Avi (use Gerrit), 2014-10-01 18:06:01]

content/ and ui/ LGTM

[rvargas (doing something else), 2014-10-01 20:01:14]

https://codereview.chromium.org/614893004/diff/1/base/files/file_util.h
File base/files/file_util.h (right):

https://codereview.chromium.org/614893004/diff/1/base/files/file_util.h#newco...
base/files/file_util.h:329: // Append |data| to |fd|. Does not close |fd| when
done.  Returns true if all of
nit: appends

https://codereview.chromium.org/614893004/diff/1/base/files/file_util.h#newco...
base/files/file_util.h:336: // false otherwise.  In case of error, guarantees
that the last system error is
I'm a on the fence about the last error bit. There is only one place on all the
file related code from base where we explicitly preserve last error (and it is
actually not a documented feature). It seems like all callers are happy not
knowing the exact reason for the failure, as the current code doesn't preserve
it.

It is quite easy to preserve the error... just a few extra lines, but then we
start to lose uniformity as almost nowhere else the error is preserved (it may,
but it may not). On the other hand, it makes the code more flexible.

Brett, any opinion?

https://codereview.chromium.org/614893004/diff/1/base/files/file_util_win.cc
File base/files/file_util_win.cc (right):

https://codereview.chromium.org/614893004/diff/1/base/files/file_util_win.cc#...
base/files/file_util_win.cc:663: // if we are writing to a pipe, which we aren't
currently doing so if this
I don't think this code has enough context to know if this is a pipe or not. In
any case, there is |written| already, so why not see what it is and return false
if != size?

[brettw, 2014-10-01 20:19:46]

I'm anti-errno myself, and this is a cross-platform wrapper so most calling code
doesn't know what OS is running, making checking require ifdefs. So unless we
need it for something in particular, I'd fall on the side of simpler code.

[Chirantan Ekbote, 2014-10-01 20:25:26]

On 2014/10/01 at 20:19:46, brettw wrote:
> I'm anti-errno myself, and this is a cross-platform wrapper so most calling
code doesn't know what OS is running, making checking require ifdefs. So unless
we need it for something in particular, I'd fall on the side of simpler code.

I'm not sure I understand what you mean.  Callers will likely not care what the
exact error was but may still want to log it and PLOG will do the right thing
there without needing any ifdefs.

[rvargas (doing something else), 2014-10-01 21:33:14]

On 2014/10/01 20:25:26, chirantan wrote:
> On 2014/10/01 at 20:19:46, brettw wrote:
> > I'm anti-errno myself, and this is a cross-platform wrapper so most calling
> code doesn't know what OS is running, making checking require ifdefs. So
unless
> we need it for something in particular, I'd fall on the side of simpler code.
> 
> I'm not sure I understand what you mean.  Callers will likely not care what
the
> exact error was but may still want to log it and PLOG will do the right thing
> there without needing any ifdefs.

If the motivation is to log then a better solution would be to keep loggin
within base, possibly with VPLOG

[wolenetz, 2014-10-02 00:30:18]

media/ l g t m %
1. cast is a bit of a different beast. I recommend finding a reviewer from
media/cast/OWNERS for that piece of this CL.
2. (base/ drive-by): are you certain we never use these to write to a pipe on
windows?

[Pawel Osciak, 2014-10-02 01:05:15]

content/common/gpu/media lgtm

[miu, 2014-10-04 01:28:07]

miu@chromium.org changed reviewers:
+ miu@chromium.org

[miu, 2014-10-04 01:28:33]

On 2014/10/02 00:30:18, wolenetz wrote:
> 1. cast is a bit of a different beast. I recommend finding a reviewer from
> media/cast/OWNERS for that piece of this CL.

media/cast lgtm

[jochen (gone - plz use gerrit), 2014-10-06 07:35:07]

jochen@chromium.org changed reviewers:
+ posciak@chromium.org

[jochen (gone - plz use gerrit), 2014-10-06 07:35:09]

my bits lgtm

[Chirantan Ekbote, 2014-10-06 18:18:04]

I've removed the bit about guaranteeing that the last error is preserved and
made it so that the implementations will log the errors with VPLOG as they
happen.  Please take another look.

[rvargas (doing something else), 2014-10-06 18:47:26]

base lgtm

[Chirantan Ekbote, 2014-10-06 19:13:54]

chirantan@chromium.org changed reviewers:
+ hshi@chromium.org

[Chirantan Ekbote, 2014-10-06 19:13:55]

Hi Haixia, please review this change as owner of third_party/zlib.

[hshi1, 2014-10-06 19:16:09]

On 2014/10/06 19:13:55, chirantan wrote:
> Hi Haixia, please review this change as owner of third_party/zlib.

LGTM

[Chirantan Ekbote, 2014-10-06 19:44:03]

The CQ bit was checked by chirantan@chromium.org

[commit-bot: I haz the power, 2014-10-06 19:46:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/614893004/40001

[commit-bot: I haz the power, 2014-10-06 21:58:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-06 21:58:10]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[Chirantan Ekbote, 2014-10-06 23:44:58]

The CQ bit was checked by chirantan@chromium.org

[commit-bot: I haz the power, 2014-10-06 23:49:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/614893004/40001

[commit-bot: I haz the power, 2014-10-07 01:03:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-07 01:03:48]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[Chirantan Ekbote, 2014-10-07 23:10:40]

The CQ bit was checked by chirantan@chromium.org

[commit-bot: I haz the power, 2014-10-07 23:13:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/614893004/40001

[commit-bot: I haz the power, 2014-10-07 23:15:49]

Committed patchset #3 (id:40001) as 41d8cd3dea64c1f1851a5e2c0dd98557b823381f

[commit-bot: I haz the power, 2014-10-07 23:16:30]

Patchset 3 (id:??) landed as
https://crrev.com/75ea2fdb5c87f133a8e1b8da16f6091fb7d5321e
Cr-Commit-Position: refs/heads/master@{#298604}