Update/improve base::File comments.

base/files/file.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef BASE_FILES_FILE_H_
 #define BASE_FILES_FILE_H_
 
 #include "build/build_config.h"
 #if defined(OS_WIN)
 #include <windows.h>
@@ skipping 110 matching lines @@
 
   // This explicit mapping matches both FILE_ on Windows and SEEK_ on Linux.
   enum Whence {
     FROM_BEGIN   = 0,
     FROM_CURRENT = 1,
     FROM_END     = 2
   };
 
   // Used to hold information about a given file.
   // If you add more fields to this structure (platform-specific fields are OK),
-  // make sure to update all functions that use it in file_util_{win|posix}.cc
-  // too, and the ParamTraits<base::PlatformFileInfo> implementation in
-  // chrome/common/common_param_traits.cc.

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

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.

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

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.

+  // make sure to update all functions that use it in file_util_{win|posix}.cc,
+  // too.
   struct BASE_EXPORT Info {
     Info();
     ~Info();
 #if defined(OS_POSIX)
     // Fills this struct with values from |stat_info|.
     void FromStat(const stat_wrapper_t& stat_info);
 #endif
 
     // The size of the file in bytes.  Undefined when is_directory is true.
     int64 size;
 
-    // True if the file corresponds to a directory.
+    // True if the file corresponds to a directory.  Always false on Windows.

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

I don't think this is true.

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

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.

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

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?

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

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

[Thiemo Nagel, 2015/03/17 19:48:35]

I have overlooked that.  :(

     bool is_directory;
 
-    // True if the file corresponds to a symbolic link.
+    // True if the file corresponds to a symbolic link.  Always false on
+    // Windows.

[rvargas (doing something else), 2015/03/13 18:37:00]

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]

(and I'm basically fine either way)

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

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]

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:48:35]

Done.

     bool is_symbolic_link;
 
     // The last modified time of a file.
     base::Time last_modified;
 
     // The last accessed time of a file.
     base::Time last_accessed;
 
     // The creation time of a file.
     base::Time creation_time;
@@ skipping 196 matching lines @@
 #endif
 
   Error error_details_;
   bool created_;
   bool async_;
 };
 
 }  // namespace base
 
 #endif  // BASE_FILES_FILE_H_