Allow passing in strings of length zero to FileWrapper::Write without closing the file.

webrtc/system_wrappers/source/file_impl.cc

Index: webrtc/system_wrappers/source/file_impl.cc
diff --git a/webrtc/system_wrappers/source/file_impl.cc b/webrtc/system_wrappers/source/file_impl.cc
index 0ee0deab6c1061edb3f475d44a3ff9b7e6f7f51d..ea09661e19d442a81be2576ead48358c7c3bb5ef 100644
--- a/webrtc/system_wrappers/source/file_impl.cc
+++ b/webrtc/system_wrappers/source/file_impl.cc
@@ -243,8 +243,8 @@ bool FileWrapperImpl::Write(const void* buf, size_t length) {
   }
 
   size_t num_bytes = fwrite(buf, 1, length, id_);
-  if (num_bytes > 0) {
-    size_in_bytes_ += num_bytes;
+  size_in_bytes_ += num_bytes;

[Henrik Grunell WebRTC, 2016/02/19 15:04:38]

Keep this inside the if block below. Not needed if we close the file.

[terelius, 2016/02/19 16:50:29]

By not closing the file, we can remove the entire if block.

+  if (num_bytes == length) {

[Henrik Grunell WebRTC, 2016/02/19 15:04:38]

The original code is unclear. Good to change this.

1. Checking that we actually wrote the amount we wanted is good.

2. Should we really allow |length| == 0? The user of this class shouldn't really
call if there's nothing to write.

3. It seems strange that the file is closed here at all. The user should decide
what to do if an error happens.

I don't want to block this fix. If (2) and (3) this causes larger changes
outside this class, I'm fine with not changing that. Add a todo to clean this
up.

[terelius, 2016/02/19 16:05:52]

I agree on 1 and 3. I'd also prefer not closing the file. If you think that's
ok, I could shorten the function by 5 lines. I guess that the original author
chose to close the file because the file position indicator is undetermined
after an fwrite error.

Regarding 2: Although it is pointless to write an empty string, I think it is
nevertheless useful to allow since the string could be the result of some
computation. If Write disallows empty strings you'd have to add a check at every
call site (unless you can guarantee that the computation can't produce empty
strings). Moreover, those checks would duplicate checks that are done by fwrite
anyway.

Secondly, the description of the return value becomes more clear if empty
strings are allowed. Compare "true if |length| bytes were written" to "true if
|length| bytes were written and |length| is non-zero".

     return true;
   }