Implement chrome.fileSystem.requestFileSystem().

Implement chrome.fileSystem.requestFileSystem().

This CL moves the private chrome.fileManagerPrivate.requestFileSystem to the
new namespace with the following differences:
- It returns an isolated file system (instead of an external).
- It's much simplified.

It's currently enabled to component apps only. This restriction will be released
in the upcoming CL.

TEST=Tested partially by existing browser tests.
BUG=323422
R=benwells@chromium.org, fukino@chromium.org, hirono@chromium.org, isherman@chromium.org, xiyuan@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/26fbaa282e764f3e340332d12a7032a488e0a41a

[mtomasz, 2015-03-17 06:56:13]

Patchset #7 (id:120001) has been deleted

[mtomasz, 2015-03-17 06:58:25]

mtomasz@chromium.org changed reviewers:
+ benwells@chromium.org, hirono@chromium.org, isherman@chromium.org,
kinuko@chromium.org

[mtomasz, 2015-03-17 06:59:04]

On 2015/03/17 06:58:25, mtomasz wrote:
> mailto:mtomasz@chromium.org changed reviewers:
> + mailto:benwells@chromium.org, mailto:hirono@chromium.org,
mailto:isherman@chromium.org,
> mailto:kinuko@chromium.org

PTAL.

kinuko:
  storage/browser/fileapi/file_system_backend.h
benwells:
  chrome/browser/extensions/api/file_handlers/app_file_handler_util.cc
  chrome/common/extensions/api/_permission_features.json
  chrome/common/extensions/api/file_system.idl
  chrome/renderer/resources/extensions/file_manager_private_custom_bindings.js
  chrome/renderer/resources/extensions/file_system_custom_bindings.js
isherman:
  extensions/browser/extension_function_histogram_value.h
  tools/metrics/histograms/histograms.xml
fukino:
  third_party/closure_compiler/externs/chrome_extensions.js
  third_party/closure_compiler/externs/file_manager_private.js
hirono:
  Everything else.

Thanks.

[mtomasz, 2015-03-17 06:59:41]

mtomasz@chromium.org changed reviewers:
+ fukino@chromium.org

[fukino, 2015-03-17 07:20:46]

chrome_extensions.js will be overridden by future compiler updates
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/closur...),
so you need to update the original file.

[hirono, 2015-03-18 03:41:05]

https://codereview.chromium.org/985533004/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/985533004/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:1026: LOG(ERROR) <<
params->options.volume_id << ": " << writable;
nit: Debug log.

https://codereview.chromium.org/985533004/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:1067:
OnConsentReceived(volume_info.volume_id, writable,
Just confirmation, if OnConsentRecived called directly, SendResponse or
SendError calls before this function returns RespondLater(). Is it OK?

https://codereview.chromium.org/985533004/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:1102: #if
defined(OS_CHROMEOS)
How about enclosing entire RequsetContext and OnConsentReceived?

https://codereview.chromium.org/985533004/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:1148: std::string
register_name = "fs";
Do we need to assign "fs"?

[Ilya Sherman, 2015-03-18 23:46:41]

histograms.xml lgtm

[mtomasz, 2015-03-19 01:17:20]

https://codereview.chromium.org/985533004/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/985533004/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:1026: LOG(ERROR) <<
params->options.volume_id << ": " << writable;
On 2015/03/18 03:41:05, hirono wrote:
> nit: Debug log.

Done.

https://codereview.chromium.org/985533004/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:1067:
OnConsentReceived(volume_info.volume_id, writable,
On 2015/03/18 03:41:05, hirono wrote:
> Just confirmation, if OnConsentRecived called directly, SendResponse or
> SendError calls before this function returns RespondLater(). Is it OK?

Good catch. It works as RespondLater() simply returns a no-on object, but it may
change. Reworked the logic. Done.

https://codereview.chromium.org/985533004/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:1102: #if
defined(OS_CHROMEOS)
On 2015/03/18 03:41:05, hirono wrote:
> How about enclosing entire RequsetContext and OnConsentReceived?

Done.

https://codereview.chromium.org/985533004/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:1148: std::string
register_name = "fs";
On 2015/03/18 03:41:05, hirono wrote:
> Do we need to assign "fs"?

Without it we would leak the mount point path, we may (or may not) be sensitive.
I added a comment.

[mtomasz, 2015-03-19 01:26:03]

On 2015/03/19 01:17:20, mtomasz wrote:
>
https://codereview.chromium.org/985533004/diff/140001/chrome/browser/extensio...
> File chrome/browser/extensions/api/file_system/file_system_api.cc (right):
> 
>
https://codereview.chromium.org/985533004/diff/140001/chrome/browser/extensio...
> chrome/browser/extensions/api/file_system/file_system_api.cc:1026: LOG(ERROR)
<<
> params->options.volume_id << ": " << writable;
> On 2015/03/18 03:41:05, hirono wrote:
> > nit: Debug log.
> 
> Done.
> 
>
https://codereview.chromium.org/985533004/diff/140001/chrome/browser/extensio...
> chrome/browser/extensions/api/file_system/file_system_api.cc:1067:
> OnConsentReceived(volume_info.volume_id, writable,
> On 2015/03/18 03:41:05, hirono wrote:
> > Just confirmation, if OnConsentRecived called directly, SendResponse or
> > SendError calls before this function returns RespondLater(). Is it OK?
> 
> Good catch. It works as RespondLater() simply returns a no-on object, but it
may
> change. Reworked the logic. Done.
> 
>
https://codereview.chromium.org/985533004/diff/140001/chrome/browser/extensio...
> chrome/browser/extensions/api/file_system/file_system_api.cc:1102: #if
> defined(OS_CHROMEOS)
> On 2015/03/18 03:41:05, hirono wrote:
> > How about enclosing entire RequsetContext and OnConsentReceived?
> 
> Done.
> 
>
https://codereview.chromium.org/985533004/diff/140001/chrome/browser/extensio...
> chrome/browser/extensions/api/file_system/file_system_api.cc:1148: std::string
> register_name = "fs";
> On 2015/03/18 03:41:05, hirono wrote:
> > Do we need to assign "fs"?
> 
> Without it we would leak the mount point path, we may (or may not) be
sensitive.
> I added a comment.

@fukino: I created cr/88989342 for the externs chrome_extension.js file.

[fukino, 2015-03-19 03:13:59]

lgtm with a nit for third_party/closure_compiler/externs/*

https://codereview.chromium.org/985533004/diff/180001/third_party/closure_com...
File third_party/closure_compiler/externs/chrome_extensions.js (right):

https://codereview.chromium.org/985533004/diff/180001/third_party/closure_com...
third_party/closure_compiler/externs/chrome_extensions.js:6830: * @typedef {{
nit: "@typeof {?{" should be better because it makes RequestFileSystemOptions
nullable like other real types. See: line #102 of this file.

[hirono, 2015-03-19 03:33:54]

lgtm, thanks!

https://codereview.chromium.org/985533004/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/file_system_api.h (right):

https://codereview.chromium.org/985533004/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.h:13: #include
"base/memory/ref_counted.h"
nit: Is this ref_counted used?

https://codereview.chromium.org/985533004/diff/180001/chrome/common/extension...
File chrome/common/extensions/api/file_system.idl (right):

https://codereview.chromium.org/985533004/diff/180001/chrome/common/extension...
chrome/common/extensions/api/file_system.idl:119: DOMString volumeId;
nit: Please fix indent.

https://codereview.chromium.org/985533004/diff/180001/ui/file_manager/image_l...
File ui/file_manager/image_loader/image_loader.js (right):

https://codereview.chromium.org/985533004/diff/180001/ui/file_manager/image_l...
ui/file_manager/image_loader/image_loader.js:30: if (event.eventType == 'mount'
&& event.status == 'success') {
nit: === are preferred.

[kinuko, 2015-03-19 10:00:45]

looks like no change in storage/browser/fileapi/file_system_backend.h, so
changes in storage/browser/fileapi/file_system_backend.h l/g/t/m :)

[mtomasz, 2015-03-19 10:21:55]

On 2015/03/19 10:00:45, kinuko wrote:
> looks like no change in storage/browser/fileapi/file_system_backend.h, so
> changes in storage/browser/fileapi/file_system_backend.h l/g/t/m :)

Oops, right!

[mtomasz, 2015-03-19 10:22:09]

mtomasz@chromium.org changed reviewers:
- kinuko@chromium.org

[mtomasz, 2015-03-19 10:28:41]

https://codereview.chromium.org/985533004/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/file_system_api.h (right):

https://codereview.chromium.org/985533004/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.h:13: #include
"base/memory/ref_counted.h"
On 2015/03/19 03:33:54, hirono wrote:
> nit: Is this ref_counted used?

Not at all. Done.

https://codereview.chromium.org/985533004/diff/180001/chrome/common/extension...
File chrome/common/extensions/api/file_system.idl (right):

https://codereview.chromium.org/985533004/diff/180001/chrome/common/extension...
chrome/common/extensions/api/file_system.idl:119: DOMString volumeId;
On 2015/03/19 03:33:54, hirono wrote:
> nit: Please fix indent.

Done.

https://codereview.chromium.org/985533004/diff/180001/third_party/closure_com...
File third_party/closure_compiler/externs/chrome_extensions.js (right):

https://codereview.chromium.org/985533004/diff/180001/third_party/closure_com...
third_party/closure_compiler/externs/chrome_extensions.js:6830: * @typedef {{
On 2015/03/19 03:13:59, fukino wrote:
> nit: "@typeof {?{" should be better because it makes RequestFileSystemOptions
> nullable like other real types. See: line #102 of this file.

Done. I'll update the original repo too.

https://codereview.chromium.org/985533004/diff/180001/ui/file_manager/image_l...
File ui/file_manager/image_loader/image_loader.js (right):

https://codereview.chromium.org/985533004/diff/180001/ui/file_manager/image_l...
ui/file_manager/image_loader/image_loader.js:30: if (event.eventType == 'mount'
&& event.status == 'success') {
On 2015/03/19 03:33:54, hirono wrote:
> nit: === are preferred.

Done.

[benwells, 2015-03-20 00:42:40]

https://codereview.chromium.org/985533004/diff/200001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/985533004/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:84: const char
kNotSupportedError[] = "Operation not supported.";
It would be good to provide users with info on why. Can you split this into two
strings: 'Operation not supported on the current platform.' and 'Operation only
supported in kiosk mode.'?

https://codereview.chromium.org/985533004/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:1031: // Only kiosk
apps in kiosk sessions can use this API. Additionally component
This check (kiosk mode) can go up before the code to get volume manager /
writable.

https://codereview.chromium.org/985533004/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:1034:
!KioskModeInfo::IsKioskEnabled(extension())) &&
I don't understand kiosk mode that well but remember hearing that there are
different varieties: one where a user can log in a normal chromebook in a kiosk
session, and one where the device has been converted to a single use kiosk
device.

Is that right? If so, is this API meant to work in both kiosk modes?

+xiyuan who can correct my understanding as required.

https://codereview.chromium.org/985533004/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:1068:
extension()->location() != Manifest::COMPONENT;
Right now this will never be true due to the check above. Is that right?

https://codereview.chromium.org/985533004/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:1097:
callback.Run(false);
I think you should use PostTask here, as you're calling RespondLater in the Run
function - and also for consistency with what you'll do once this is
implemented.

https://codereview.chromium.org/985533004/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:1141:
SetError(kSecurityError);
Is this the correct error?

https://codereview.chromium.org/985533004/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:1152:
GURL("chrome-extension://" + extension_id()),
I think there is a constant you can use for the scheme.

https://codereview.chromium.org/985533004/diff/200001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/file_system_api.h (right):

https://codereview.chromium.org/985533004/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.h:20: class
FileSystemContext;
Is this used?

https://codereview.chromium.org/985533004/diff/200001/chrome/common/extension...
File chrome/common/extensions/api/file_system.idl (right):

https://codereview.chromium.org/985533004/diff/200001/chrome/common/extension...
chrome/common/extensions/api/file_system.idl:185: // [nodoc] static void
getVolumeList(GetVolumeListCallback callback);
Why is this commented out?

https://codereview.chromium.org/985533004/diff/200001/chrome/common/extension...
chrome/common/extensions/api/file_system.idl:220: [nodoc] static void
onVolumeListChanged();
Any reason why the volume list isn't passed in?

More importantly, is this implemented?

[mtomasz, 2015-03-20 01:56:01]

https://codereview.chromium.org/985533004/diff/200001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/985533004/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:84: const char
kNotSupportedError[] = "Operation not supported.";
On 2015/03/20 00:42:39, benwells wrote:
> It would be good to provide users with info on why. Can you split this into
two
> strings: 'Operation not supported on the current platform.' and 'Operation
only
> supported in kiosk mode.'?

Done.

https://codereview.chromium.org/985533004/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:1031: // Only kiosk
apps in kiosk sessions can use this API. Additionally component
On 2015/03/20 00:42:39, benwells wrote:
> This check (kiosk mode) can go up before the code to get volume manager /
> writable.

Done.

https://codereview.chromium.org/985533004/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:1034:
!KioskModeInfo::IsKioskEnabled(extension())) &&
On 2015/03/20 00:42:39, benwells wrote:
> I don't understand kiosk mode that well but remember hearing that there are
> different varieties: one where a user can log in a normal chromebook in a
kiosk
> session, and one where the device has been converted to a single use kiosk
> device.
> 
> Is that right? If so, is this API meant to work in both kiosk modes?
> 
> +xiyuan who can correct my understanding as required.

Yes, the API is meant to work in both modes, but for auto-launch (aka. converted
to a single use kiosk device) there will be no confirmation dialog in opposite
to manual-launch which will show the confirmation dialog. See upcoming
crbug.com/1023673002/

https://codereview.chromium.org/985533004/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:1068:
extension()->location() != Manifest::COMPONENT;
On 2015/03/20 00:42:39, benwells wrote:
> Right now this will never be true due to the check above. Is that right?

Not really. It will be true for kiosk apps launched in not-auto-launch (aka
manual) mode.

https://codereview.chromium.org/985533004/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:1097:
callback.Run(false);
On 2015/03/20 00:42:39, benwells wrote:
> I think you should use PostTask here, as you're calling RespondLater in the
Run
> function - and also for consistency with what you'll do once this is
> implemented.

Done.

https://codereview.chromium.org/985533004/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:1141:
SetError(kSecurityError);
On 2015/03/20 00:42:39, benwells wrote:
> Is this the correct error?

GetVirtualPath() is not used to check if the volume still exist. Such check is
in #1119. GetVirtualPath() should fail only due to security restrictions. I
moved the above comment to a proper place, as it was confusing. Done.

https://codereview.chromium.org/985533004/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:1152:
GURL("chrome-extension://" + extension_id()),
On 2015/03/20 00:42:39, benwells wrote:
> I think there is a constant you can use for the scheme.

Done.

https://codereview.chromium.org/985533004/diff/200001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/file_system_api.h (right):

https://codereview.chromium.org/985533004/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.h:20: class
FileSystemContext;
On 2015/03/20 00:42:39, benwells wrote:
> Is this used?

Removed.

https://codereview.chromium.org/985533004/diff/200001/chrome/common/extension...
File chrome/common/extensions/api/file_system.idl (right):

https://codereview.chromium.org/985533004/diff/200001/chrome/common/extension...
chrome/common/extensions/api/file_system.idl:185: // [nodoc] static void
getVolumeList(GetVolumeListCallback callback);
On 2015/03/20 00:42:39, benwells wrote:
> Why is this commented out?

It shouldn't be here. I removed the idl which is not used yet.

https://codereview.chromium.org/985533004/diff/200001/chrome/common/extension...
chrome/common/extensions/api/file_system.idl:220: [nodoc] static void
onVolumeListChanged();
On 2015/03/20 00:42:39, benwells wrote:
> Any reason why the volume list isn't passed in?
> 
> More importantly, is this implemented?

We can pass. Let's do it in a separate CL. Removed.

[mtomasz, 2015-03-20 01:56:18]

mtomasz@chromium.org changed reviewers:
+ xiyuan@chromium.org

[mtomasz, 2015-03-20 01:56:57]

@xiyuan: PTAL at checks for kiosk mode detection (auto vs. manual) in
file_system_api.cc. Thanks.

[benwells, 2015-03-20 02:43:58]

Just the one question....

https://codereview.chromium.org/985533004/diff/200001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/985533004/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:1068:
extension()->location() != Manifest::COMPONENT;
On 2015/03/20 01:56:00, mtomasz wrote:
> On 2015/03/20 00:42:39, benwells wrote:
> > Right now this will never be true due to the check above. Is that right?
> 
> Not really. It will be true for kiosk apps launched in not-auto-launch (aka
> manual) mode.

Ohhh, I'd misread the logic above...

https://codereview.chromium.org/985533004/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/985533004/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:1029: // extensions
and apps, which is not documented though.
So the logic is, this can be used in kiosk mode OR by component extensions /
apps. Why do component extensions / apps need to use it?

[xiyuan, 2015-03-20 03:03:12]

https://codereview.chromium.org/985533004/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/985533004/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:1072:
!chromeos::KioskAppManager::Get()->IsAutoLaunchEnabled() &&
This name is a bit misleading. IsAutoLaunchEnabled returns whether it is allowed
to set auto launch for a kiosk app. Here, we want to find out whether the app is
auto launched. I thought it could be done via
KioskAppManager::IsCurrentAppAutoLaunchedWithZeroDelay and found that was never
implemented. :p and not exactly fit our needs here since we want to find out
whether a given app is auto launched. 

If you don't mind, could you update KioskAppManager to remove the never
implemented IsCurrentAppAutoLaunchedWithZeroDelay and add an
IsAppAutoLaunchedWithZeroDelay? Think we could just do the following:
bool KioskAppManager::IsAppAutoLaunchedWithZeroDelay(const std::string& app_id)
const {
  return !app_id.empty() && app_id ==
currently_auto_launched_with_zero_delay_app_;
}

Another way is to use existing api to get app info, which has a field tell you
whether it is auto launched. Something like:

KioskAppManager::App app_info;
KioskAppManager::Get()->GetApp(extension_id(), &app_info);
const bool is_auto_launched = app_info.was_auto_launched_with_zero_delay;

[mtomasz, 2015-03-20 03:26:23]

https://codereview.chromium.org/985533004/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/985533004/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:1029: // extensions
and apps, which is not documented though.
On 2015/03/20 02:43:58, benwells wrote:
> So the logic is, this can be used in kiosk mode OR by component extensions /
> apps. Why do component extensions / apps need to use it?

It's used by Files app, Gallery app, Audio player, Video player and Image loader
extension, which are all component extensions/apps in Chrome OS.

https://codereview.chromium.org/985533004/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:1072:
!chromeos::KioskAppManager::Get()->IsAutoLaunchEnabled() &&
On 2015/03/20 03:03:12, xiyuan wrote:
> This name is a bit misleading. IsAutoLaunchEnabled returns whether it is
allowed
> to set auto launch for a kiosk app. Here, we want to find out whether the app
is
> auto launched. I thought it could be done via
> KioskAppManager::IsCurrentAppAutoLaunchedWithZeroDelay and found that was
never
> implemented. :p and not exactly fit our needs here since we want to find out
> whether a given app is auto launched. 
> 
> If you don't mind, could you update KioskAppManager to remove the never
> implemented IsCurrentAppAutoLaunchedWithZeroDelay and add an
> IsAppAutoLaunchedWithZeroDelay? Think we could just do the following:
> bool KioskAppManager::IsAppAutoLaunchedWithZeroDelay(const std::string&
app_id)
> const {
>   return !app_id.empty() && app_id ==
> currently_auto_launched_with_zero_delay_app_;
> }
> 
> Another way is to use existing api to get app info, which has a field tell you
> whether it is auto launched. Something like:
> 
> KioskAppManager::App app_info;
> KioskAppManager::Get()->GetApp(extension_id(), &app_info);
> const bool is_auto_launched = app_info.was_auto_launched_with_zero_delay;

Good catch. I like using KioskAppManager::Get()->GetApp(). Fixed + removed the
not implemented method. Done.

[xiyuan, 2015-03-20 03:34:34]

kiosk_app_manager.h and file_system_api.h/cc LGTM

Thank you for helping cleaning the header.

[benwells, 2015-03-20 04:09:29]

https://codereview.chromium.org/985533004/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/985533004/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:1029: // extensions
and apps, which is not documented though.
On 2015/03/20 03:26:23, mtomasz wrote:
> On 2015/03/20 02:43:58, benwells wrote:
> > So the logic is, this can be used in kiosk mode OR by component extensions /
> > apps. Why do component extensions / apps need to use it?
> 
> It's used by Files app, Gallery app, Audio player, Video player and Image
loader
> extension, which are all component extensions/apps in Chrome OS.

Could you add a whitelist for these apps? This is a pretty dangerous API.

[mtomasz, 2015-03-20 06:27:38]

On 2015/03/20 04:09:29, benwells wrote:
>
https://codereview.chromium.org/985533004/diff/220001/chrome/browser/extensio...
> File chrome/browser/extensions/api/file_system/file_system_api.cc (right):
> 
>
https://codereview.chromium.org/985533004/diff/220001/chrome/browser/extensio...
> chrome/browser/extensions/api/file_system/file_system_api.cc:1029: //
extensions
> and apps, which is not documented though.
> On 2015/03/20 03:26:23, mtomasz wrote:
> > On 2015/03/20 02:43:58, benwells wrote:
> > > So the logic is, this can be used in kiosk mode OR by component extensions
/
> > > apps. Why do component extensions / apps need to use it?
> > 
> > It's used by Files app, Gallery app, Audio player, Video player and Image
> loader
> > extension, which are all component extensions/apps in Chrome OS.
> 
> Could you add a whitelist for these apps? This is a pretty dangerous API.

@benwells: I don't see a way to whitelist component apps while allowing all
non-component apps in _permission_features.json. We'd need a separate permission
feature, or have the whitelist in C++ code. WDYT?

[benwells, 2015-03-20 06:52:21]

On 2015/03/20 06:27:38, mtomasz wrote:
> On 2015/03/20 04:09:29, benwells wrote:
> >
>
https://codereview.chromium.org/985533004/diff/220001/chrome/browser/extensio...
> > File chrome/browser/extensions/api/file_system/file_system_api.cc (right):
> > 
> >
>
https://codereview.chromium.org/985533004/diff/220001/chrome/browser/extensio...
> > chrome/browser/extensions/api/file_system/file_system_api.cc:1029: //
> extensions
> > and apps, which is not documented though.
> > On 2015/03/20 03:26:23, mtomasz wrote:
> > > On 2015/03/20 02:43:58, benwells wrote:
> > > > So the logic is, this can be used in kiosk mode OR by component
extensions
> /
> > > > apps. Why do component extensions / apps need to use it?
> > > 
> > > It's used by Files app, Gallery app, Audio player, Video player and Image
> > loader
> > > extension, which are all component extensions/apps in Chrome OS.
> > 
> > Could you add a whitelist for these apps? This is a pretty dangerous API.
> 
> @benwells: I don't see a way to whitelist component apps while allowing all
> non-component apps in _permission_features.json. We'd need a separate
permission
> feature, or have the whitelist in C++ code. WDYT?

As this is limited to dev mode anyway, it is ok for now. So lgtm.

But before going to stable you should add a whitelist in, and yeah it will have
to be in c++.

[mtomasz, 2015-03-20 06:58:58]

The CQ bit was checked by mtomasz@chromium.org

[mtomasz, 2015-03-20 06:58:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, hirono@chromium.org, fukino@chromium.org
Link to the patchset: https://codereview.chromium.org/985533004/#ps240001
(title: "Fixed kiosk mode.")

[commit-bot: I haz the power, 2015-03-20 06:59:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/985533004/240001

[commit-bot: I haz the power, 2015-03-20 07:07:46]

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

[commit-bot: I haz the power, 2015-03-20 07:07:49]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[commit-bot: I haz the power, 2015-03-20 09:32:20]

Patchset 12 (id:??) landed as
https://crrev.com/26fbaa282e764f3e340332d12a7032a488e0a41a
Cr-Commit-Position: refs/heads/master@{#321538}

[mtomasz, 2015-03-20 09:32:41]

Committed patchset #12 (id:240001) manually as
26fbaa282e764f3e340332d12a7032a488e0a41a (presubmit successful).

[Dan Beam, 2015-03-25 23:00:17]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2015-03-25 23:00:20]

https://codereview.chromium.org/985533004/diff/240001/third_party/closure_com...
File third_party/closure_compiler/externs/chrome_extensions.js (right):

https://codereview.chromium.org/985533004/diff/240001/third_party/closure_com...
third_party/closure_compiler/externs/chrome_extensions.js:6836:
chrome.fileSystem.RequestFileSystemOptions;
this is a generated file, changes to it will be lost.

I'm adding a message to this effect now in bump_compiler_version

fukino@/hirono@: please don't LG changes to this file like this

[mtomasz, 2015-03-25 23:19:05]

On 2015/03/25 23:00:20, Dan Beam wrote:
>
https://codereview.chromium.org/985533004/diff/240001/third_party/closure_com...
> File third_party/closure_compiler/externs/chrome_extensions.js (right):
> 
>
https://codereview.chromium.org/985533004/diff/240001/third_party/closure_com...
> third_party/closure_compiler/externs/chrome_extensions.js:6836:
> chrome.fileSystem.RequestFileSystemOptions;
> this is a generated file, changes to it will be lost.
> 
> I'm adding a message to this effect now in bump_compiler_version
> 
> fukino@/hirono@: please don't LG changes to this file like this

Dan Beam: I committed the same changes to the original repository. I did it to
the local file just so we don't need to wait until the original file is rolled
out to Chromium. What's the correct way of doing it?

[Dan Beam, 2015-03-26 00:45:20]

On 2015/03/25 23:19:05, mtomasz wrote:
> On 2015/03/25 23:00:20, Dan Beam wrote:
> >
>
https://codereview.chromium.org/985533004/diff/240001/third_party/closure_com...
> > File third_party/closure_compiler/externs/chrome_extensions.js (right):
> > 
> >
>
https://codereview.chromium.org/985533004/diff/240001/third_party/closure_com...
> > third_party/closure_compiler/externs/chrome_extensions.js:6836:
> > chrome.fileSystem.RequestFileSystemOptions;
> > this is a generated file, changes to it will be lost.
> > 
> > I'm adding a message to this effect now in bump_compiler_version
> > 
> > fukino@/hirono@: please don't LG changes to this file like this
> 
> Dan Beam: I committed the same changes to the original repository. I did it to
> the local file just so we don't need to wait until the original file is rolled
> out to Chromium. What's the correct way of doing it?

if you mirrored it to the internal file, then it's fine.  the correct way is
autogenerating externs canonically in chrome and pushing them to google3.

sorry for the confusion: as long as you upstreamed and they were eventually
consistent, that's fine.