Not sure if I understand. Why shouldn't the tests call it from the background page? Is this forbidden by documentation, or not working in real life scenario?


chrome.fileSystem.chooseEntry is a stable API, and it's available to all Chrome packaged apps. It's quite a basic API, so I believe we shouldn't change the behavior out of blue now. We may break plenty of apps.

Today, chrome.fileSystem.chooseEntry is forbidden from being called from a background page, and returns the error shown in comment 1.

Check out this comment: https://cs.chromium.org/chromium/src/ui/file_manager/zip_archiver/js/compressor-foreground.js?type=cs&q=filesystem%5C.chooseentry+file:%5C.js+-file:test/+-file:tests/&l=9

There's a weird exception: it can be called from an *extension* (not a platform app) if there's a browser tab open somewhere. But in fact, allowing it from an extension was temporary, it's intended to be an app API: https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/file_system/file_system_api.cc?type=cs&q=file:file_system_api%5C.cc+chooseentryfunction+is_platform_app&l=565

This exception is causing problems moving the function out of Chrome, and it only seems to be necessary for this single test -- it's not used outside of the file manager's integration_tests AFAICT.

So, I'd like to remove this exception, but that means fixing the test to avoid calling chrome.fileSystem.chooseEntry from a background page -- or converting the test into a platform app, which would make much more sense to me since the file manager itself is a platform app.

That's right, it is deliberately forbidden from a background page. This is a security thing - choosing a file is a security action (access to files is based on it) and having a window that the file chooser is associated with shows the user what app is requesting a file.

If a background page could open files it could do so without any windows open, and it wouldn't be clear what app is opening the file.

I'd suggest we don't change that restriction to make this test work.

OK. I think the long-term solution is to convert the test to a platform app (which will require the test to find another way to call chooseEntry so it happens from an app window).

To unblock chrome.fileSystem, I'll create a delegate API function that will allow Chrome to continue using its exception for extensions, without affecting AppShell. (So there will be no effective change in Chrome. A background page of a platform app is still forbidden from opening a dialog.)

Assigning to CrOS-FilesApp-CodeHealth since this affects our tests.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d37f424f2d3c29701dbf6245734bb6f792289a0e

commit d37f424f2d3c29701dbf6245734bb6f792289a0e
Author: Trent Apted <tapted@chromium.org>
Date: Thu Jul 19 09:14:29 2018

Don't bother showing an unused Browser* for FileManagerBrowserTestBase.

This affects file manager tests, gallery tests, and others. With a few
exceptions, these tests have no use for the browser() that
StartupBrowserCreator creates during startup. Not creating one leaves
more time for the tests to do things they really care about before
potentially timing out.

In fact, _not_ creating the Browser surfaces an existing bug more
prominently. That is, some tests have an iceberg dependency on the
existence of this Browser* that they should never have had in the
first place.

Bug: 736930
Change-Id: I1030e41800f3d2e0e5868ecd64d70c7d30f47c28
Reviewed-on: https://chromium-review.googlesource.com/1117977
Reviewed-by: Noel Gordon <noel@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576427}
[modify] https://crrev.com/d37f424f2d3c29701dbf6245734bb6f792289a0e/chrome/browser/chromeos/file_manager/external_filesystem_apitest.cc
[modify] https://crrev.com/d37f424f2d3c29701dbf6245734bb6f792289a0e/chrome/browser/chromeos/file_manager/file_manager_browsertest.cc
[modify] https://crrev.com/d37f424f2d3c29701dbf6245734bb6f792289a0e/chrome/browser/chromeos/file_manager/file_manager_browsertest_base.cc
[modify] https://crrev.com/d37f424f2d3c29701dbf6245734bb6f792289a0e/chrome/browser/chromeos/file_manager/file_manager_browsertest_base.h