Remove PlatformFile from fileapi::FileSystemFileUtil

Remove PlatformFile from fileapi::FileSystemFileUtil

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

[rvargas (doing something else), 2014-05-09 22:10:57]

PTAL

[kinuko, 2014-05-12 05:39:56]

https://codereview.chromium.org/274903002/diff/40001/content/browser/fileapi/...
File content/browser/fileapi/obfuscated_file_util_unittest.cc (left):

https://codereview.chromium.org/274903002/diff/40001/content/browser/fileapi/...
content/browser/fileapi/obfuscated_file_util_unittest.cc:351:
EXPECT_TRUE(base::ClosePlatformFile(file_handle));
I think this code probably expects a flush happens here (though closing a file
doesn't necessarily guarantee it). Could we explicitly reset file or call
file.Close() (or file.Flush()) to avoid potential flakiness?

https://codereview.chromium.org/274903002/diff/40001/webkit/browser/fileapi/a...
File webkit/browser/fileapi/async_file_util_adapter.cc (left):

https://codereview.chromium.org/274903002/diff/40001/webkit/browser/fileapi/a...
webkit/browser/fileapi/async_file_util_adapter.cc:160: const bool success =
base::FileUtilProxy::RelayCreateOrOpen(
I think this is the last callsite of RelayCreateOrOpen, we can likely remove it
too

https://codereview.chromium.org/274903002/diff/40001/webkit/browser/fileapi/a...
File webkit/browser/fileapi/async_file_util_adapter.cc (right):

https://codereview.chromium.org/274903002/diff/40001/webkit/browser/fileapi/a...
webkit/browser/fileapi/async_file_util_adapter.cc:151: base::File
deleter(platform_file);
nit: deleter -> closer might be better

https://codereview.chromium.org/274903002/diff/40001/webkit/browser/fileapi/o...
File webkit/browser/fileapi/obfuscated_file_util.cc (right):

https://codereview.chromium.org/274903002/diff/40001/webkit/browser/fileapi/o...
webkit/browser/fileapi/obfuscated_file_util.cc:1044: *db =
GetDirectoryDatabase(dest_url, true);
I'd probably remove this part from this method but just take *db as input param,
this method seems to be doing too many things that are not strongly related. 
Also... this looks to be the only callsite of GenerateNewLocalPath, so probably
it (GenerateNewLocalPath) could be extended to getting the root directory part
too, i.e.:

SandboxDirectoryDatabase* db = GetDirectoryDatabase(dest_url, true);
GenerateNewLocalPath(db, context, dest_url, &root, local_path);

https://codereview.chromium.org/274903002/diff/40001/webkit/browser/fileapi/o...
webkit/browser/fileapi/obfuscated_file_util.cc:1054: base::File::Error
ObfuscatedFileUtil::PostCreateFile(
The term 'Post' is a bit confusing (we often use it to refer to posting a task),
CommitCreateFile() maybe?

Also since this is basically to update the DB related code, I'd put *db param
first.

https://codereview.chromium.org/274903002/diff/40001/webkit/browser/fileapi/o...
File webkit/browser/fileapi/obfuscated_file_util.h (right):

https://codereview.chromium.org/274903002/diff/40001/webkit/browser/fileapi/o...
webkit/browser/fileapi/obfuscated_file_util.h:274: SandboxDirectoryDatabase*
db);
nit: could we move these methods declarations/definitions after
Create{,AndOpen}File?

[rvargas (doing something else), 2014-05-12 23:39:59]

Thanks!

https://codereview.chromium.org/274903002/diff/40001/content/browser/fileapi/...
File content/browser/fileapi/obfuscated_file_util_unittest.cc (left):

https://codereview.chromium.org/274903002/diff/40001/content/browser/fileapi/...
content/browser/fileapi/obfuscated_file_util_unittest.cc:351:
EXPECT_TRUE(base::ClosePlatformFile(file_handle));
On 2014/05/12 05:39:57, kinuko wrote:
> I think this code probably expects a flush happens here (though closing a file
> doesn't necessarily guarantee it). Could we explicitly reset file or call
> file.Close() (or file.Flush()) to avoid potential flakiness?

Done (IMO, if this is racing it needs more than Close/Flush to work :( ).

https://codereview.chromium.org/274903002/diff/40001/webkit/browser/fileapi/a...
File webkit/browser/fileapi/async_file_util_adapter.cc (left):

https://codereview.chromium.org/274903002/diff/40001/webkit/browser/fileapi/a...
webkit/browser/fileapi/async_file_util_adapter.cc:160: const bool success =
base::FileUtilProxy::RelayCreateOrOpen(
On 2014/05/12 05:39:57, kinuko wrote:
> I think this is the last callsite of RelayCreateOrOpen, we can likely remove
it
> too

Yes, I'm doing the cleanup after landing
https://codereview.chromium.org/252583007

https://codereview.chromium.org/274903002/diff/40001/webkit/browser/fileapi/a...
File webkit/browser/fileapi/async_file_util_adapter.cc (right):

https://codereview.chromium.org/274903002/diff/40001/webkit/browser/fileapi/a...
webkit/browser/fileapi/async_file_util_adapter.cc:151: base::File
deleter(platform_file);
On 2014/05/12 05:39:57, kinuko wrote:
> nit: deleter -> closer might be better

Done.

https://codereview.chromium.org/274903002/diff/40001/webkit/browser/fileapi/o...
File webkit/browser/fileapi/obfuscated_file_util.cc (right):

https://codereview.chromium.org/274903002/diff/40001/webkit/browser/fileapi/o...
webkit/browser/fileapi/obfuscated_file_util.cc:1044: *db =
GetDirectoryDatabase(dest_url, true);
On 2014/05/12 05:39:57, kinuko wrote:
> I'd probably remove this part from this method but just take *db as input
param,
> this method seems to be doing too many things that are not strongly related. 
> Also... this looks to be the only callsite of GenerateNewLocalPath, so
probably
> it (GenerateNewLocalPath) could be extended to getting the root directory part
> too, i.e.:
> 
> SandboxDirectoryDatabase* db = GetDirectoryDatabase(dest_url, true);
> GenerateNewLocalPath(db, context, dest_url, &root, local_path);

Done.

https://codereview.chromium.org/274903002/diff/40001/webkit/browser/fileapi/o...
webkit/browser/fileapi/obfuscated_file_util.cc:1054: base::File::Error
ObfuscatedFileUtil::PostCreateFile(
On 2014/05/12 05:39:57, kinuko wrote:
> The term 'Post' is a bit confusing (we often use it to refer to posting a
task),
> CommitCreateFile() maybe?
> 
> Also since this is basically to update the DB related code, I'd put *db param
> first.

Done. (db is now the first in_out argument)

https://codereview.chromium.org/274903002/diff/40001/webkit/browser/fileapi/o...
File webkit/browser/fileapi/obfuscated_file_util.h (right):

https://codereview.chromium.org/274903002/diff/40001/webkit/browser/fileapi/o...
webkit/browser/fileapi/obfuscated_file_util.h:274: SandboxDirectoryDatabase*
db);
On 2014/05/12 05:39:57, kinuko wrote:
> nit: could we move these methods declarations/definitions after
> Create{,AndOpen}File?

Done.

[kinuko, 2014-05-13 13:09:47]

lgtm

[rvargas (doing something else), 2014-05-13 17:56:41]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2014-05-13 17:56:56]

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

[commit-bot: I haz the power, 2014-05-13 21:39:51]

Change committed as 270214