FileWrapper[Impl] modifications and actually remove the "Impl" class.

FileWrapper[Impl] modifications and actually remove the "Impl" class.

This is a somewhat involved refactoring of this class. Here's an overview of the changes:

* FileWrapper can now be used as a regular class and instances allocated on the stack.
* The type now has support for move semantics and copy isn't allowed.
* New public ctor with FILE* that can be used instead of OpenFromFileHandle.
* New static Open() method.  The intent of this is to allow opening a file and getting back a FileWrapper instance.  Using this method instead of Create(), will allow us in the future to make the FILE* member pointer, to be const and simplify threading (get rid of the lock).
* Rename the Open() method to is_open() and make it inline.
* The FileWrapper interface is no longer a pure virtual interface.  There's only one implementation so there's no need to go through a vtable for everything.
* Functionality offered by the class, is now reduced.  No support for looping (not clear if that was actually useful to users of that flag), no need to implement the 'read_only_' functionality in the class, since file APIs implement that already, no support for *not* managing the file handle (this wasn't used).  OpenFromFileHandle always "manages" the file.
* Delete the unused WriteText() method and don't support opening files in text mode.  Text mode is only different on Windows and on Windows it translates \n to \r\n, which means that files such as log files, could have a slightly different format on Windows than other platforms.  Besides, tools on Windows can handle UNIX line endings.
* Remove FileName(), change Trace code to manage its own path.
* Rename id_ member variable to file_.
* Removed the open_ member variable since the same functionality can be gotten from just checking the file pointer.
* Don't call CloseFile inside of Write.  Write shouldn't be changing the state of the class beyond just attempting to write.
* Remove concept of looping from FileWrapper and never close inside of Read()
* Changed stream base classes to inherit from a common base class instead of both defining the Rewind method. Ultimately, Id' like to remove these interfaces and just have FileWrapper.
* Remove read_only param from OpenFromFileHandle
* Renamed size_in_bytes_ to position_, since it gets set to 0 when Rewind() is called (and the size actually does not change).
* Switch out rw lock for CriticalSection. The r/w lock was only used for reading when checking the open_ flag.

BUG=

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

[tommi, 2016-06-12 16:44:16]

Tag UpdateFileName as requiring |crit_| to be held

[tommi, 2016-06-12 17:43:09]

Simplify FileOpen

[tommi, 2016-06-12 18:10:28]

Remove file_impl.h

[tommi, 2016-06-12 19:20:43]

Rename Open() to is_open() and inline.

[tommi, 2016-06-12 19:21:58]

Description was changed from

==========
FileWrapper[Impl] modifications.

Offer public ctor with FILE*.
Rename id_ to file_.
Add static Open() method. Clean up OpenFile implementation.
Don't call CloseFile inside of Write
Remove concept of looping from FileWrapper and never close inside of Read()
Removed managed_file_handle_
OpenFromFileHandle now always manages the file
NULL->nullptr, param checks outside of lock, remove 'loop' from
OpenFromFileHandle
Remove FileWrapperImpl, reduce virtual methods, CloseFile returns void, support
movable
Dissallow copy, fix stream base classes wrt Rewind
Remove 'open_' member variable.
Remove support for opening in 'text' mode (it's only functionally different on
Windows)
Remove read_only param from OpenFromFileHandle
Remove read_only_ and rename position field to position_
Switch out rw lock for CriticalSection
Remove FileName(), change Trace code to manage its own path.
Delete WriteText().

BUG=
==========

to

==========
FileWrapper[Impl] modifications.

Offer public ctor with FILE*.
Rename id_ to file_.
Add static Open() method. Clean up OpenFile implementation.
Rename Open() to is_open() and make it inline.
Don't call CloseFile inside of Write
Remove concept of looping from FileWrapper and never close inside of Read()
Removed managed_file_handle_
OpenFromFileHandle now always manages the file
NULL->nullptr, param checks outside of lock, remove 'loop' from
OpenFromFileHandle
Remove FileWrapperImpl, reduce virtual methods, CloseFile returns void, support
movable
Dissallow copy, fix stream base classes wrt Rewind
Remove 'open_' member variable.
Remove support for opening in 'text' mode (it's only functionally different on
Windows)
Remove read_only param from OpenFromFileHandle
Remove read_only_ and rename position field to position_
Switch out rw lock for CriticalSection
Remove FileName(), change Trace code to manage its own path.
Delete WriteText().

BUG=
==========

[tommi, 2016-06-12 19:59:08]

Fix compile error in func_test_manager.cc

[tommi, 2016-06-13 20:19:30]

Description was changed from

==========
FileWrapper[Impl] modifications.

Offer public ctor with FILE*.
Rename id_ to file_.
Add static Open() method. Clean up OpenFile implementation.
Rename Open() to is_open() and make it inline.
Don't call CloseFile inside of Write
Remove concept of looping from FileWrapper and never close inside of Read()
Removed managed_file_handle_
OpenFromFileHandle now always manages the file
NULL->nullptr, param checks outside of lock, remove 'loop' from
OpenFromFileHandle
Remove FileWrapperImpl, reduce virtual methods, CloseFile returns void, support
movable
Dissallow copy, fix stream base classes wrt Rewind
Remove 'open_' member variable.
Remove support for opening in 'text' mode (it's only functionally different on
Windows)
Remove read_only param from OpenFromFileHandle
Remove read_only_ and rename position field to position_
Switch out rw lock for CriticalSection
Remove FileName(), change Trace code to manage its own path.
Delete WriteText().

BUG=
==========

to

==========
FileWrapper[Impl] modifications and actually remove the "Impl" class.

This is a somewhat involved refactoring of this class. Here's an overview of the
changes:

* FileWrapper can now be used as a regular class and instances allocated on the
stack.
* The type now has support for move semantics and copy isn't allowed.
* New public ctor with FILE* that can be used instead of OpenFromFileHandle.
* New static Open() method.  The intent of this is to allow opening a file and
getting back a FileWrapper instance.  Using this method instead of Create(),
will allow us in the future to make the FILE* member pointer, to be const and
simplify threading (get rid of the lock).
* Rename the Open() method to is_open() and make it inline.
* The FileWrapper interface is no longer a pure virtual interface.  There's only
one implementation so there's no need to go through a vtable for everything.
* Functionality offered by the class, is now reduced.  No support for looping
(not clear if that was actually useful to users of that flag), no need to
implement the 'read_only_' functionality in the class, since file APIs implement
that already, no support for *not* managing the file handle (this wasn't used). 
OpenFromFileHandle always "manages" the file.
* Delete the unused WriteText() method and don't support opening files in text
mode.  Text mode is only different on Windows and on Windows it translates \n to
\r\n, which means that files such as log files, could have a slightly different
format on Windows than other platforms.  Besides, tools on Windows can handle
UNIX line endings.
* Remove FileName(), change Trace code to manage its own path.
* Rename id_ member variable to file_.
* Removed the open_ member variable since the same functionality can be gotten
from just checking the file pointer.
* Don't call CloseFile inside of Write.  Write shouldn't be changing the state
of the class beyond just attempting to write.
* Remove concept of looping from FileWrapper and never close inside of Read()
* Changed stream base classes to inherit from a common base class instead of
both defining the Rewind method. Ultimately, Id' like to remove these interfaces
and just have FileWrapper.
* Remove read_only param from OpenFromFileHandle
* Renamed size_in_bytes_ to position_, since it gets set to 0 when Rewind() is
called (and the size actually does not change).
* Switch out rw lock for CriticalSection. The r/w lock was only used for reading
when checking the open_ flag.

BUG=
==========

[tommi, 2016-06-13 20:19:30]

tommi@webrtc.org changed reviewers:
+ asapersson@webrtc.org

[åsapersson, 2016-06-14 15:22:12]

lgtm

https://codereview.webrtc.org/2054373002/diff/100001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/2054373002/diff/100001/webrtc/common_types.h#ne...
webrtc/common_types.h:65: // Reads |length| bytes from file to |buf|. Returns
the number of bytes read
|len|

https://codereview.webrtc.org/2054373002/diff/100001/webrtc/system_wrappers/i...
File webrtc/system_wrappers/include/file_wrapper.h (right):

https://codereview.webrtc.org/2054373002/diff/100001/webrtc/system_wrappers/i...
webrtc/system_wrappers/include/file_wrapper.h:47: int OpenFile(const char*
file_name_utf8, bool read_only);
make method return bool?

https://codereview.webrtc.org/2054373002/diff/100001/webrtc/system_wrappers/i...
webrtc/system_wrappers/include/file_wrapper.h:49: // Initializes the wrapper
from an existing handle. |read_only| must match in
update comment..

https://codereview.webrtc.org/2054373002/diff/100001/webrtc/system_wrappers/i...
webrtc/system_wrappers/include/file_wrapper.h:52: int OpenFromFileHandle(FILE*
handle);
return bool?

https://codereview.webrtc.org/2054373002/diff/100001/webrtc/system_wrappers/i...
webrtc/system_wrappers/include/file_wrapper.h:59: int SetMaxFileSize(size_t
bytes);
void?

[tommi, 2016-06-14 20:44:56]

Address comments

[tommi, 2016-06-14 20:45:34]

https://codereview.webrtc.org/2054373002/diff/100001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/2054373002/diff/100001/webrtc/common_types.h#ne...
webrtc/common_types.h:65: // Reads |length| bytes from file to |buf|. Returns
the number of bytes read
On 2016/06/14 15:22:11, åsapersson wrote:
> |len|

Done.

https://codereview.webrtc.org/2054373002/diff/100001/webrtc/system_wrappers/i...
File webrtc/system_wrappers/include/file_wrapper.h (right):

https://codereview.webrtc.org/2054373002/diff/100001/webrtc/system_wrappers/i...
webrtc/system_wrappers/include/file_wrapper.h:47: int OpenFile(const char*
file_name_utf8, bool read_only);
On 2016/06/14 15:22:12, åsapersson wrote:
> make method return bool?

Done.

https://codereview.webrtc.org/2054373002/diff/100001/webrtc/system_wrappers/i...
webrtc/system_wrappers/include/file_wrapper.h:49: // Initializes the wrapper
from an existing handle. |read_only| must match in
On 2016/06/14 15:22:12, åsapersson wrote:
> update comment..

Done.

https://codereview.webrtc.org/2054373002/diff/100001/webrtc/system_wrappers/i...
webrtc/system_wrappers/include/file_wrapper.h:52: int OpenFromFileHandle(FILE*
handle);
On 2016/06/14 15:22:12, åsapersson wrote:
> return bool?

Done.

https://codereview.webrtc.org/2054373002/diff/100001/webrtc/system_wrappers/i...
webrtc/system_wrappers/include/file_wrapper.h:59: int SetMaxFileSize(size_t
bytes);
On 2016/06/14 15:22:12, åsapersson wrote:
> void?

Done.

[tommi, 2016-06-14 20:45:43]

The CQ bit was checked by tommi@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-14 20:45:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2054373002/120001

[commit-bot: I haz the power, 2016-06-14 20:54:22]

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

[commit-bot: I haz the power, 2016-06-14 20:54:22]

Dry run: Try jobs failed on following builders:
  win_x64_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/15478)

[tommi, 2016-06-15 16:32:45]

Fix use of ASSERT instead of ASSERT_TRUE in test

[tommi, 2016-06-15 16:33:02]

The CQ bit was checked by tommi@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-15 16:33:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2054373002/140001

[commit-bot: I haz the power, 2016-06-15 17:05:19]

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

[commit-bot: I haz the power, 2016-06-15 17:05:20]

Dry run: This issue passed the CQ dry run.

[tommi, 2016-06-15 17:28:17]

The CQ bit was checked by tommi@webrtc.org

[tommi, 2016-06-15 17:28:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
asapersson@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2054373002/#ps140001 (title:
"Fix use of ASSERT instead of ASSERT_TRUE in test")

[commit-bot: I haz the power, 2016-06-15 17:28:29]

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

[commit-bot: I haz the power, 2016-06-15 17:30:18]

Description was changed from

==========
FileWrapper[Impl] modifications and actually remove the "Impl" class.

This is a somewhat involved refactoring of this class. Here's an overview of the
changes:

* FileWrapper can now be used as a regular class and instances allocated on the
stack.
* The type now has support for move semantics and copy isn't allowed.
* New public ctor with FILE* that can be used instead of OpenFromFileHandle.
* New static Open() method.  The intent of this is to allow opening a file and
getting back a FileWrapper instance.  Using this method instead of Create(),
will allow us in the future to make the FILE* member pointer, to be const and
simplify threading (get rid of the lock).
* Rename the Open() method to is_open() and make it inline.
* The FileWrapper interface is no longer a pure virtual interface.  There's only
one implementation so there's no need to go through a vtable for everything.
* Functionality offered by the class, is now reduced.  No support for looping
(not clear if that was actually useful to users of that flag), no need to
implement the 'read_only_' functionality in the class, since file APIs implement
that already, no support for *not* managing the file handle (this wasn't used). 
OpenFromFileHandle always "manages" the file.
* Delete the unused WriteText() method and don't support opening files in text
mode.  Text mode is only different on Windows and on Windows it translates \n to
\r\n, which means that files such as log files, could have a slightly different
format on Windows than other platforms.  Besides, tools on Windows can handle
UNIX line endings.
* Remove FileName(), change Trace code to manage its own path.
* Rename id_ member variable to file_.
* Removed the open_ member variable since the same functionality can be gotten
from just checking the file pointer.
* Don't call CloseFile inside of Write.  Write shouldn't be changing the state
of the class beyond just attempting to write.
* Remove concept of looping from FileWrapper and never close inside of Read()
* Changed stream base classes to inherit from a common base class instead of
both defining the Rewind method. Ultimately, Id' like to remove these interfaces
and just have FileWrapper.
* Remove read_only param from OpenFromFileHandle
* Renamed size_in_bytes_ to position_, since it gets set to 0 when Rewind() is
called (and the size actually does not change).
* Switch out rw lock for CriticalSection. The r/w lock was only used for reading
when checking the open_ flag.

BUG=
==========

to

==========
FileWrapper[Impl] modifications and actually remove the "Impl" class.

This is a somewhat involved refactoring of this class. Here's an overview of the
changes:

* FileWrapper can now be used as a regular class and instances allocated on the
stack.
* The type now has support for move semantics and copy isn't allowed.
* New public ctor with FILE* that can be used instead of OpenFromFileHandle.
* New static Open() method.  The intent of this is to allow opening a file and
getting back a FileWrapper instance.  Using this method instead of Create(),
will allow us in the future to make the FILE* member pointer, to be const and
simplify threading (get rid of the lock).
* Rename the Open() method to is_open() and make it inline.
* The FileWrapper interface is no longer a pure virtual interface.  There's only
one implementation so there's no need to go through a vtable for everything.
* Functionality offered by the class, is now reduced.  No support for looping
(not clear if that was actually useful to users of that flag), no need to
implement the 'read_only_' functionality in the class, since file APIs implement
that already, no support for *not* managing the file handle (this wasn't used). 
OpenFromFileHandle always "manages" the file.
* Delete the unused WriteText() method and don't support opening files in text
mode.  Text mode is only different on Windows and on Windows it translates \n to
\r\n, which means that files such as log files, could have a slightly different
format on Windows than other platforms.  Besides, tools on Windows can handle
UNIX line endings.
* Remove FileName(), change Trace code to manage its own path.
* Rename id_ member variable to file_.
* Removed the open_ member variable since the same functionality can be gotten
from just checking the file pointer.
* Don't call CloseFile inside of Write.  Write shouldn't be changing the state
of the class beyond just attempting to write.
* Remove concept of looping from FileWrapper and never close inside of Read()
* Changed stream base classes to inherit from a common base class instead of
both defining the Rewind method. Ultimately, Id' like to remove these interfaces
and just have FileWrapper.
* Remove read_only param from OpenFromFileHandle
* Renamed size_in_bytes_ to position_, since it gets set to 0 when Rewind() is
called (and the size actually does not change).
* Switch out rw lock for CriticalSection. The r/w lock was only used for reading
when checking the open_ flag.

BUG=
==========

[commit-bot: I haz the power, 2016-06-15 17:30:20]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-06-15 17:30:23]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-15 17:30:26]

Description was changed from

==========
FileWrapper[Impl] modifications and actually remove the "Impl" class.

This is a somewhat involved refactoring of this class. Here's an overview of the
changes:

* FileWrapper can now be used as a regular class and instances allocated on the
stack.
* The type now has support for move semantics and copy isn't allowed.
* New public ctor with FILE* that can be used instead of OpenFromFileHandle.
* New static Open() method.  The intent of this is to allow opening a file and
getting back a FileWrapper instance.  Using this method instead of Create(),
will allow us in the future to make the FILE* member pointer, to be const and
simplify threading (get rid of the lock).
* Rename the Open() method to is_open() and make it inline.
* The FileWrapper interface is no longer a pure virtual interface.  There's only
one implementation so there's no need to go through a vtable for everything.
* Functionality offered by the class, is now reduced.  No support for looping
(not clear if that was actually useful to users of that flag), no need to
implement the 'read_only_' functionality in the class, since file APIs implement
that already, no support for *not* managing the file handle (this wasn't used). 
OpenFromFileHandle always "manages" the file.
* Delete the unused WriteText() method and don't support opening files in text
mode.  Text mode is only different on Windows and on Windows it translates \n to
\r\n, which means that files such as log files, could have a slightly different
format on Windows than other platforms.  Besides, tools on Windows can handle
UNIX line endings.
* Remove FileName(), change Trace code to manage its own path.
* Rename id_ member variable to file_.
* Removed the open_ member variable since the same functionality can be gotten
from just checking the file pointer.
* Don't call CloseFile inside of Write.  Write shouldn't be changing the state
of the class beyond just attempting to write.
* Remove concept of looping from FileWrapper and never close inside of Read()
* Changed stream base classes to inherit from a common base class instead of
both defining the Rewind method. Ultimately, Id' like to remove these interfaces
and just have FileWrapper.
* Remove read_only param from OpenFromFileHandle
* Renamed size_in_bytes_ to position_, since it gets set to 0 when Rewind() is
called (and the size actually does not change).
* Switch out rw lock for CriticalSection. The r/w lock was only used for reading
when checking the open_ flag.

BUG=
==========

to

==========
FileWrapper[Impl] modifications and actually remove the "Impl" class.

This is a somewhat involved refactoring of this class. Here's an overview of the
changes:

* FileWrapper can now be used as a regular class and instances allocated on the
stack.
* The type now has support for move semantics and copy isn't allowed.
* New public ctor with FILE* that can be used instead of OpenFromFileHandle.
* New static Open() method.  The intent of this is to allow opening a file and
getting back a FileWrapper instance.  Using this method instead of Create(),
will allow us in the future to make the FILE* member pointer, to be const and
simplify threading (get rid of the lock).
* Rename the Open() method to is_open() and make it inline.
* The FileWrapper interface is no longer a pure virtual interface.  There's only
one implementation so there's no need to go through a vtable for everything.
* Functionality offered by the class, is now reduced.  No support for looping
(not clear if that was actually useful to users of that flag), no need to
implement the 'read_only_' functionality in the class, since file APIs implement
that already, no support for *not* managing the file handle (this wasn't used). 
OpenFromFileHandle always "manages" the file.
* Delete the unused WriteText() method and don't support opening files in text
mode.  Text mode is only different on Windows and on Windows it translates \n to
\r\n, which means that files such as log files, could have a slightly different
format on Windows than other platforms.  Besides, tools on Windows can handle
UNIX line endings.
* Remove FileName(), change Trace code to manage its own path.
* Rename id_ member variable to file_.
* Removed the open_ member variable since the same functionality can be gotten
from just checking the file pointer.
* Don't call CloseFile inside of Write.  Write shouldn't be changing the state
of the class beyond just attempting to write.
* Remove concept of looping from FileWrapper and never close inside of Read()
* Changed stream base classes to inherit from a common base class instead of
both defining the Rewind method. Ultimately, Id' like to remove these interfaces
and just have FileWrapper.
* Remove read_only param from OpenFromFileHandle
* Renamed size_in_bytes_ to position_, since it gets set to 0 when Rewind() is
called (and the size actually does not change).
* Switch out rw lock for CriticalSection. The r/w lock was only used for reading
when checking the open_ flag.

BUG=

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

[commit-bot: I haz the power, 2016-06-15 17:30:28]

Patchset 8 (id:??) landed as
https://crrev.com/a6219cc3ef08dd9b2981b065b6f051d7de0866f8
Cr-Commit-Position: refs/heads/master@{#13155}

[nisse-webrtc, 2016-10-19 11:10:14]

On 2016/06/15 17:30:28, commit-bot: I haz the power wrote:
> Patchset 8 (id:??) landed as
> https://crrev.com/a6219cc3ef08dd9b2981b065b6f051d7de0866f8
> Cr-Commit-Position: refs/heads/master@{#13155}

This change breaks building with rtc_enable_data_logging = true, errors like

../../webrtc/system_wrappers/source/data_log.cc:228:16: error: no member named
'WriteText' in 'webrtc::FileWrapper'
        file_->WriteText("%s[%u],", column_it->first.c_str(),
        ~~~~~  ^

I'm considering simply deleting all of the data_log stuff, used in only one
place, webrtc/modules/audio_coding/neteq/audio_decoder_unittest.cc; if no one's
noticed in four months that it's broken, it's unlikely to ever be used.