Initial version of new file wrapper

Initial version of new file wrapper

BUG=webrt:6177
R=mflodman@webrtc.org, sprang@webrtc.org

Committed: https://crrev.com/d714d19d224b356ab2f14ed7ba6e872a4df6c84f
Cr-Commit-Position: refs/heads/master@{#14016}

[palmkvist, 2016-08-05 13:49:33]

palmkvist@webrtc.org changed reviewers:
+ sprang@webrtc.org

[sprang_webrtc, 2016-08-05 14:45:14]

https://codereview.webrtc.org/2214763002/diff/20001/webrtc/base/file.cc
File webrtc/base/file.cc (right):

https://codereview.webrtc.org/2214763002/diff/20001/webrtc/base/file.cc#newco...
webrtc/base/file.cc:40: }
nit: empty line between methods
here and elsewhere

https://codereview.webrtc.org/2214763002/diff/20001/webrtc/base/file.cc#newco...
webrtc/base/file.cc:75: #if defined(WEBRTC_WIN)
If there's a large amount of code different between platforms, you may want to
have separate files for the implementations

https://codereview.webrtc.org/2214763002/diff/20001/webrtc/base/file.cc#newco...
webrtc/base/file.cc:78: RTC_CHECK(length < std::numeric_limits<DWORD>::max());
RTC_CHECK_LT

https://codereview.webrtc.org/2214763002/diff/20001/webrtc/base/file.cc#newco...
webrtc/base/file.cc:81: NULL))
nullptr

https://codereview.webrtc.org/2214763002/diff/20001/webrtc/base/file.cc#newco...
webrtc/base/file.cc:82: return size_t{bytes_written};
wrap in {} if if-statement is multi-line

Don't think you need cast

https://codereview.webrtc.org/2214763002/diff/20001/webrtc/base/file.cc#newco...
webrtc/base/file.cc:147: })
Avoid macros if possible

https://codereview.webrtc.org/2214763002/diff/20001/webrtc/base/file.h
File webrtc/base/file.h (right):

https://codereview.webrtc.org/2214763002/diff/20001/webrtc/base/file.h#newcode18
webrtc/base/file.h:18: 
Could you add comments about the intended usage here, especially how the
*NoBestEffort differs?

Actually I'm not sure we need the NoBestEffort (previously WriteAll?) ones

https://codereview.webrtc.org/2214763002/diff/20001/webrtc/base/file.h#newcode22
webrtc/base/file.h:22: ~File();
Does the destructor implicitly close the file? Might be good to document itc.

https://codereview.webrtc.org/2214763002/diff/20001/webrtc/base/file.h#newcode24
webrtc/base/file.h:24: size_t Write(const char* data, size_t length);
uint8_t* instead of char* here and elsewhere

https://codereview.webrtc.org/2214763002/diff/20001/webrtc/base/file_unittest.cc
File webrtc/base/file_unittest.cc (right):

https://codereview.webrtc.org/2214763002/diff/20001/webrtc/base/file_unittest...
webrtc/base/file_unittest.cc:23: rtc::PlatformFile OpenFile(const std::string&
path) {
Could this be a static function in the File class instead?
Also, a static method there to get a temp file I think would be great.

https://codereview.webrtc.org/2214763002/diff/20001/webrtc/base/file_unittest...
webrtc/base/file_unittest.cc:75: EXPECT_EQ(size_t{10}, file.Write(data, 10));
I think you can just use 10u instead of a cast.

[palmkvist, 2016-08-08 11:15:35]

https://codereview.webrtc.org/2214763002/diff/20001/webrtc/base/file.cc
File webrtc/base/file.cc (right):

https://codereview.webrtc.org/2214763002/diff/20001/webrtc/base/file.cc#newco...
webrtc/base/file.cc:40: }
On 2016/08/05 14:45:14, språng wrote:
> nit: empty line between methods
> here and elsewhere

Done.

https://codereview.webrtc.org/2214763002/diff/20001/webrtc/base/file.cc#newco...
webrtc/base/file.cc:75: #if defined(WEBRTC_WIN)
On 2016/08/05 14:45:14, språng wrote:
> If there's a large amount of code different between platforms, you may want to
> have separate files for the implementations

Done.

https://codereview.webrtc.org/2214763002/diff/20001/webrtc/base/file.cc#newco...
webrtc/base/file.cc:78: RTC_CHECK(length < std::numeric_limits<DWORD>::max());
On 2016/08/05 14:45:14, språng wrote:
> RTC_CHECK_LT

Done.

https://codereview.webrtc.org/2214763002/diff/20001/webrtc/base/file.cc#newco...
webrtc/base/file.cc:81: NULL))
On 2016/08/05 14:45:14, språng wrote:
> nullptr

Done.

https://codereview.webrtc.org/2214763002/diff/20001/webrtc/base/file.cc#newco...
webrtc/base/file.cc:147: })
On 2016/08/05 14:45:14, språng wrote:
> Avoid macros if possible

It is possible, with some repetition (four times, so not a lot, but still). I
also intend to add logging if the loop runs too many times in debug mode
(whenever I figure out what the defined flag is), all of which obscures the
otherwise rather simple methods.

Are these strong enough arguments, or should I duplicate the code? (lambdas
would add unnecessary overhead, and since this is supposed to be an efficient
base wrapper I figure that's not ideal)

https://codereview.webrtc.org/2214763002/diff/20001/webrtc/base/file.h
File webrtc/base/file.h (right):

https://codereview.webrtc.org/2214763002/diff/20001/webrtc/base/file.h#newcode22
webrtc/base/file.h:22: ~File();
On 2016/08/05 14:45:14, språng wrote:
> Does the destructor implicitly close the file? Might be good to document itc.

Done.

https://codereview.webrtc.org/2214763002/diff/20001/webrtc/base/file.h#newcode24
webrtc/base/file.h:24: size_t Write(const char* data, size_t length);
On 2016/08/05 14:45:14, språng wrote:
> uint8_t* instead of char* here and elsewhere

Done.

https://codereview.webrtc.org/2214763002/diff/20001/webrtc/base/file_unittest.cc
File webrtc/base/file_unittest.cc (right):

https://codereview.webrtc.org/2214763002/diff/20001/webrtc/base/file_unittest...
webrtc/base/file_unittest.cc:23: rtc::PlatformFile OpenFile(const std::string&
path) {
On 2016/08/05 14:45:14, språng wrote:
> Could this be a static function in the File class instead?
> Also, a static method there to get a temp file I think would be great.

Definitely, but it apparently won't work in chrome, so I didn't put it there.
Should I add it anyway, and make compilation conditional in some way?

https://codereview.webrtc.org/2214763002/diff/20001/webrtc/base/file_unittest...
webrtc/base/file_unittest.cc:75: EXPECT_EQ(size_t{10}, file.Write(data, 10));
On 2016/08/05 14:45:14, språng wrote:
> I think you can just use 10u instead of a cast.

Done.

[sprang_webrtc, 2016-08-15 09:43:08]

https://codereview.webrtc.org/2214763002/diff/270001/webrtc/base/file.h
File webrtc/base/file.h (right):

https://codereview.webrtc.org/2214763002/diff/270001/webrtc/base/file.h#newco...
webrtc/base/file.h:26: // The NoBestEffort variants do not do this.
I'm thinking we should switch this naming around, making synchronous write the
default and naming the other WriteBestEffort. Feels like you normally want the
WriteAll version. WDYT?

https://codereview.webrtc.org/2214763002/diff/270001/webrtc/base/file_posix.cc
File webrtc/base/file_posix.cc (right):

https://codereview.webrtc.org/2214763002/diff/270001/webrtc/base/file_posix.c...
webrtc/base/file_posix.cc:25: })
I think I would actually preffer having explicit code rather than this macro,
despite that adding some boilderplate.

https://codereview.webrtc.org/2214763002/diff/270001/webrtc/base/file_posix.c...
webrtc/base/file_posix.cc:29: return rv < 0 ? 0 : size_t{rv};
Prefer static_cast<size_t>() to size_t{}
Also don't think you need this cast here, unless ssize_t is actually larger than
size_t? In that case do a static assert.

https://codereview.webrtc.org/2214763002/diff/270001/webrtc/base/file_unittes...
File webrtc/base/file_unittest.cc (right):

https://codereview.webrtc.org/2214763002/diff/270001/webrtc/base/file_unittes...
webrtc/base/file_unittest.cc:31: #else
I usually like 
#else  // if defined(WEBRTC_WIN)
for readability.

https://codereview.webrtc.org/2214763002/diff/270001/webrtc/base/file_unittes...
webrtc/base/file_unittest.cc:129: #endif
Looks like a LastError() method in FileTest could help here and below.

[palmkvist, 2016-08-16 08:55:36]

https://codereview.webrtc.org/2214763002/diff/270001/webrtc/base/file.h
File webrtc/base/file.h (right):

https://codereview.webrtc.org/2214763002/diff/270001/webrtc/base/file.h#newco...
webrtc/base/file.h:26: // The NoBestEffort variants do not do this.
On 2016/08/15 09:43:07, språng wrote:
> I'm thinking we should switch this naming around, making synchronous write the
> default and naming the other WriteBestEffort. Feels like you normally want the
> WriteAll version. WDYT?

I agree that WriteAll should be the default, but I figured the best way to do
that was to give that the simplest name; Write is equivalent to the previous
WriteAll, WriteNoBestEffort is the version that might not write everything.

After thinking some more on it though I think that the NoBestEffort versions are
probably not very useful, my assumption is that you either want to write
everything in a blocking fashion, or you want some stronger asynchronous
guarantees. 

I vote removing the NoBestEffort versions and later adding some proper async
versions.

https://codereview.webrtc.org/2214763002/diff/270001/webrtc/base/file_posix.cc
File webrtc/base/file_posix.cc (right):

https://codereview.webrtc.org/2214763002/diff/270001/webrtc/base/file_posix.c...
webrtc/base/file_posix.cc:29: return rv < 0 ? 0 : size_t{rv};
On 2016/08/15 09:43:07, språng wrote:
> Prefer static_cast<size_t>() to size_t{}
> Also don't think you need this cast here, unless ssize_t is actually larger
than
> size_t? In that case do a static assert.

Done.

https://codereview.webrtc.org/2214763002/diff/270001/webrtc/base/file_unittes...
File webrtc/base/file_unittest.cc (right):

https://codereview.webrtc.org/2214763002/diff/270001/webrtc/base/file_unittes...
webrtc/base/file_unittest.cc:31: #else
On 2016/08/15 09:43:07, språng wrote:
> I usually like 
> #else  // if defined(WEBRTC_WIN)
> for readability.

Done.

[sprang, 2016-08-16 09:27:43]

sprang@google.com changed reviewers:
+ sprang@google.com

[sprang, 2016-08-16 09:27:44]

https://codereview.webrtc.org/2214763002/diff/270001/webrtc/base/file.h
File webrtc/base/file.h (right):

https://codereview.webrtc.org/2214763002/diff/270001/webrtc/base/file.h#newco...
webrtc/base/file.h:26: // The NoBestEffort variants do not do this.
On 2016/08/16 08:55:35, palmkvist wrote:
> On 2016/08/15 09:43:07, språng wrote:
> > I'm thinking we should switch this naming around, making synchronous write
the
> > default and naming the other WriteBestEffort. Feels like you normally want
the
> > WriteAll version. WDYT?
> 
> I agree that WriteAll should be the default, but I figured the best way to do
> that was to give that the simplest name; Write is equivalent to the previous
> WriteAll, WriteNoBestEffort is the version that might not write everything.
> 
> After thinking some more on it though I think that the NoBestEffort versions
are
> probably not very useful, my assumption is that you either want to write
> everything in a blocking fashion, or you want some stronger asynchronous
> guarantees. 
> 
> I vote removing the NoBestEffort versions and later adding some proper async
> versions.

Agreed, let's only add a best-effort method if we actually have a use case for
it.

[palmkvist, 2016-08-17 11:53:40]

palmkvist@webrtc.org changed reviewers:
+ mflodman@webrtc.org

[palmkvist, 2016-08-17 11:53:41]

This should be a simple replacement for FileWrapper, initially supporting only
synchronous reads/writes (no actual use case for async yet), intended to be
efficient and minimalistic (i.e. a good default).

Comments?

[sprang_webrtc, 2016-08-24 09:07:11]

sprang@webrtc.org changed reviewers:
- sprang@google.com

[sprang_webrtc, 2016-08-24 09:07:12]

lgtm
mflodman, ptal

[mflodman, 2016-08-31 07:53:28]

Quick owner's review, trusting Erik's thorough review for this.

LGTM with one comment.

https://codereview.webrtc.org/2214763002/diff/450001/webrtc/base/file_unittes...
File webrtc/base/file_unittest.cc (right):

https://codereview.webrtc.org/2214763002/diff/450001/webrtc/base/file_unittes...
webrtc/base/file_unittest.cc:2: *  Copyright 2015 The WebRTC Project Authors.
All rights reserved.
s/2015/2016

[palmkvist, 2016-08-31 08:59:18]

The CQ bit was checked by palmkvist@webrtc.org

[palmkvist, 2016-08-31 08:59:19]

The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org,
mflodman@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2214763002/#ps470001 (title:
"Correct copyright year")

[commit-bot: I haz the power, 2016-08-31 08:59:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-08-31 09:00:58]

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

[commit-bot: I haz the power, 2016-08-31 09:00:59]

Try jobs failed on following builders:
  mac_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/13859)

[palmkvist, 2016-08-31 11:10:48]

The CQ bit was checked by palmkvist@webrtc.org

[palmkvist, 2016-08-31 11:10:49]

The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org,
mflodman@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2214763002/#ps490001 (title:
"Rebase")

[commit-bot: I haz the power, 2016-08-31 11:10:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-08-31 11:11:40]

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

[commit-bot: I haz the power, 2016-08-31 11:11:41]

Try jobs failed on following builders:
  ios64_gn_dbg on master.tryserver.webrtc (JOB_FAILED, no build URL)
  ios64_gn_rel on master.tryserver.webrtc (JOB_FAILED, no build URL)

[palmkvist, 2016-09-01 09:44:20]

Description was changed from

==========
Initial version of new file wrapper

BUG=webrt:6177
==========

to

==========
Initial version of new file wrapper

BUG=webrt:6177
R=mflodman@webrtc.org, sprang@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/d714d19d224b356ab2f14ed7b...
==========

[palmkvist, 2016-09-01 09:44:23]

Committed patchset #26 (id:490001) manually as
d714d19d224b356ab2f14ed7ba6e872a4df6c84f (presubmit successful).

[commit-bot: I haz the power, 2016-09-01 09:44:25]

Description was changed from

==========
Initial version of new file wrapper

BUG=webrt:6177
R=mflodman@webrtc.org, sprang@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/d714d19d224b356ab2f14ed7b...
==========

to

==========
Initial version of new file wrapper

BUG=webrt:6177
R=mflodman@webrtc.org, sprang@webrtc.org

Committed: https://crrev.com/d714d19d224b356ab2f14ed7ba6e872a4df6c84f
Cr-Commit-Position: refs/heads/master@{#14016}
==========

[palmkvist, 2016-09-01 11:15:21]

A revert of this CL (patchset #26 id:490001) has been created in
https://codereview.webrtc.org/2299983002/ by palmkvist@webrtc.org.

The reason for reverting is: Breaks things upstream.