Base: Move platform_file.* to files/file.*

Base: Move platform_file.* to files/file.*

PlatformFile has grown beyond the initial expectations and it doesn't make
sense to continue supporting individual function wrappers instead of a
proper file class.

BUG=322664
TEST=base_unittests
R=brettw@chromium.org

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

[rvargas (doing something else), 2013-11-28 02:07:21]

I'm moving the review here (from https://codereview.chromium.org/82963004/)
because rietveld/git are acting up a little after rebasing with the new files in
place.

PTAL

[brettw, 2013-12-02 19:03:07]

Just some minor questions.

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

https://codereview.chromium.org/93513002/diff/1/base/files/file.h#newcode136
base/files/file.h:136: File();
Do we need a private unimplemented copy constructor and operator= to disallow
non-move copying?

https://codereview.chromium.org/93513002/diff/1/base/files/file.h#newcode221
base/files/file.h:221: bool SetTime(Time last_access_time, Time
last_modified_time);
Do you think this would be better named "SetTimes"?

https://codereview.chromium.org/93513002/diff/1/base/files/file.h#newcode224
base/files/file.h:224: bool GetInfo(Info* info);
Can this be const?

[rvargas (doing something else), 2013-12-02 20:40:12]

Thanks

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

https://codereview.chromium.org/93513002/diff/1/base/files/file.h#newcode136
base/files/file.h:136: File();
On 2013/12/02 19:03:07, brettw wrote:
> Do we need a private unimplemented copy constructor and operator= to disallow
> non-move copying?

MOVE_ONLY_TYPE_FOR_CPP_03 provides that.

https://codereview.chromium.org/93513002/diff/1/base/files/file.h#newcode221
base/files/file.h:221: bool SetTime(Time last_access_time, Time
last_modified_time);
On 2013/12/02 19:03:07, brettw wrote:
> Do you think this would be better named "SetTimes"?

Yes it does. I'll change it.

https://codereview.chromium.org/93513002/diff/1/base/files/file.h#newcode224
base/files/file.h:224: bool GetInfo(Info* info);
On 2013/12/02 19:03:07, brettw wrote:
> Can this be const?

It could, but then we get into the whole constness of the underlying entity and
not this object.

As in, Read() for the most part doesn't change the file, and Write does, but
neither operation changes the file handle nor other members of this object :(...
and if we look closely Read may actually modify something (last access time),
and GetInfo could also do that.

So when I thought about it I decided it was better to use const only for methods
that just inspect the members of this object.

[brettw, 2013-12-02 20:44:25]

LGTM.

Your const reasoning sounds reasonable, I wonder if we should add that
description to the comment above the class?

We should be able to write a cool AsyncFile object that looks similar but
provides callbacks for each operation.

[rvargas (doing something else), 2013-12-03 00:34:05]

On 2013/12/02 20:44:25, brettw wrote:
> LGTM.
> 
> Your const reasoning sounds reasonable, I wonder if we should add that
> description to the comment above the class?
> 
> We should be able to write a cool AsyncFile object that looks similar but
> provides callbacks for each operation.

Thanks. Added a comment for const.

[commit-bot: I haz the power, 2013-12-03 00:35:17]

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

[commit-bot: I haz the power, 2013-12-03 04:17:51]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-12-03 18:53:04]

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

[commit-bot: I haz the power, 2013-12-03 19:17:04]

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

[commit-bot: I haz the power, 2013-12-03 23:46:25]

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

[commit-bot: I haz the power, 2013-12-04 00:46:35]

Change committed as 238504

[Nico, 2014-01-08 03:28:38]

https://codereview.chromium.org/93513002/diff/40001/base/files/file.h
File base/files/file.h (right):

https://codereview.chromium.org/93513002/diff/40001/base/files/file.h#newcode172
base/files/file.h:172: Error error() const { return error_; }
This looks like an error-prone api. Quick, what does this do:

  File my_file = ...;
  if (my_file.error()) {
    // Do stuff here.
  }

Turns out that "Do stuff here" is executed if there's NO error, since error()
returns an Error, not a bool, and enum value 0, the false value is FILE_OK.

This method needs to be renamed. (error_code() or something)

[rvargas (doing something else), 2014-01-08 03:42:07]

https://codereview.chromium.org/93513002/diff/40001/base/files/file.h
File base/files/file.h (right):

https://codereview.chromium.org/93513002/diff/40001/base/files/file.h#newcode172
base/files/file.h:172: Error error() const { return error_; }
On 2014/01/08 03:28:38, Nico wrote:
> This looks like an error-prone api. Quick, what does this do:
> 
>   File my_file = ...;
>   if (my_file.error()) {
>     // Do stuff here.
>   }
> 
> Turns out that "Do stuff here" is executed if there's NO error, since error()
> returns an Error, not a bool, and enum value 0, the false value is FILE_OK.
> 
> This method needs to be renamed. (error_code() or something)

yeah... that is an incorrect pattern no mater what error() is named, as the
thing to check is not the error code but the state of the object itself:

File my_file(...);
if (my_file.IsValid()) {
  // do something
}

This method is just a way for _really_ interested parties to find out what
happened, and it only refers to opening the file, not to further operations
(Read etc).

I'm open to suggestions of how to express that directly on the name :)

[Nico, 2014-01-08 03:46:28]

error_details()?


On Tue, Jan 7, 2014 at 7:42 PM, <rvargas@chromium.org> wrote:

>
> https://codereview.chromium.org/93513002/diff/40001/base/files/file.h
> File base/files/file.h (right):
>
> https://codereview.chromium.org/93513002/diff/40001/base/
> files/file.h#newcode172
> base/files/file.h:172: Error error() const { return error_; }
> On 2014/01/08 03:28:38, Nico wrote:
>
>> This looks like an error-prone api. Quick, what does this do:
>>
>
>     File my_file = ...;
>>    if (my_file.error()) {
>>      // Do stuff here.
>>    }
>>
>
>  Turns out that "Do stuff here" is executed if there's NO error, since
>>
> error()
>
>> returns an Error, not a bool, and enum value 0, the false value is
>>
> FILE_OK.
>
>  This method needs to be renamed. (error_code() or something)
>>
>
> yeah... that is an incorrect pattern no mater what error() is named, as
> the thing to check is not the error code but the state of the object
> itself:
>
> File my_file(...);
> if (my_file.IsValid()) {
>   // do something
> }
>
> This method is just a way for _really_ interested parties to find out
> what happened, and it only refers to opening the file, not to further
> operations (Read etc).
>
> I'm open to suggestions of how to express that directly on the name :)
>
> https://codereview.chromium.org/93513002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.