Implement File::WriteAtCurrentPos for Windows

Implement File::WriteAtCurrentPos for Windows

BUG=145873, 322664
R=cpu@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255622

[rvargas (doing something else), 2014-03-05 21:01:41]

PTAL

[cpu_(ooo_6.6-7.5), 2014-03-06 20:43:41]

lgtm

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

https://codereview.chromium.org/183333004/diff/1/base/files/file_win.cc#newco...
base/files/file_win.cc:142: if (::ReadFile(file_, data, size, &bytes_read,
&overlapped))
I am confused about the style unification here, below you seem to want

 if ( WindowsCallThatReuturnsBOOL(...) != FALSE)

But at 142 you dont.

https://codereview.chromium.org/183333004/diff/1/base/files/file_win.cc#newco...
base/files/file_win.cc:148: }
Just a suggestion:

it would look pretty if there was a function here

bool CheckConditions(int size) {
  base::ThreadRestrictions::AssertIOAllowed();
  DCHECK(IsValid());
  return (size < 0);
}

that you could use all over this file, by my count at least 22 times, in two
flavors

if (CheckContions(size))
 return -1;

or just CheckContions(0);

[rvargas (doing something else), 2014-03-06 21:55:34]

Thanks!.

+thakis for owners

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

https://codereview.chromium.org/183333004/diff/1/base/files/file_win.cc#newco...
base/files/file_win.cc:142: if (::ReadFile(file_, data, size, &bytes_read,
&overlapped))
On 2014/03/06 20:43:41, cpu wrote:
> I am confused about the style unification here, below you seem to want
> 
>  if ( WindowsCallThatReuturnsBOOL(...) != FALSE)
> 
> But at 142 you dont.

I generally don't want to compare against anything (like here), but there are
some places (return statements) where we have to compare against something or
the compiler will warn about casting from int to bool (and fail given that there
is a warning). Between comparing against FALSE and using "!!Foo()" I prefer
comparing against FALSE.

https://codereview.chromium.org/183333004/diff/1/base/files/file_win.cc#newco...
base/files/file_win.cc:148: }
On 2014/03/06 20:43:41, cpu wrote:
> Just a suggestion:
> 
> it would look pretty if there was a function here
> 
> bool CheckConditions(int size) {
>   base::ThreadRestrictions::AssertIOAllowed();
>   DCHECK(IsValid());
>   return (size < 0);
> }
> 
> that you could use all over this file, by my count at least 22 times, in two
> flavors
> 
> if (CheckContions(size))
>  return -1;
> 
> or just CheckContions(0);

That method would look a little suspicious to me as it seems to be about three
lines that are not that connected to each other (especially the size check):
internal object state, usage on the proper thread (both preconditions), and
argument sanity that may or may not apply for a given method.

Also, I'd say that most people care about the implementation of a single
operation, so there is value in having all there is to a function right there.

[Nico, 2014-03-06 23:57:58]

lgtm

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

https://codereview.chromium.org/183333004/diff/1/base/files/file_win.cc#newco...
base/files/file_win.cc:193: int File::WriteAtCurrentPos(const char* data, int
size) {
fwiw, i'd have named "write" "writeatoffset" and "writeatcurrentpos" "write",
for consistentcy with fwrite and istream::write etc

[rvargas (doing something else), 2014-03-07 01:28:19]

The CQ bit was checked by rvargas@chromium.org

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rvargas@chromium.org/183333004/1

[commit-bot: I haz the power, 2014-03-07 15:35:28]

Change committed as 255622