[Refactor] to rename and re-layer the file_util stack layers.

[Refactor] to rename and re-layer the file_util stack layers.

Splits FileSystemFileUtil into two classes.
1. FileSystemFileUtil (pure virtual and non-virtual functions + functions to forward to an underlying file_util.)
2. NativeFileUtil (FileUtil for OS-native operations)

Renames *FileSystemFileUtil into *FileUtil except for FileSystemFileUtil.

Renames *_file_system_file_util.{cc,h} into *_file_util.{cc,h}.

Renames variable *_file_system_file_util into *_file_util.


BUG=none
TEST=All FileAPI-related tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=99641

[Dai Mikurube (NOT FULLTIME), 2011-07-29 11:35:43]

Splits FileSystemFileUtil into three classes.
1. FileUtil (pure virtual and non-virtual functions)
2. NativeFileUtil (FileUtil for OS-native operations)
3. OverlayFileUtil (the parent class for overlaying FileUtils)

Renames *FileSystemFileUtil into *FileUtil.

Renames *_file_system_file_util.{cc,h} into *_file_util.{cc,h}.

Renames variable *_file_system_file_util into *_file_util.


Applying only a part of this change is fine.

[Dai Mikurube (NOT FULLTIME), 2011-08-03 07:44:29]

I've splitted the base class FileSystemFileUtil and renamed some -FileUtil
classes in this change.
(***FileSystemFileUtil is too long!)

It's just a suggestion.  What do you think?

[ericu, 2011-08-03 22:59:26]

I see that this makes a slightly more elegant inheritance/composition hierarchy,
but it's also more code and a bunch of churn.  What's the goal of this change? 
Why is it necessary?

http://codereview.chromium.org/7470037/diff/10001/webkit/fileapi/fileapi_file...
File webkit/fileapi/fileapi_file_util.h (right):

http://codereview.chromium.org/7470037/diff/10001/webkit/fileapi/fileapi_file...
webkit/fileapi/fileapi_file_util.h:40: class FileUtil {
It's a bit odd to call the file fileapi_file_util.h and have the class be
FileUtil.  Perhaps rename the class?

http://codereview.chromium.org/7470037/diff/10001/webkit/fileapi/native_file_...
File webkit/fileapi/native_file_util.cc (right):

http://codereview.chromium.org/7470037/diff/10001/webkit/fileapi/native_file_...
webkit/fileapi/native_file_util.cc:3: // found in the LICENSE file.
This file seems to be based on file_system_file_util.cc, but the version control
system doesn't appear to know it.  Please use mv or cp to create this file [as
you did with all the other new files] so that the diffs show up properly and we
don't lose the history.

http://codereview.chromium.org/7470037/diff/10001/webkit/fileapi/native_file_...
webkit/fileapi/native_file_util.cc:18: bool
ParentExists(FileSystemOperationContext* context,
This function isn't used.

http://codereview.chromium.org/7470037/diff/10001/webkit/fileapi/native_file_...
File webkit/fileapi/native_file_util.h (right):

http://codereview.chromium.org/7470037/diff/10001/webkit/fileapi/native_file_...
webkit/fileapi/native_file_util.h:23: class NativeFileUtil : public FileUtil {
This needs a class comment that says what this particular subclass is used for.

http://codereview.chromium.org/7470037/diff/10001/webkit/fileapi/native_file_...
webkit/fileapi/native_file_util.h:41: const FilePath& file_path, bool* created);
Please add OVERRIDE tags on all overridden methods in all files.

[Dai Mikurube (NOT FULLTIME), 2011-08-04 03:54:48]

Thank you for your comments, Eric!

The main reasons of this change are :
* There are duplicated code for underlying file_utils in QuotaFU, LocalFSFU and
ObfuscatedFSFU.  They should be better to unify.
* It’s hard to tell common non-overridden functions among -FileUtils and
class-specific overridden functions in FileSystemFileUtil.  It loses readability
and maintainability.

For them, the "Patch Set 2" is required at least.

Renaming file names, variables and classes for "*FileSystemFileUtil" is just
nice to have.  It's a bunch of churn.

http://codereview.chromium.org/7470037/diff/10001/webkit/fileapi/fileapi_file...
File webkit/fileapi/fileapi_file_util.h (right):

http://codereview.chromium.org/7470037/diff/10001/webkit/fileapi/fileapi_file...
webkit/fileapi/fileapi_file_util.h:40: class FileUtil {
On 2011/08/03 22:59:26, ericu wrote:
> It's a bit odd to call the file fileapi_file_util.h and have the class be
> FileUtil.  Perhaps rename the class?

Agreed.  Renaming it to FileApiFileUtil.

http://codereview.chromium.org/7470037/diff/10001/webkit/fileapi/native_file_...
File webkit/fileapi/native_file_util.cc (right):

http://codereview.chromium.org/7470037/diff/10001/webkit/fileapi/native_file_...
webkit/fileapi/native_file_util.cc:3: // found in the LICENSE file.
On 2011/08/03 22:59:26, ericu wrote:
> This file seems to be based on file_system_file_util.cc, but the version
control
> system doesn't appear to know it.  Please use mv or cp to create this file [as
> you did with all the other new files] so that the diffs show up properly and
we
> don't lose the history.

Actually I used git mv for this class, but Rietveld took it like this for some
reason...
I'll try remaking the patch.

http://codereview.chromium.org/7470037/diff/10001/webkit/fileapi/native_file_...
webkit/fileapi/native_file_util.cc:18: bool
ParentExists(FileSystemOperationContext* context,
On 2011/08/03 22:59:26, ericu wrote:
> This function isn't used.

Exactly.  Removed.

http://codereview.chromium.org/7470037/diff/10001/webkit/fileapi/native_file_...
File webkit/fileapi/native_file_util.h (right):

http://codereview.chromium.org/7470037/diff/10001/webkit/fileapi/native_file_...
webkit/fileapi/native_file_util.h:23: class NativeFileUtil : public FileUtil {
On 2011/08/03 22:59:26, ericu wrote:
> This needs a class comment that says what this particular subclass is used
for.

Done.

http://codereview.chromium.org/7470037/diff/10001/webkit/fileapi/native_file_...
webkit/fileapi/native_file_util.h:41: const FilePath& file_path, bool* created);
On 2011/08/03 22:59:26, ericu wrote:
> Please add OVERRIDE tags on all overridden methods in all files.

Done.

[kinuko, 2011-08-04 04:21:05]

On Thu, Aug 4, 2011 at 12:54 PM, <dmikurube@chromium.org> wrote:

> Thank you for your comments, Eric!
>
> The main reasons of this change are :
> * There are duplicated code for underlying file_utils in QuotaFU, LocalFSFU
> and
> ObfuscatedFSFU.  They should be better to unify.
> * It’s hard to tell common non-overridden functions among -FileUtils and
> class-specific overridden functions in FileSystemFileUtil.  It loses
> readability
> and maintainability.
>
> For them, the "Patch Set 2" is required at least.
>

Mikurube-san, if it's possible I think you / we should show this next patch
or the final exit vision first for reviewers to properly review.  Again, as
I said before, it doesn't need to be *complete*.



> Renaming file names, variables and classes for "*FileSystemFileUtil" is
> just
> nice to have.  It's a bunch of churn.
>
>
>
> http://codereview.chromium.**org/7470037/diff/10001/webkit/**
>
fileapi/fileapi_file_util.h<http://codereview.chromium.org/7470037/diff/10001/webkit/fileapi/fileapi_file_util.h>
> File webkit/fileapi/fileapi_file_**util.h (right):
>
> http://codereview.chromium.**org/7470037/diff/10001/webkit/**
>
fileapi/fileapi_file_util.h#**newcode40<http://codereview.chromium.org/7470037/diff/10001/webkit/fileapi/fileapi_file_util.h#newcode40>
> webkit/fileapi/fileapi_file_**util.h:40: class FileUtil {
> On 2011/08/03 22:59:26, ericu wrote:
>
>> It's a bit odd to call the file fileapi_file_util.h and have the class
>>
> be
>
>> FileUtil.  Perhaps rename the class?
>>
>
> Agreed.  Renaming it to FileApiFileUtil.
>
>
> http://codereview.chromium.**org/7470037/diff/10001/webkit/**
>
fileapi/native_file_util.cc<http://codereview.chromium.org/7470037/diff/10001/webkit/fileapi/native_file_util.cc>
> File webkit/fileapi/native_file_**util.cc (right):
>
> http://codereview.chromium.**org/7470037/diff/10001/webkit/**
>
fileapi/native_file_util.cc#**newcode3<http://codereview.chromium.org/7470037/diff/10001/webkit/fileapi/native_file_util.cc#newcode3>
> webkit/fileapi/native_file_**util.cc:3: // found in the LICENSE file.
> On 2011/08/03 22:59:26, ericu wrote:
>
>> This file seems to be based on file_system_file_util.cc, but the
>>
> version control
>
>> system doesn't appear to know it.  Please use mv or cp to create this
>>
> file [as
>
>> you did with all the other new files] so that the diffs show up
>>
> properly and we
>
>> don't lose the history.
>>
>
> Actually I used git mv for this class, but Rietveld took it like this
> for some reason...
> I'll try remaking the patch.
>
>
> http://codereview.chromium.**org/7470037/diff/10001/webkit/**
>
fileapi/native_file_util.cc#**newcode18<http://codereview.chromium.org/7470037/diff/10001/webkit/fileapi/native_file_util.cc#newcode18>
> webkit/fileapi/native_file_**util.cc:18: bool
> ParentExists(**FileSystemOperationContext* context,
> On 2011/08/03 22:59:26, ericu wrote:
>
>> This function isn't used.
>>
>
> Exactly.  Removed.
>
>
> http://codereview.chromium.**org/7470037/diff/10001/webkit/**
>
fileapi/native_file_util.h<http://codereview.chromium.org/7470037/diff/10001/webkit/fileapi/native_file_util.h>
> File webkit/fileapi/native_file_**util.h (right):
>
> http://codereview.chromium.**org/7470037/diff/10001/webkit/**
>
fileapi/native_file_util.h#**newcode23<http://codereview.chromium.org/7470037/diff/10001/webkit/fileapi/native_file_util.h#newcode23>
> webkit/fileapi/native_file_**util.h:23: class NativeFileUtil : public
> FileUtil {
> On 2011/08/03 22:59:26, ericu wrote:
>
>> This needs a class comment that says what this particular subclass is
>>
> used for.
>
> Done.
>
>
> http://codereview.chromium.**org/7470037/diff/10001/webkit/**
>
fileapi/native_file_util.h#**newcode41<http://codereview.chromium.org/7470037/diff/10001/webkit/fileapi/native_file_util.h#newcode41>
> webkit/fileapi/native_file_**util.h:41: const FilePath& file_path, bool*
> created);
> On 2011/08/03 22:59:26, ericu wrote:
>
>> Please add OVERRIDE tags on all overridden methods in all files.
>>
>
> Done.
>
>
>
http://codereview.chromium.**org/7470037/<http://codereview.chromium.org/7470...
>

[kinuko, 2011-08-04 05:56:27]

Partial comments.  I'm wondering if we could address/combine multiple issues
into this one to simplify each change.

http://codereview.chromium.org/7470037/diff/10001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_file_util_proxy.cc (right):

http://codereview.chromium.org/7470037/diff/10001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_util_proxy.cc:5: #include
"webkit/fileapi/file_system_file_util_proxy.h"
should this also be renamed to fileapi_file_util_proxy.{h,cc}?

http://codereview.chromium.org/7470037/diff/14001/webkit/fileapi/overlay_file...
File webkit/fileapi/overlay_file_util.h (right):

http://codereview.chromium.org/7470037/diff/14001/webkit/fileapi/overlay_file...
webkit/fileapi/overlay_file_util.h:23: class OverlayFileUtil : public
FileApiFileUtil {
I'm feeling that probably we could combine this with the base FileUtil class? 
Like:

class FileAPIFileUtil {
 public:
  PlatformFileError OneMethod(...) {
    if (underlying_file_util()) {
      underlying_file_util()->OneMethod(...);
    }
    NOTREACHED();
  }

  // ...

 protected:
  FileAPIFileUtil* underlying_file_util() = 0;
};

Or I guess we could even combine the idea of LocalFU to reduce bunch of
duplicated code in the LFU?

class FileAPIFileUtil {
 public:
  PlatformFileError OneMethod(...) {
    if (underlying_file_util()) {
      underlying_file_util()->OneMethod(TranslatePath(path), ...);
    }
    NOTREACHED();
  }

  // ...

 protected:
  FileAPIFileUtil* underlying_file_util() = 0;
  FilePath TranslatePath() = 0;
};

[ericu, 2011-08-05 18:48:11]

Just checking--is this one of the refactorings that's going to be abandoned now?

[Dai Mikurube (NOT FULLTIME), 2011-08-08 02:34:06]

Thanks.

> Mikurube-san, if it's possible I think you / we should show this next patch
> or the final exit vision first for reviewers to properly review.  Again, as
> I said before, it doesn't need to be *complete*.

> Just checking--is this one of the refactorings that's going to be abandoned
now?

Hmm, this change has a separated issue from the other refactoring, and the
current patch is the exit change.  (I wrote it weeks ago.  Rebased, merged and
uploaded it last week.)

I can merge the "vision" of this change with other refactorings.  But I think it
makes the vision hard-to-understand.

http://codereview.chromium.org/7470037/diff/14001/webkit/fileapi/overlay_file...
File webkit/fileapi/overlay_file_util.h (right):

http://codereview.chromium.org/7470037/diff/14001/webkit/fileapi/overlay_file...
webkit/fileapi/overlay_file_util.h:23: class OverlayFileUtil : public
FileApiFileUtil {
IMHO, combining makes NativeFileUtil to have an invalid underlying_file_util.  I
guess reducing the number classes looks less reasonable than such
NativeFileUtil...

On 2011/08/04 05:56:28, kinuko wrote:
> I'm feeling that probably we could combine this with the base FileUtil class? 
> Like:
> 
> class FileAPIFileUtil {
>  public:
>   PlatformFileError OneMethod(...) {
>     if (underlying_file_util()) {
>       underlying_file_util()->OneMethod(...);
>     }
>     NOTREACHED();
>   }
> 
>   // ...
> 
>  protected:
>   FileAPIFileUtil* underlying_file_util() = 0;
> };
> 
> Or I guess we could even combine the idea of LocalFU to reduce bunch of
> duplicated code in the LFU?
> 
> class FileAPIFileUtil {
>  public:
>   PlatformFileError OneMethod(...) {
>     if (underlying_file_util()) {
>       underlying_file_util()->OneMethod(TranslatePath(path), ...);
>     }
>     NOTREACHED();
>   }
> 
>   // ...
> 
>  protected:
>   FileAPIFileUtil* underlying_file_util() = 0;
>   FilePath TranslatePath() = 0;
> };
>

[ericu, 2011-08-08 22:18:53]

On Sun, Aug 7, 2011 at 7:34 PM,  <dmikurube@chromium.org> wrote:
> Thanks.
>
>> Mikurube-san, if it's possible I think you / we should show this next
>> patch
>> or the final exit vision first for reviewers to properly review.  Again,
>> as
>> I said before, it doesn't need to be *complete*.
>
>> Just checking--is this one of the refactorings that's going to be
>> abandoned
>
> now?
>
> Hmm, this change has a separated issue from the other refactoring, and the
> current patch is the exit change.  (I wrote it weeks ago.  Rebased, merged
> and
> uploaded it last week.)
>
> I can merge the "vision" of this change with other refactorings.  But I
> think it
> makes the vision hard-to-understand.

How about just a writeup of what the vision is, as a document [with
specific details], not code?  That would be easier to understand, and
wouldn't make you put multiple changes in one CL.

>
http://codereview.chromium.org/7470037/diff/14001/webkit/fileapi/overlay_file...
> File webkit/fileapi/overlay_file_util.h (right):
>
>
http://codereview.chromium.org/7470037/diff/14001/webkit/fileapi/overlay_file...
> webkit/fileapi/overlay_file_util.h:23: class OverlayFileUtil : public
> FileApiFileUtil {
> IMHO, combining makes NativeFileUtil to have an invalid
> underlying_file_util.  I guess reducing the number classes looks less
> reasonable than such NativeFileUtil...
>
> On 2011/08/04 05:56:28, kinuko wrote:
>>
>> I'm feeling that probably we could combine this with the base FileUtil
>
> class?
>>
>> Like:
>
>> class FileAPIFileUtil {
>>  public:
>>   PlatformFileError OneMethod(...) {
>>     if (underlying_file_util()) {
>>       underlying_file_util()->OneMethod(...);
>>     }
>>     NOTREACHED();
>>   }
>
>>   // ...
>
>>  protected:
>>   FileAPIFileUtil* underlying_file_util() = 0;
>> };
>
>> Or I guess we could even combine the idea of LocalFU to reduce bunch
>
> of
>>
>> duplicated code in the LFU?
>
>> class FileAPIFileUtil {
>>  public:
>>   PlatformFileError OneMethod(...) {
>>     if (underlying_file_util()) {
>>       underlying_file_util()->OneMethod(TranslatePath(path), ...);
>>     }
>>     NOTREACHED();
>>   }
>
>>   // ...
>
>>  protected:
>>   FileAPIFileUtil* underlying_file_util() = 0;
>>   FilePath TranslatePath() = 0;
>> };
>
>
> http://codereview.chromium.org/7470037/
>

[Dai Mikurube (NOT FULLTIME), 2011-08-18 04:52:53]

Updated it with combined FileApiFU and OverlayFU.

[Dai Mikurube (NOT FULLTIME), 2011-08-22 09:53:36]

On 2011/08/18 04:52:53, Dai Mikurube wrote:
> Updated it with combined FileApiFU and OverlayFU.

Caught up to http://codereview.chromium.org/7608011/.


I'm thinking we can rename FileApiFileUtil back to FileSystemFileUtil with
NativeFU separated out.

At first, I named the abstract class "FileUtil" since
fileapi::FileSystemFileUtil was too long and redundant.  It had both "fileapi"
and "FileSystem".  But I named the files "fileapi_file_util.*" since file_util.*
existed in base/.  The same file names in different directories could cause
build confusion.  Finally, we renamed the class FileApiFileUtil since the
difference between the class and file were confusing.

Now, FileApiFileUtil is not so much better than FileSystemFileUtil.  Just 3
letters shorter.  I think we can rename it back in this change.


This change is ready for review except for FileApiFileUtil.  Could you please
take a look at this?

[ericu, 2011-08-22 17:59:36]

I'm happy with the idea of switching back to FSFU, as well.  As you say, the new
name isn't much shorter, so it's not worth the churn.

http://codereview.chromium.org/7470037/diff/26001/webkit/fileapi/fileapi_file...
File webkit/fileapi/fileapi_file_util.cc (right):

http://codereview.chromium.org/7470037/diff/26001/webkit/fileapi/fileapi_file...
webkit/fileapi/fileapi_file_util.cc:289: PlatformFileError
FileApiFileUtil::CreateOrOpen(
It's good for implementations of methods to match the same order as the
declarations in the header file.  If you want to group these together, you
should reorder the header file as well.  Of course, then the derived classes
should probably match the same order too, and all that churn makes it hard to
track the changes [or lack thereof] in the refactoring, but if you want to group
these, now's the time to do it, while we're cleaning everything else up.

http://codereview.chromium.org/7470037/diff/26001/webkit/fileapi/fileapi_file...
webkit/fileapi/fileapi_file_util.cc:333: NOTREACHED() << "Subclasses must
provide implementation if it has no"
s/it has/they have/g

http://codereview.chromium.org/7470037/diff/26001/webkit/fileapi/quota_file_u...
File webkit/fileapi/quota_file_util_unittest.cc (right):

http://codereview.chromium.org/7470037/diff/26001/webkit/fileapi/quota_file_u...
webkit/fileapi/quota_file_util_unittest.cc:19: #include
"webkit/fileapi/fileapi_file_util.h"
Is this include needed?

http://codereview.chromium.org/7470037/diff/26001/webkit/fileapi/sandbox_moun...
File webkit/fileapi/sandbox_mount_point_provider.cc (right):

http://codereview.chromium.org/7470037/diff/26001/webkit/fileapi/sandbox_moun...
webkit/fileapi/sandbox_mount_point_provider.cc:23: #include
"webkit/fileapi/local_file_util.h"
Is local_file_util.h needed?

http://codereview.chromium.org/7470037/diff/26001/webkit/tools/test_shell/sim...
File webkit/tools/test_shell/simple_file_system.cc (right):

http://codereview.chromium.org/7470037/diff/26001/webkit/tools/test_shell/sim...
webkit/tools/test_shell/simple_file_system.cc:27: #include
"webkit/fileapi/fileapi_file_util.h"
Not needed.

http://codereview.chromium.org/7470037/diff/26001/webkit/tools/test_shell/sim...
webkit/tools/test_shell/simple_file_system.cc:45: using
fileapi::FileApiFileUtil;
Not needed.

http://codereview.chromium.org/7470037/diff/26001/webkit/tools/test_shell/sim...
File webkit/tools/test_shell/simple_file_writer.cc (right):

http://codereview.chromium.org/7470037/diff/26001/webkit/tools/test_shell/sim...
webkit/tools/test_shell/simple_file_writer.cc:13: #include
"webkit/fileapi/fileapi_file_util.h"
Not needed.

http://codereview.chromium.org/7470037/diff/26001/webkit/tools/test_shell/sim...
webkit/tools/test_shell/simple_file_writer.cc:17: using
fileapi::FileApiFileUtil;
Not needed.

[kinuko, 2011-08-24 07:50:31]

On 2011/08/22 17:59:36, ericu wrote:
> I'm happy with the idea of switching back to FSFU, as well.  As you say, the
new
> name isn't much shorter, so it's not worth the churn.

+1 to switch back to FSFU, changing FSFU to FileApiFileUtil is making this
change look really big while the # of chars we can save is not that much.

>
http://codereview.chromium.org/7470037/diff/26001/webkit/fileapi/fileapi_file...
> File webkit/fileapi/fileapi_file_util.cc (right):
> 
>
http://codereview.chromium.org/7470037/diff/26001/webkit/fileapi/fileapi_file...
> webkit/fileapi/fileapi_file_util.cc:289: PlatformFileError
> FileApiFileUtil::CreateOrOpen(
> It's good for implementations of methods to match the same order as the
> declarations in the header file.  If you want to group these together, you
> should reorder the header file as well.  Of course, then the derived classes
> should probably match the same order too, and all that churn makes it hard to
> track the changes [or lack thereof] in the refactoring, but if you want to
group
> these, now's the time to do it, while we're cleaning everything else up.
> 
>
http://codereview.chromium.org/7470037/diff/26001/webkit/fileapi/fileapi_file...
> webkit/fileapi/fileapi_file_util.cc:333: NOTREACHED() << "Subclasses must
> provide implementation if it has no"
> s/it has/they have/g
> 
>
http://codereview.chromium.org/7470037/diff/26001/webkit/fileapi/quota_file_u...
> File webkit/fileapi/quota_file_util_unittest.cc (right):
> 
>
http://codereview.chromium.org/7470037/diff/26001/webkit/fileapi/quota_file_u...
> webkit/fileapi/quota_file_util_unittest.cc:19: #include
> "webkit/fileapi/fileapi_file_util.h"
> Is this include needed?
> 
>
http://codereview.chromium.org/7470037/diff/26001/webkit/fileapi/sandbox_moun...
> File webkit/fileapi/sandbox_mount_point_provider.cc (right):
> 
>
http://codereview.chromium.org/7470037/diff/26001/webkit/fileapi/sandbox_moun...
> webkit/fileapi/sandbox_mount_point_provider.cc:23: #include
> "webkit/fileapi/local_file_util.h"
> Is local_file_util.h needed?
> 
>
http://codereview.chromium.org/7470037/diff/26001/webkit/tools/test_shell/sim...
> File webkit/tools/test_shell/simple_file_system.cc (right):
> 
>
http://codereview.chromium.org/7470037/diff/26001/webkit/tools/test_shell/sim...
> webkit/tools/test_shell/simple_file_system.cc:27: #include
> "webkit/fileapi/fileapi_file_util.h"
> Not needed.
> 
>
http://codereview.chromium.org/7470037/diff/26001/webkit/tools/test_shell/sim...
> webkit/tools/test_shell/simple_file_system.cc:45: using
> fileapi::FileApiFileUtil;
> Not needed.
> 
>
http://codereview.chromium.org/7470037/diff/26001/webkit/tools/test_shell/sim...
> File webkit/tools/test_shell/simple_file_writer.cc (right):
> 
>
http://codereview.chromium.org/7470037/diff/26001/webkit/tools/test_shell/sim...
> webkit/tools/test_shell/simple_file_writer.cc:13: #include
> "webkit/fileapi/fileapi_file_util.h"
> Not needed.
> 
>
http://codereview.chromium.org/7470037/diff/26001/webkit/tools/test_shell/sim...
> webkit/tools/test_shell/simple_file_writer.cc:17: using
> fileapi::FileApiFileUtil;
> Not needed.

[Dai Mikurube (NOT FULLTIME), 2011-08-24 11:57:30]

Eric, Kinuko-san,

Thank you.  Switched FileApiFileUtil back to FileSystemFileUtil, and reordered
the member functions.

http://codereview.chromium.org/7470037/diff/26001/webkit/fileapi/fileapi_file...
File webkit/fileapi/fileapi_file_util.cc (right):

http://codereview.chromium.org/7470037/diff/26001/webkit/fileapi/fileapi_file...
webkit/fileapi/fileapi_file_util.cc:289: PlatformFileError
FileApiFileUtil::CreateOrOpen(
On 2011/08/22 17:59:37, ericu wrote:
> It's good for implementations of methods to match the same order as the
> declarations in the header file.  If you want to group these together, you
> should reorder the header file as well.  Of course, then the derived classes
> should probably match the same order too, and all that churn makes it hard to
> track the changes [or lack thereof] in the refactoring, but if you want to
group
> these, now's the time to do it, while we're cleaning everything else up.

Agreed, and reordered the functions.

http://codereview.chromium.org/7470037/diff/26001/webkit/fileapi/fileapi_file...
webkit/fileapi/fileapi_file_util.cc:333: NOTREACHED() << "Subclasses must
provide implementation if it has no"
On 2011/08/22 17:59:37, ericu wrote:
> s/it has/they have/g

Done.

http://codereview.chromium.org/7470037/diff/26001/webkit/fileapi/quota_file_u...
File webkit/fileapi/quota_file_util_unittest.cc (right):

http://codereview.chromium.org/7470037/diff/26001/webkit/fileapi/quota_file_u...
webkit/fileapi/quota_file_util_unittest.cc:19: #include
"webkit/fileapi/fileapi_file_util.h"
On 2011/08/22 17:59:37, ericu wrote:
> Is this include needed?

Done.

http://codereview.chromium.org/7470037/diff/26001/webkit/fileapi/sandbox_moun...
File webkit/fileapi/sandbox_mount_point_provider.cc (right):

http://codereview.chromium.org/7470037/diff/26001/webkit/fileapi/sandbox_moun...
webkit/fileapi/sandbox_mount_point_provider.cc:23: #include
"webkit/fileapi/local_file_util.h"
On 2011/08/22 17:59:37, ericu wrote:
> Is local_file_util.h needed?

Done.

http://codereview.chromium.org/7470037/diff/26001/webkit/tools/test_shell/sim...
File webkit/tools/test_shell/simple_file_system.cc (right):

http://codereview.chromium.org/7470037/diff/26001/webkit/tools/test_shell/sim...
webkit/tools/test_shell/simple_file_system.cc:27: #include
"webkit/fileapi/fileapi_file_util.h"
On 2011/08/22 17:59:37, ericu wrote:
> Not needed.

Done.

http://codereview.chromium.org/7470037/diff/26001/webkit/tools/test_shell/sim...
webkit/tools/test_shell/simple_file_system.cc:45: using
fileapi::FileApiFileUtil;
On 2011/08/22 17:59:37, ericu wrote:
> Not needed.

Done.

http://codereview.chromium.org/7470037/diff/26001/webkit/tools/test_shell/sim...
File webkit/tools/test_shell/simple_file_writer.cc (right):

http://codereview.chromium.org/7470037/diff/26001/webkit/tools/test_shell/sim...
webkit/tools/test_shell/simple_file_writer.cc:13: #include
"webkit/fileapi/fileapi_file_util.h"
On 2011/08/22 17:59:37, ericu wrote:
> Not needed.

Done.

http://codereview.chromium.org/7470037/diff/26001/webkit/tools/test_shell/sim...
webkit/tools/test_shell/simple_file_writer.cc:17: using
fileapi::FileApiFileUtil;
On 2011/08/22 17:59:37, ericu wrote:
> Not needed.

Done.

[ericu, 2011-08-25 17:58:05]

Getting very close now!

I've assumed that all the changes were the reorderings we spoke about; I didn't
check line-by-line this time.

http://codereview.chromium.org/7470037/diff/44001/webkit/chromeos/fileapi/cro...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

http://codereview.chromium.org/7470037/diff/44001/webkit/chromeos/fileapi/cro...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:19: #include
"webkit/chromeos/fileapi/file_access_permissions.h"
Why don't we need an include of local_file_util.h?

http://codereview.chromium.org/7470037/diff/44001/webkit/fileapi/native_file_...
File webkit/fileapi/native_file_util_unittest.cc (right):

http://codereview.chromium.org/7470037/diff/44001/webkit/fileapi/native_file_...
webkit/fileapi/native_file_util_unittest.cc:52: // ObfuscatedFileUtil's unit
test.
This file should have stayed as file_system_file_util_unittest.cc; it's about
the superclass and its implementations, not about the native filesystem support.

Of course, we do still need unit tests for NativeFileUtil, and that's worth a
comment in NativeFileUtil.h or .cc.

http://codereview.chromium.org/7470037/diff/44001/webkit/fileapi/quota_file_u...
File webkit/fileapi/quota_file_util.h (right):

http://codereview.chromium.org/7470037/diff/44001/webkit/fileapi/quota_file_u...
webkit/fileapi/quota_file_util.h:28: // Creates a QuotaFileUtil instance with an
underlying FileUtil instance.
Should say NativeFileUtil.

[Dai Mikurube (NOT FULLTIME), 2011-08-26 05:10:59]

Thanks.

http://codereview.chromium.org/7470037/diff/44001/webkit/chromeos/fileapi/cro...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

http://codereview.chromium.org/7470037/diff/44001/webkit/chromeos/fileapi/cro...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:19: #include
"webkit/chromeos/fileapi/file_access_permissions.h"
On 2011/08/25 17:58:05, ericu wrote:
> Why don't we need an include of local_file_util.h?

I guess since it's included in cros_mount_point_provider.h.

http://codereview.chromium.org/7470037/diff/44001/webkit/fileapi/native_file_...
File webkit/fileapi/native_file_util_unittest.cc (right):

http://codereview.chromium.org/7470037/diff/44001/webkit/fileapi/native_file_...
webkit/fileapi/native_file_util_unittest.cc:52: // ObfuscatedFileUtil's unit
test.
On 2011/08/25 17:58:05, ericu wrote:
> This file should have stayed as file_system_file_util_unittest.cc; it's about
> the superclass and its implementations, not about the native filesystem
support.
> 
> Of course, we do still need unit tests for NativeFileUtil, and that's worth a
> comment in NativeFileUtil.h or .cc.

Ah, sure.  Done.

http://codereview.chromium.org/7470037/diff/44001/webkit/fileapi/quota_file_u...
File webkit/fileapi/quota_file_util.h (right):

http://codereview.chromium.org/7470037/diff/44001/webkit/fileapi/quota_file_u...
webkit/fileapi/quota_file_util.h:28: // Creates a QuotaFileUtil instance with an
underlying FileUtil instance.
On 2011/08/25 17:58:05, ericu wrote:
> Should say NativeFileUtil.

Done.

[kinuko, 2011-08-29 07:19:59]

Mostly nits, I think it's looking good to me.

http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_file_util.h (right):

http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_util.h:6: #define WEBKIT_FILEAPI_FILE_UTIL_H_
This needs to be changed back to WEBKIT_FILEAPI_FILE_SYSTEM_FILE_UTIL_H_

(Thanks for repeated changes)

http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_util.h:59: explicit
FileSystemFileUtil(FileSystemFileUtil* underlying_file_util);
Should these c-tor be protected?

http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_util.h:288: #endif  //
WEBKIT_FILEAPI_FILE_UTIL_H_
ditto.

http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/local_file_u...
File webkit/fileapi/local_file_util.h (right):

http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/local_file_u...
webkit/fileapi/local_file_util.h:50: 
while you're there could you remove all the empty lines between virtual methods?

http://www.chromium.org/developers/coding-style
"When you implement an interface, group those functions in your header file in
one section, clearly labeled as implementing that interface. Be sure to preserve
the order of the virtual base class, and do not add comments or whitespace
between them, as the reader can refer to the virtual base class for more
information."

http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/native_file_...
File webkit/fileapi/native_file_util.h (right):

http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/native_file_...
webkit/fileapi/native_file_util.h:24: // This class handles accessing to the OS
native filesystem.
nit: 'access to the OS' or 'accessing the OS'?

http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/native_file_...
webkit/fileapi/native_file_util.h:36: 
ditto about empty lines.  Thanks!

http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/obfuscated_f...
File webkit/fileapi/obfuscated_file_util.h (right):

http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_util.h:52: class AbstractOriginEnumerator {
I think this question is to Eric:
It's not related to this patch itself, but I wonder why we need an abstract
class defined here while its only implementation is ObfuscatedFSOEnumerator?  Am
I missing something or maybe it was introduced due to some historical reason?

http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/quota_file_u...
File webkit/fileapi/quota_file_util.h (right):

http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/quota_file_u...
webkit/fileapi/quota_file_util.h:36: 
ditto... could we remove empty lines between the virtual methods?

[Dai Mikurube (NOT FULLTIME), 2011-08-30 07:47:26]

Thank you.

http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_file_util.h (right):

http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_util.h:6: #define WEBKIT_FILEAPI_FILE_UTIL_H_
On 2011/08/29 07:20:00, kinuko wrote:
> This needs to be changed back to WEBKIT_FILEAPI_FILE_SYSTEM_FILE_UTIL_H_
> 
> (Thanks for repeated changes)

Done.

http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_util.h:59: explicit
FileSystemFileUtil(FileSystemFileUtil* underlying_file_util);
On 2011/08/29 07:20:00, kinuko wrote:
> Should these c-tor be protected?

Done.

http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_util.h:288: #endif  //
WEBKIT_FILEAPI_FILE_UTIL_H_
On 2011/08/29 07:20:00, kinuko wrote:
> ditto.

Done.

http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/local_file_u...
File webkit/fileapi/local_file_util.h (right):

http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/local_file_u...
webkit/fileapi/local_file_util.h:50: 
On 2011/08/29 07:20:00, kinuko wrote:
> while you're there could you remove all the empty lines between virtual
methods?
> 
> http://www.chromium.org/developers/coding-style
> "When you implement an interface, group those functions in your header file in
> one section, clearly labeled as implementing that interface. Be sure to
preserve
> the order of the virtual base class, and do not add comments or whitespace
> between them, as the reader can refer to the virtual base class for more
> information."

Done.

http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/native_file_...
File webkit/fileapi/native_file_util.h (right):

http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/native_file_...
webkit/fileapi/native_file_util.h:24: // This class handles accessing to the OS
native filesystem.
On 2011/08/29 07:20:00, kinuko wrote:
> nit: 'access to the OS' or 'accessing the OS'?

Done.

http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/native_file_...
webkit/fileapi/native_file_util.h:36: 
On 2011/08/29 07:20:00, kinuko wrote:
> ditto about empty lines.  Thanks!

Done.

http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/obfuscated_f...
File webkit/fileapi/obfuscated_file_util.h (right):

http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_util.h:52: class AbstractOriginEnumerator {
On 2011/08/29 07:20:00, kinuko wrote:
> I think this question is to Eric:
> It's not related to this patch itself, but I wonder why we need an abstract
> class defined here while its only implementation is ObfuscatedFSOEnumerator? 
Am
> I missing something or maybe it was introduced due to some historical reason?

Ah, I was considering it, too.  What do you think, Eric?
We can handle it in another patch if a change is required.

http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/quota_file_u...
File webkit/fileapi/quota_file_util.h (right):

http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/quota_file_u...
webkit/fileapi/quota_file_util.h:36: 
On 2011/08/29 07:20:00, kinuko wrote:
> ditto... could we remove empty lines between the virtual methods?

Done.

[ericu, 2011-08-30 17:31:37]

On Tue, Aug 30, 2011 at 12:47 AM,  <dmikurube@chromium.org> wrote:
> Thank you.
>
>
>
http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/file_system_...
> File webkit/fileapi/file_system_file_util.h (right):
>
>
http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/file_system_...
> webkit/fileapi/file_system_file_util.h:6: #define
> WEBKIT_FILEAPI_FILE_UTIL_H_
> On 2011/08/29 07:20:00, kinuko wrote:
>>
>> This needs to be changed back to
>
> WEBKIT_FILEAPI_FILE_SYSTEM_FILE_UTIL_H_
>
>> (Thanks for repeated changes)
>
> Done.
>
>
http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/file_system_...
> webkit/fileapi/file_system_file_util.h:59: explicit
> FileSystemFileUtil(FileSystemFileUtil* underlying_file_util);
> On 2011/08/29 07:20:00, kinuko wrote:
>>
>> Should these c-tor be protected?
>
> Done.
>
>
http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/file_system_...
> webkit/fileapi/file_system_file_util.h:288: #endif  //
> WEBKIT_FILEAPI_FILE_UTIL_H_
> On 2011/08/29 07:20:00, kinuko wrote:
>>
>> ditto.
>
> Done.
>
>
http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/local_file_u...
> File webkit/fileapi/local_file_util.h (right):
>
>
http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/local_file_u...
> webkit/fileapi/local_file_util.h:50:
> On 2011/08/29 07:20:00, kinuko wrote:
>>
>> while you're there could you remove all the empty lines between
>
> virtual methods?
>
>> http://www.chromium.org/developers/coding-style
>> "When you implement an interface, group those functions in your header
>
> file in
>>
>> one section, clearly labeled as implementing that interface. Be sure
>
> to preserve
>>
>> the order of the virtual base class, and do not add comments or
>
> whitespace
>>
>> between them, as the reader can refer to the virtual base class for
>
> more
>>
>> information."
>
> Done.
>
>
http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/native_file_...
> File webkit/fileapi/native_file_util.h (right):
>
>
http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/native_file_...
> webkit/fileapi/native_file_util.h:24: // This class handles accessing to
> the OS native filesystem.
> On 2011/08/29 07:20:00, kinuko wrote:
>>
>> nit: 'access to the OS' or 'accessing the OS'?
>
> Done.
>
>
http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/native_file_...
> webkit/fileapi/native_file_util.h:36:
> On 2011/08/29 07:20:00, kinuko wrote:
>>
>> ditto about empty lines.  Thanks!
>
> Done.
>
>
http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/obfuscated_f...
> File webkit/fileapi/obfuscated_file_util.h (right):
>
>
http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/obfuscated_f...
> webkit/fileapi/obfuscated_file_util.h:52: class AbstractOriginEnumerator
> {
> On 2011/08/29 07:20:00, kinuko wrote:
>>
>> I think this question is to Eric:
>> It's not related to this patch itself, but I wonder why we need an
>
> abstract
>>
>> class defined here while its only implementation is
>
> ObfuscatedFSOEnumerator?  Am
>>
>> I missing something or maybe it was introduced due to some historical
>
> reason?
>
> Ah, I was considering it, too.  What do you think, Eric?
> We can handle it in another patch if a change is required.

It's not really necessary.  If we were going to have multiple sandbox
implementation [some obfuscated, some not] we'd want that abstract
interface, just like we have one for files, but since we don't have
that any more, this should go away in another CL.

>
http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/quota_file_u...
> File webkit/fileapi/quota_file_util.h (right):
>
>
http://codereview.chromium.org/7470037/diff/51010/webkit/fileapi/quota_file_u...
> webkit/fileapi/quota_file_util.h:36:
> On 2011/08/29 07:20:00, kinuko wrote:
>>
>> ditto... could we remove empty lines between the virtual methods?
>
> Done.
>
> http://codereview.chromium.org/7470037/
>

[ericu, 2011-08-30 17:33:17]

I think you let two files slip in here by accident from another CL;
path_obfuscator.{cc,h} are new this revision.

The rest LGTM, as long as you drop those files out.  If you actually meant to
add them to this CL, let me know and I'll review them.

Sorry I took so long on this and the other review; somehow I lost track of them
in my email.  I'll be more careful in the future.

[Dai Mikurube (NOT FULLTIME), 2011-08-31 07:47:24]

On 2011/08/30 17:33:17, ericu wrote:
> I think you let two files slip in here by accident from another CL;
> path_obfuscator.{cc,h} are new this revision.

Thanks for your good catch.  You're right.
Removed them two.

> 
> The rest LGTM, as long as you drop those files out.  If you actually meant to
> add them to this CL, let me know and I'll review them.
> 
> Sorry I took so long on this and the other review; somehow I lost track of
them
> in my email.  I'll be more careful in the future.

Don't mind.  I appreciate your detailed comments on this change. :)

[Dai Mikurube (NOT FULLTIME), 2011-09-01 05:29:34]

Rebased and submitted to try bots.

[Dai Mikurube (NOT FULLTIME), 2011-09-05 07:39:22]

Rebased.  Kinuko-san, could you PTAL at this change?

[kinuko, 2011-09-05 08:06:34]

On 2011/09/05 07:39:22, Dai Mikurube wrote:
> Rebased.  Kinuko-san, could you PTAL at this change?

LGTM (at a quick glance at the rebased changes)

[Dai Mikurube (NOT FULLTIME), 2011-09-05 11:05:28]

On 2011/09/05 08:06:34, kinuko wrote:
> On 2011/09/05 07:39:22, Dai Mikurube wrote:
> > Rebased.  Kinuko-san, could you PTAL at this change?
> 
> LGTM (at a quick glance at the rebased changes)

Thanks.  Checked the "Commit" box!

[commit-bot: I haz the power, 2011-09-05 11:05:31]

Can't process patch for file webkit/fileapi/local_file_util_unittest.cc.
A +