Remove thread restriction of fileapi::FileSystemContext::ResolveURL.

Remove thread restriction of fileapi::FileSystemContext::ResolveURL.

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

[kinaba, 2014-04-18 05:31:59]

Patch Set 1 takes a different way from originally discussion in
https://codereview.chromium.org/242493002/. Here's why (comments are welcome):

There are three options here.
1) Relay to IO thread in fileapi_util,
2) Relay to IO thread in FileSystemContext, or
3) Relay to IO thread in SandboxFileSystemBackend(Delegate).

As far as I understand 3) was the approach mainly discussed in the thread,
but I'm not quite convinced. Currently all methods of FileSystemBackend
(excluding
those introduced in ExternalFileSystemBackend) are called from IO thread, and
I believe all subclass instances are implicitly assuming the
single-threadedness.
That is, there's no lock or no thread forwarding, etc. Introducing
multi-threaded
access to this level and let each backend handle it separately when needed
doesn't
sound scaling (even though SandboxFileSystemBackend is the only affected class
as of now.)

I alternatively implemented 2). FileSystemContext somehow already handles
multi-threaded
access (in Shutdown(), or CrackFileSystemURL is also called from everywhere.)
Thus I humbly
thought it can tolerate one more such.

WDYT?

[mtomasz, 2014-04-18 07:00:10]

https://codereview.chromium.org/242443004/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/file_manager/fileapi_util.cc (right):

https://codereview.chromium.org/242443004/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_manager/fileapi_util.cc:182:
root_virtual_path.IsParent(iterator->virtual_path));
FYI, this fails in tests.

[kinuko, 2014-04-18 07:35:02]

lgtm, from fileapi pov I can live with this code.

https://codereview.chromium.org/242443004/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/file_manager/fileapi_util.cc (right):

https://codereview.chromium.org/242443004/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_manager/fileapi_util.cc:144: }
Looks like this code will no longer be necessary

[nhiroki, 2014-04-21 01:40:20]

LGTM. Thank you for working on this!

[kinaba, 2014-04-21 04:51:40]

@kinuko, @nhiroki, @mtomasz, thanks for the reviews. Could you take one more
look for the (largely) revised patch?

The test failures in the initial patch were fundamental.

* Patch Set 2 fixes half of the issues, the easier part: we needed to grant
permissions before doing ResolveURL operation.

* Patch Set 3 is for the rest. Files.app sometimes wants to call ResolveURL for
non-existing entries. Concretely speaking, for sending "changed" notification
for deleted directory, and for returning the "save" dialog result when the user
chose a fresh file name to save.

The last patch set allows FSC::ResolveURL to success even if the entry does not
exist. Let me know if it does not make sense.

https://codereview.chromium.org/242443004/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/file_manager/fileapi_util.cc (right):

https://codereview.chromium.org/242443004/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_manager/fileapi_util.cc:144: }
On 2014/04/18 07:35:02, kinuko wrote:
> Looks like this code will no longer be necessary

Done.

[mtomasz, 2014-04-21 05:03:04]

https://codereview.chromium.org/242443004/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_manager/fileapi_util.cc (right):

https://codereview.chromium.org/242443004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_manager/fileapi_util.cc:158:
fileapi::FileSystemContext::ResolvedEntryType type) {
I'm wondering if introducing a new argument is necessary. Why not just using
base::File::FILE_ERROR_NOT_FOUND instead, and removing the #161 if?

[kinaba, 2014-04-21 05:19:08]

https://codereview.chromium.org/242443004/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_manager/fileapi_util.cc (right):

https://codereview.chromium.org/242443004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_manager/fileapi_util.cc:158:
fileapi::FileSystemContext::ResolvedEntryType type) {
On 2014/04/21 05:03:05, mtomasz wrote:
> I'm wondering if introducing a new argument is necessary. Why not just using
> base::File::FILE_ERROR_NOT_FOUND instead, and removing the #161 if?

I think "ERROR but output value fields (like |entry_definition|) filled" state
is less common in Chromium and thus confusing. Even worse, FILE_ERROR_NOT_FOUND
may be returned either from FileSystemBackend or GetMetadata, and the latter is
the only case I want to treat as success.

[mtomasz, 2014-04-21 05:48:33]

https://codereview.chromium.org/242443004/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_manager/fileapi_util.cc (right):

https://codereview.chromium.org/242443004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_manager/fileapi_util.cc:158:
fileapi::FileSystemContext::ResolvedEntryType type) {
On 2014/04/21 05:19:09, kinaba wrote:
> On 2014/04/21 05:03:05, mtomasz wrote:
> > I'm wondering if introducing a new argument is necessary. Why not just using
> > base::File::FILE_ERROR_NOT_FOUND instead, and removing the #161 if?
> 
> I think "ERROR but output value fields (like |entry_definition|) filled" state
> is less common in Chromium and thus confusing. Even worse,
FILE_ERROR_NOT_FOUND
> may be returned either from FileSystemBackend or GetMetadata, and the latter
is
> the only case I want to treat as success.

My concern was that, if we returned an error, then the implementation of
FileSystemContext::ResolveURL() would be much easier. It would just return error
in case of an error. With this patch, it is quite complex. If the file doesn't
exist, then it will return FILE_OK, but with RESOLVED_ENTRY_NOT_FOUND, with is
dangerously similar to FILE_ERROR_NOT_FOUND.

If we just returned File::FILE_ERROR_NOT_FOUND, then we could move the tricky
logic (treating not found as success) to this converter class, and hence keeping
FileSystemContext simple.

But I'm not very familiar with the FileSystemContext class, so if you prefer the
current solution, feel free to go ahead. I don't have hard feelings here.

[mtomasz, 2014-04-21 05:50:01]

https://codereview.chromium.org/242443004/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_manager/fileapi_util.cc (right):

https://codereview.chromium.org/242443004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_manager/fileapi_util.cc:175: entry_definition.error
= error;
nit: This is always FILE_OK here, so how about inlining FILE_OK?

[nhiroki, 2014-04-21 07:01:04]

lgtm2 for the fileapi part

[kinaba, 2014-04-21 07:59:24]

Updated.

@kinuko, what do you think about the point Tomasz and I am discussing?
Both work fine for Files.app side but which is preferable in FileSystem API
side?

If we took the approach as Tomasz suggested, 
it'll look like https://codereview.chromium.org/244673003/.

https://codereview.chromium.org/242443004/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_manager/fileapi_util.cc (right):

https://codereview.chromium.org/242443004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_manager/fileapi_util.cc:158:
fileapi::FileSystemContext::ResolvedEntryType type) {
On 2014/04/21 05:48:33, mtomasz wrote:
> On 2014/04/21 05:19:09, kinaba wrote:
> > On 2014/04/21 05:03:05, mtomasz wrote:
> > > I'm wondering if introducing a new argument is necessary. Why not just
using
> > > base::File::FILE_ERROR_NOT_FOUND instead, and removing the #161 if?
> > 
> > I think "ERROR but output value fields (like |entry_definition|) filled"
state
> > is less common in Chromium and thus confusing. Even worse,
> FILE_ERROR_NOT_FOUND
> > may be returned either from FileSystemBackend or GetMetadata, and the latter
> is
> > the only case I want to treat as success.
> 
> My concern was that, if we returned an error, then the implementation of
> FileSystemContext::ResolveURL() would be much easier. It would just return
error
> in case of an error. With this patch, it is quite complex. If the file doesn't
> exist, then it will return FILE_OK, but with RESOLVED_ENTRY_NOT_FOUND, with is
> dangerously similar to FILE_ERROR_NOT_FOUND.
> 
> If we just returned File::FILE_ERROR_NOT_FOUND, then we could move the tricky
> logic (treating not found as success) to this converter class, and hence
keeping
> FileSystemContext simple.
> 
> But I'm not very familiar with the FileSystemContext class, so if you prefer
the
> current solution, feel free to go ahead. I don't have hard feelings here.

My concern is that the suggested plan makes the API of FileSystemContext
complex (i.e., when the callback parameters are available or not?
previously they're available iff error code indicates success, but now not.)

https://codereview.chromium.org/242443004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_manager/fileapi_util.cc:175: entry_definition.error
= error;
On 2014/04/21 05:50:02, mtomasz wrote:
> nit: This is always FILE_OK here, so how about inlining FILE_OK? 

Done.

[mtomasz, 2014-04-21 08:04:07]

lgtm, but let's wait for @kinuko's point of view.

https://codereview.chromium.org/242443004/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_manager/fileapi_util.cc (right):

https://codereview.chromium.org/242443004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_manager/fileapi_util.cc:158:
fileapi::FileSystemContext::ResolvedEntryType type) {
On 2014/04/21 07:59:25, kinaba wrote:
> On 2014/04/21 05:48:33, mtomasz wrote:
> > On 2014/04/21 05:19:09, kinaba wrote:
> > > On 2014/04/21 05:03:05, mtomasz wrote:
> > > > I'm wondering if introducing a new argument is necessary. Why not just
> using
> > > > base::File::FILE_ERROR_NOT_FOUND instead, and removing the #161 if?
> > > 
> > > I think "ERROR but output value fields (like |entry_definition|) filled"
> state
> > > is less common in Chromium and thus confusing. Even worse,
> > FILE_ERROR_NOT_FOUND
> > > may be returned either from FileSystemBackend or GetMetadata, and the
latter
> > is
> > > the only case I want to treat as success.
> > 
> > My concern was that, if we returned an error, then the implementation of
> > FileSystemContext::ResolveURL() would be much easier. It would just return
> error
> > in case of an error. With this patch, it is quite complex. If the file
doesn't
> > exist, then it will return FILE_OK, but with RESOLVED_ENTRY_NOT_FOUND, with
is
> > dangerously similar to FILE_ERROR_NOT_FOUND.
> > 
> > If we just returned File::FILE_ERROR_NOT_FOUND, then we could move the
tricky
> > logic (treating not found as success) to this converter class, and hence
> keeping
> > FileSystemContext simple.
> > 
> > But I'm not very familiar with the FileSystemContext class, so if you prefer
> the
> > current solution, feel free to go ahead. I don't have hard feelings here.
> 
> My concern is that the suggested plan makes the API of FileSystemContext
> complex (i.e., when the callback parameters are available or not?
> previously they're available iff error code indicates success, but now not.)

Ah, now I understand your point of view. Yep, I think both solutions are not
perfect. Let see what's @kinuko's opinion on this.

[kinuko, 2014-04-21 08:56:02]

lgtm

https://codereview.chromium.org/242443004/diff/80001/webkit/browser/fileapi/f...
File webkit/browser/fileapi/file_system_context.h (right):

https://codereview.chromium.org/242443004/diff/80001/webkit/browser/fileapi/f...
webkit/browser/fileapi/file_system_context.h:217: // thread with a message loop.
|callback| is invoked on the caller thread.
So the new contract is: this returns OK if the given URL is 'resolvable', and
also returns ResolvedEntryType based on the actual resolved entry type... is
that the objective?  If so could we update the method comment to document that?

[kinaba, 2014-04-21 09:21:59]

https://codereview.chromium.org/242443004/diff/80001/webkit/browser/fileapi/f...
File webkit/browser/fileapi/file_system_context.h (right):

https://codereview.chromium.org/242443004/diff/80001/webkit/browser/fileapi/f...
webkit/browser/fileapi/file_system_context.h:217: // thread with a message loop.
|callback| is invoked on the caller thread.
On 2014/04/21 08:56:02, kinuko wrote:
> So the new contract is: this returns OK if the given URL is 'resolvable', and
> also returns ResolvedEntryType based on the actual resolved entry type... is
> that the objective?  If so could we update the method comment to document
that?

Exactly. Updated the comment.

[kinaba, 2014-04-21 09:22:26]

On 2014/04/21 09:21:59, kinaba wrote:
>
https://codereview.chromium.org/242443004/diff/80001/webkit/browser/fileapi/f...
> File webkit/browser/fileapi/file_system_context.h (right):
> 
>
https://codereview.chromium.org/242443004/diff/80001/webkit/browser/fileapi/f...
> webkit/browser/fileapi/file_system_context.h:217: // thread with a message
loop.
> |callback| is invoked on the caller thread.
> On 2014/04/21 08:56:02, kinuko wrote:
> > So the new contract is: this returns OK if the given URL is 'resolvable',
and
> > also returns ResolvedEntryType based on the actual resolved entry type... is
> > that the objective?  If so could we update the method comment to document
> that?
> 
> Exactly. Updated the comment.

Wait. Something is broken in the comment. Fixing...

[kinaba, 2014-04-21 09:25:20]

On 2014/04/21 09:22:26, kinaba wrote:
> On 2014/04/21 09:21:59, kinaba wrote:
> >
>
https://codereview.chromium.org/242443004/diff/80001/webkit/browser/fileapi/f...
> > File webkit/browser/fileapi/file_system_context.h (right):
> > 
> >
>
https://codereview.chromium.org/242443004/diff/80001/webkit/browser/fileapi/f...
> > webkit/browser/fileapi/file_system_context.h:217: // thread with a message
> loop.
> > |callback| is invoked on the caller thread.
> > On 2014/04/21 08:56:02, kinuko wrote:
> > > So the new contract is: this returns OK if the given URL is 'resolvable',
> and
> > > also returns ResolvedEntryType based on the actual resolved entry type...
is
> > > that the objective?  If so could we update the method comment to document
> > that?
> > 
> > Exactly. Updated the comment.
> 
> Wait. Something is broken in the comment. Fixing...

Done.

[kinuko, 2014-04-21 10:01:37]

https://codereview.chromium.org/242443004/diff/130001/webkit/browser/fileapi/...
File webkit/browser/fileapi/file_system_context.h (right):

https://codereview.chromium.org/242443004/diff/130001/webkit/browser/fileapi/...
webkit/browser/fileapi/file_system_context.h:216: // the entry referred by the
URL does not exist, but in that case
nit: this might be read it could always succeed, it'd be nice to say when it
succeeds or not.

[kinaba, 2014-04-22 00:32:54]

https://codereview.chromium.org/242443004/diff/130001/webkit/browser/fileapi/...
File webkit/browser/fileapi/file_system_context.h (right):

https://codereview.chromium.org/242443004/diff/130001/webkit/browser/fileapi/...
webkit/browser/fileapi/file_system_context.h:216: // the entry referred by the
URL does not exist, but in that case
On 2014/04/21 10:01:37, kinuko wrote:
> nit: this might be read it could always succeed, it'd be nice to say when it
> succeeds or not.

Done.

[kinaba, 2014-04-22 01:14:37]

The CQ bit was checked by kinaba@chromium.org

[commit-bot: I haz the power, 2014-04-22 01:14:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinaba@chromium.org/242443004/150001

[commit-bot: I haz the power, 2014-04-22 02:39:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-22 02:39:49]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_x64_rel

[kinaba, 2014-04-22 03:13:49]

The CQ bit was checked by kinaba@chromium.org

[commit-bot: I haz the power, 2014-04-22 03:14:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinaba@chromium.org/242443004/150001

[commit-bot: I haz the power, 2014-04-22 05:03:09]

Change committed as 265179