Implement IsNonNativeLocalPathDirectory for supporting non-local non-Drive directory access in apps.

Implement IsNonNativeLocalPathDirectory for supporting non-local non-Drive directory access in apps.

What the CL does is:
* Almost mechanically moves out the local function CheckIfDirectoryExists
from open_util.cc to fileapi_util.cc and just use it for the desired implementation.
* Removes Drive-specific helper code for the feature.
* Allows file_system_api test setup code to refer the info of File Manager app.
(Because the new general implementation checks the directory existence in the
permission level of the file manager, the test needs access to it.)

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

[kinaba, 2014-05-30 03:44:59]

@mtomasz: */drive/* (just deleting unused code) and */file_manager/*
@benwells: file_system_apitest_chromeos.cc

[mtomasz, 2014-05-30 03:53:44]

https://codereview.chromium.org/304373004/diff/10006/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_manager/filesystem_api_util.cc (right):

https://codereview.chromium.org/304373004/diff/10006/chrome/browser/chromeos/...
chrome/browser/chromeos/file_manager/filesystem_api_util.cc:130:
content::BrowserThread::PostTask(content::BrowserThread::UI,
This PostTask looks unnecessary, since we are already on UI.

[kinaba, 2014-05-30 04:07:01]

https://codereview.chromium.org/304373004/diff/10006/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_manager/filesystem_api_util.cc (right):

https://codereview.chromium.org/304373004/diff/10006/chrome/browser/chromeos/...
chrome/browser/chromeos/file_manager/filesystem_api_util.cc:130:
content::BrowserThread::PostTask(content::BrowserThread::UI,
On 2014/05/30 03:53:44, mtomasz wrote:
> This PostTask looks unnecessary, since we are already on UI.

This is intentional to make all callbacks done asynchronously.

The current client code looks safe, but in some cases it sometimes causes
trouble if callback.Run() is synchronously called here before exiting the caller
function (unexpected stack overflow, early destruction of objects in the
callback before the caller losing the access to it, etc.)

[mtomasz, 2014-05-30 05:23:48]

https://codereview.chromium.org/304373004/diff/10006/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_manager/filesystem_api_util.cc (right):

https://codereview.chromium.org/304373004/diff/10006/chrome/browser/chromeos/...
chrome/browser/chromeos/file_manager/filesystem_api_util.cc:130:
content::BrowserThread::PostTask(content::BrowserThread::UI,
On 2014/05/30 04:07:01, kinaba wrote:
> On 2014/05/30 03:53:44, mtomasz wrote:
> > This PostTask looks unnecessary, since we are already on UI.
> 
> This is intentional to make all callbacks done asynchronously.
> 
> The current client code looks safe, but in some cases it sometimes causes
> trouble if callback.Run() is synchronously called here before exiting the
caller
> function (unexpected stack overflow, early destruction of objects in the
> callback before the caller losing the access to it, etc.)

Hm. Isn't it an overkill? Do we always do that? If so, shall we add a comment?

[kinaba, 2014-05-30 05:43:00]

https://codereview.chromium.org/304373004/diff/10006/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_manager/filesystem_api_util.cc (right):

https://codereview.chromium.org/304373004/diff/10006/chrome/browser/chromeos/...
chrome/browser/chromeos/file_manager/filesystem_api_util.cc:130:
content::BrowserThread::PostTask(content::BrowserThread::UI,
On 2014/05/30 05:23:48, mtomasz wrote:
> On 2014/05/30 04:07:01, kinaba wrote:
> > On 2014/05/30 03:53:44, mtomasz wrote:
> > > This PostTask looks unnecessary, since we are already on UI.
> > 
> > This is intentional to make all callbacks done asynchronously.
> > 
> > The current client code looks safe, but in some cases it sometimes causes
> > trouble if callback.Run() is synchronously called here before exiting the
> caller
> > function (unexpected stack overflow, early destruction of objects in the
> > callback before the caller losing the access to it, etc.)
> 
> Hm. Isn't it an overkill? Do we always do that? If so, shall we add a comment?

We always do it at least in the Drive code (see, e.g., crbug.com/139446 the
"We only need to do this ..." paragraph), and conditional sync/async callback
had actually caused issues in the past (like codereview.chromium.org/18033003/)
in other places as well.

Added a comment for clarification.

[mtomasz, 2014-05-30 05:55:06]

lgtm

https://codereview.chromium.org/304373004/diff/10006/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_manager/filesystem_api_util.cc (right):

https://codereview.chromium.org/304373004/diff/10006/chrome/browser/chromeos/...
chrome/browser/chromeos/file_manager/filesystem_api_util.cc:130:
content::BrowserThread::PostTask(content::BrowserThread::UI,
On 2014/05/30 05:43:00, kinaba wrote:
> On 2014/05/30 05:23:48, mtomasz wrote:
> > On 2014/05/30 04:07:01, kinaba wrote:
> > > On 2014/05/30 03:53:44, mtomasz wrote:
> > > > This PostTask looks unnecessary, since we are already on UI.
> > > 
> > > This is intentional to make all callbacks done asynchronously.
> > > 
> > > The current client code looks safe, but in some cases it sometimes causes
> > > trouble if callback.Run() is synchronously called here before exiting the
> > caller
> > > function (unexpected stack overflow, early destruction of objects in the
> > > callback before the caller losing the access to it, etc.)
> > 
> > Hm. Isn't it an overkill? Do we always do that? If so, shall we add a
comment?
> 
> We always do it at least in the Drive code (see, e.g., crbug.com/139446 the
> "We only need to do this ..." paragraph), and conditional sync/async callback
> had actually caused issues in the past (like
codereview.chromium.org/18033003/)
> in other places as well.
> 
> Added a comment for clarification.

Got it. I'll review File System Provider API code to follow the same pattern
then.

[benwells, 2014-06-02 01:31:33]

test lgtm

[kinaba, 2014-06-02 01:32:40]

The CQ bit was checked by kinaba@chromium.org

[commit-bot: I haz the power, 2014-06-02 01:33:03]

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

[commit-bot: I haz the power, 2014-06-02 03:31:04]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-06-02 05:50:17]

Change committed as 274173