Update/improve base::File comments.

Update/improve base::File comments.

BUG=none

Committed: https://crrev.com/718a46e5e13e8bb8ab44c78909b7b6a042f46190
Cr-Commit-Position: refs/heads/master@{#321003}

[Thiemo Nagel, 2015-03-13 10:02:52]

tnagel@chromium.org changed reviewers:
+ rvargas@chromium.org

[Thiemo Nagel, 2015-03-13 10:02:52]

Hi Ricardo,

could you please take a look at this small doc improvement?  Is it correct to
remove the reference to "ParamTraits<base::PlatformFileInfo>"?  It seems to me
that it doesn't exist (anymore).

Cheers,
Thiemo

[rvargas (doing something else), 2015-03-13 18:37:01]

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

https://codereview.chromium.org/1000393003/diff/1/base/files/file.h#oldcode133
base/files/file.h:133: // chrome/common/common_param_traits.cc.
This is now ParamTraits<base::File::Info>... but I *think* there's nothing to be
updated over there (depends directly in this code), so sure, let's remove this.

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

https://codereview.chromium.org/1000393003/diff/1/base/files/file.h#newcode144
base/files/file.h:144: // True if the file corresponds to a directory.  Always
false on Windows.
I don't think this is true.

https://codereview.chromium.org/1000393003/diff/1/base/files/file.h#newcode148
base/files/file.h:148: // Windows.
Reflects the current implementation, but may not be what we want from the
interface. I don't have a use case to fix the implementation though (which is
why I have never bothered). Where does it matter? (just curious)

[rvargas (doing something else), 2015-03-13 18:44:20]

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

https://codereview.chromium.org/1000393003/diff/1/base/files/file.h#newcode144
base/files/file.h:144: // True if the file corresponds to a directory.  Always
false on Windows.
To be clear, if the File points to a directory, GetInfo should return true. I
think it does, it doesn't, it sounds like a bug.

https://codereview.chromium.org/1000393003/diff/1/base/files/file.h#newcode148
base/files/file.h:148: // Windows.
(and I'm basically fine either way)

[Thiemo Nagel, 2015-03-16 16:28:39]

Thanks for the review!

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

https://codereview.chromium.org/1000393003/diff/1/base/files/file.h#oldcode133
base/files/file.h:133: // chrome/common/common_param_traits.cc.
On 2015/03/13 18:37:01, rvargas wrote:
> This is now ParamTraits<base::File::Info>... but I *think* there's nothing to
be
> updated over there (depends directly in this code), so sure, let's remove
this.

I couldn't find it and thought it had been removed entirely, but now that you
say that it still exists, I'd just update the reference.

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

https://codereview.chromium.org/1000393003/diff/1/base/files/file.h#newcode144
base/files/file.h:144: // True if the file corresponds to a directory.  Always
false on Windows.
On 2015/03/13 18:44:20, rvargas wrote:
> To be clear, if the File points to a directory, GetInfo should return true. I
> think it does, it doesn't, it sounds like a bug.

I thought you couldn't even open a directory in Windows, i.e. IsValid() should
be false for base::File("c:\windows")?  So you can't even call GetInfo() on a
directory on Windows?

At least when I tried opening a directory on a Windows bot, it failed.  Also I
can't find code in file_win.cc which sets is_directory to true.  But maybe I'm
missing something crucial here?

https://codereview.chromium.org/1000393003/diff/1/base/files/file.h#newcode148
base/files/file.h:148: // Windows.
On 2015/03/13 18:37:00, rvargas wrote:
> Reflects the current implementation, but may not be what we want from the
> interface. I don't have a use case to fix the implementation though (which is
> why I have never bothered). Where does it matter? (just curious)

There is no specific use case.  It's just me trying to understand the properties
of base::File.  When an OS-independent abstraction contains parts that are
specific to only a subset of all supported OSes, I thought it might be useful to
document that.

[rvargas (doing something else), 2015-03-16 19:57:14]

Thanks for writing this.

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

https://codereview.chromium.org/1000393003/diff/1/base/files/file.h#newcode144
base/files/file.h:144: // True if the file corresponds to a directory.  Always
false on Windows.
On 2015/03/16 16:28:39, Thiemo Nagel wrote:
> On 2015/03/13 18:44:20, rvargas wrote:
> > To be clear, if the File points to a directory, GetInfo should return true.
I
> > think it does, it doesn't, it sounds like a bug.
> 
> I thought you couldn't even open a directory in Windows, i.e. IsValid() should
> be false for base::File("c:\windows")?  So you can't even call GetInfo() on a
> directory on Windows?

It requires File::Flags::FLAG_BACKUP_SEMANTICS (to be renamed and made
multi-platform)

> 
> At least when I tried opening a directory on a Windows bot, it failed.  Also I
> can't find code in file_win.cc which sets is_directory to true.  But maybe I'm
> missing something crucial here?

File::GetInfo() (line 287)

https://codereview.chromium.org/1000393003/diff/1/base/files/file.h#newcode148
base/files/file.h:148: // Windows.
On 2015/03/16 16:28:39, Thiemo Nagel wrote:
> On 2015/03/13 18:37:00, rvargas wrote:
> > Reflects the current implementation, but may not be what we want from the
> > interface. I don't have a use case to fix the implementation though (which
is
> > why I have never bothered). Where does it matter? (just curious)
> 
> There is no specific use case.  It's just me trying to understand the
properties
> of base::File.  When an OS-independent abstraction contains parts that are
> specific to only a subset of all supported OSes, I thought it might be useful
to
> document that.

Could you make the comment state that this is an implementation detail rather
than a contract (as in, it's easy enough to fix for whoever actually needs that
info in Windows). I'm not sure if what I'm saying makes sense... but if we ever
fix this I don't want to have to worry about current callers assuming it is
false because the doc says so.

[Thiemo Nagel, 2015-03-17 19:45:53]

Patchset #3 (id:40001) has been deleted

[Thiemo Nagel, 2015-03-17 19:48:36]

Thank you for your comments.  I hope it's correct now...

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

https://codereview.chromium.org/1000393003/diff/1/base/files/file.h#newcode144
base/files/file.h:144: // True if the file corresponds to a directory.  Always
false on Windows.
> File::GetInfo() (line 287)

I have overlooked that.  :(

https://codereview.chromium.org/1000393003/diff/1/base/files/file.h#newcode148
base/files/file.h:148: // Windows.
On 2015/03/16 19:57:14, rvargas wrote:
> On 2015/03/16 16:28:39, Thiemo Nagel wrote:
> > On 2015/03/13 18:37:00, rvargas wrote:
> > > Reflects the current implementation, but may not be what we want from the
> > > interface. I don't have a use case to fix the implementation though (which
> is
> > > why I have never bothered). Where does it matter? (just curious)
> > 
> > There is no specific use case.  It's just me trying to understand the
> properties
> > of base::File.  When an OS-independent abstraction contains parts that are
> > specific to only a subset of all supported OSes, I thought it might be
useful
> to
> > document that.
> 
> Could you make the comment state that this is an implementation detail rather
> than a contract (as in, it's easy enough to fix for whoever actually needs
that
> info in Windows). I'm not sure if what I'm saying makes sense... but if we
ever
> fix this I don't want to have to worry about current callers assuming it is
> false because the doc says so.

Done.

[rvargas (doing something else), 2015-03-17 21:18:16]

lgtm

[Thiemo Nagel, 2015-03-17 21:19:04]

On 2015/03/17 21:18:16, rvargas wrote:
> lgtm

Thank you!

[Thiemo Nagel, 2015-03-17 21:19:07]

The CQ bit was checked by tnagel@chromium.org

[commit-bot: I haz the power, 2015-03-17 21:25:03]

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

[commit-bot: I haz the power, 2015-03-17 22:39:58]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-03-17 22:40:23]

Patchset 3 (id:??) landed as
https://crrev.com/718a46e5e13e8bb8ab44c78909b7b6a042f46190
Cr-Commit-Position: refs/heads/master@{#321003}