Wired support for file truncating with RemoteFileSystemOperation::OpenFile() method (case when base…

Wired support for file truncating and opening in R/W mode to  RemoteFileSystemOperation::OpenFile() method.

BUG=133879
TEST=coming...

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

[zel, 2012-06-27 00:37:38]

Please review GData layer only (open+truncate part), I will remove file close
notification business once you land your CL at
http://codereview.chromium.org/10541113/

[kinaba, 2012-06-27 09:17:21]

http://codereview.chromium.org/10600013/diff/9020/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_file_system_proxy.cc (right):

http://codereview.chromium.org/10600013/diff/9020/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system_proxy.cc:125: void
OpenAndTruncateOnIOPool(
I think we can simply use OpenPlatformFileOnIOPool() also for truncation case.
Passing base::PLATFORM_FILE_OPEN_TRUNCATED to base::CreatePlatformFile should
automatically truncate the file to size-0.

The reason I needed to call base::TruncatePlatformFile in DoTruncateOnFileThread
is for handling non-zero truncation.

http://codereview.chromium.org/10600013/diff/9020/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system_proxy.cc:355: if (file_flags &
base::PLATFORM_FILE_OPEN_TRUNCATED) {
As noted above, I don't think this branching is necessary.

http://codereview.chromium.org/10600013/diff/9020/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system_proxy.cc:461: (file_flags &
base::PLATFORM_FILE_DELETE_ON_CLOSE)) {
base::PLATFORM_FILE_OPEN_ALWAYS (if exist=>open else=>create) is an error, too.

http://codereview.chromium.org/10600013/diff/9020/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system_proxy.cc:494: }
It might be slightly safer to callback failure when none of the recognized
file_flags are set. Currently, say, if file_flags==0, then no feedback is
returned to the caller. (Though it should never happen if callers are correctly
implemented...)

[zel, 2012-06-27 22:20:45]

http://codereview.chromium.org/10600013/diff/9020/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_file_system_proxy.cc (right):

http://codereview.chromium.org/10600013/diff/9020/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system_proxy.cc:125: void
OpenAndTruncateOnIOPool(
On 2012/06/27 09:17:21, kinaba wrote:
> I think we can simply use OpenPlatformFileOnIOPool() also for truncation case.
> Passing base::PLATFORM_FILE_OPEN_TRUNCATED to base::CreatePlatformFile should
> automatically truncate the file to size-0.
> 
> The reason I needed to call base::TruncatePlatformFile in
DoTruncateOnFileThread
> is for handling non-zero truncation.

I see. Done.

http://codereview.chromium.org/10600013/diff/9020/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system_proxy.cc:355: if (file_flags &
base::PLATFORM_FILE_OPEN_TRUNCATED) {
On 2012/06/27 09:17:21, kinaba wrote:
> As noted above, I don't think this branching is necessary.

Done.

http://codereview.chromium.org/10600013/diff/9020/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system_proxy.cc:461: (file_flags &
base::PLATFORM_FILE_DELETE_ON_CLOSE)) {
On 2012/06/27 09:17:21, kinaba wrote:
> base::PLATFORM_FILE_OPEN_ALWAYS (if exist=>open else=>create) is an error,
too.

Done.

http://codereview.chromium.org/10600013/diff/9020/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system_proxy.cc:494: }
On 2012/06/27 09:17:21, kinaba wrote:
> It might be slightly safer to callback failure when none of the recognized
> file_flags are set. Currently, say, if file_flags==0, then no feedback is
> returned to the caller. (Though it should never happen if callers are
correctly
> implemented...)

Done.

[kinaba, 2012-06-28 05:52:33]

Mostly looking good.

http://codereview.chromium.org/10600013/diff/23021/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_browser_handler_api.cc (left):

http://codereview.chromium.org/10600013/diff/23021/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_handler_api.cc:237: success =
DoCreateFile();
(Not sure if this part is intended to be a part of the landing CL, but just in
case.)

Can we remove this file creation safely?
I can imagine this would cause trouble for GData file system, but just removing
it looks to violate the documented behavior:
http://code.google.com/chrome/extensions/dev/fileBrowserHandler.html.

http://codereview.chromium.org/10600013/diff/23021/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_file_system_proxy.cc (right):

http://codereview.chromium.org/10600013/diff/23021/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system_proxy.cc:125: void
OpenAndTruncateOnIOPool(
This function is not used now. It can be removed.

[zel, 2012-06-28 17:50:45]

http://codereview.chromium.org/10600013/diff/23021/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_browser_handler_api.cc (left):

http://codereview.chromium.org/10600013/diff/23021/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_handler_api.cc:237: success =
DoCreateFile();
On 2012/06/28 05:52:34, kinaba wrote:
> (Not sure if this part is intended to be a part of the landing CL, but just in
> case.)
> 
> Can we remove this file creation safely?
> I can imagine this would cause trouble for GData file system, but just
removing
> it looks to violate the documented behavior:
> http://code.google.com/chrome/extensions/dev/fileBrowserHandler.html.

Yes, we have to remove this since file picker API will currently create a file
not just grant permissions to it.

http://codereview.chromium.org/10600013/diff/23021/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_file_system_proxy.cc (right):

http://codereview.chromium.org/10600013/diff/23021/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system_proxy.cc:125: void
OpenAndTruncateOnIOPool(
On 2012/06/28 05:52:34, kinaba wrote:
> This function is not used now. It can be removed.

done

[kinaba, 2012-06-29 01:48:08]

LGTM for GDataFileSystem part, with a comment.

http://codereview.chromium.org/10600013/diff/23021/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_browser_handler_api.cc (left):

http://codereview.chromium.org/10600013/diff/23021/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_handler_api.cc:237: success =
DoCreateFile();
On 2012/06/28 17:50:46, zel wrote:
> On 2012/06/28 05:52:34, kinaba wrote:
> > (Not sure if this part is intended to be a part of the landing CL, but just
in
> > case.)
> > 
> > Can we remove this file creation safely?
> > I can imagine this would cause trouble for GData file system, but just
> removing
> > it looks to violate the documented behavior:
> > http://code.google.com/chrome/extensions/dev/fileBrowserHandler.html.
> 
> Yes, we have to remove this since file picker API will currently create a file
> not just grant permissions to it. 

I see, but it changes the behavior of existing code for local filesystem,
it is documented to create a file, not just giving permissions.
We may want to change the spec of the API, or at least leave some comment here.
(It's odd that CreateFileOnFileThread is not creating a file).

[kinaba, 2012-07-10 11:33:16]

The close notification part has finally landed http://crrev.com/145856.
Sorry for keeping you waiting so long.

[tbarzic, 2012-07-17 22:53:48]

On 2012/07/10 11:33:16, kinaba wrote:
> The close notification part has finally landed http://crrev.com/145856.
> Sorry for keeping you waiting so long.

you'll have to do something like in order for
FileBrowserHandlerExtensionTest.EndToEnd to pass
https://chromiumcodereview.appspot.com/10664002/diff/7001/chrome/test/data/ex...

[zel, 2012-07-18 01:56:14]

On 2012/07/17 22:53:48, tbarzic wrote:
> On 2012/07/10 11:33:16, kinaba wrote:
> > The close notification part has finally landed http://crrev.com/145856.
> > Sorry for keeping you waiting so long.
> 
> you'll have to do something like in order for
> FileBrowserHandlerExtensionTest.EndToEnd to pass
>
https://chromiumcodereview.appspot.com/10664002/diff/7001/chrome/test/data/ex...

done.

[tbarzic, 2012-07-18 01:57:51]

On 2012/07/18 01:56:14, zel wrote:
> On 2012/07/17 22:53:48, tbarzic wrote:
> > On 2012/07/10 11:33:16, kinaba wrote:
> > > The close notification part has finally landed http://crrev.com/145856.
> > > Sorry for keeping you waiting so long.
> > 
> > you'll have to do something like in order for
> > FileBrowserHandlerExtensionTest.EndToEnd to pass
> >
>
https://chromiumcodereview.appspot.com/10664002/diff/7001/chrome/test/data/ex...
> 
> done.

lgtm

[commit-bot: I haz the power, 2012-07-18 06:00:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zelidrag@chromium.org/10600013/46002

[commit-bot: I haz the power, 2012-07-18 06:00:54]

Presubmit check for 10600013-46002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
You might be calling functions intended only for testing from
production code.  It is OK to ignore this warning if you know what
you are doing, as the heuristics used to detect the situation are
not perfect.  The commit queue will not block on this warning.
Email joi@chromium.org if you have questions.
  chrome/browser/chromeos/extensions/file_browser_handler_api.cc:62
    virtual void set_function_for_test( \
  chrome/browser/chromeos/extensions/file_browser_handler_api.cc:300
    result->set_function_for_test(this);

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/browser/fileapi

Presubmit checks took 1.1s to calculate.