Move the check for fileBrowserHandler permission to FileBrowserHandler.

Move the check for fileBrowserHandler permission to FileBrowserHandler.

The previous check location was forcing all apps and extensions which call
the chrome.fileSystem.requestFileSystem API, to have "fileBrowserHandler"
permission as well.

That however doesn't make much sense, as those apps are not always handlers.
This CL moves the check to FileBrowserHandler::GetHandler() so simply the
permission has to be used only if the app wants to be a handler.

TEST=unit_test: FileBrowserHandlerManifestTest.GetHandlersRequiresPermission
BUG=470494
Committed: https://crrev.com/c5d0b60cdf33ce7330d6b9ef352b8119e65ec9de
Cr-Commit-Position: refs/heads/master@{#322343}

[mtomasz, 2015-03-25 10:03:06]

@yoz, @sky, @tzik: PTAL. Thanks.

yoz:
  chrome/browser/extensions/extension_special_storage_policy.cc [22]
  chrome/browser/extensions/extension_special_storage_policy.h [22]
  chrome/browser/extensions/mock_extension_special_storage_policy.cc [22]
  chrome/browser/extensions/mock_extension_special_storage_policy.h [22]
  chrome/common/extensions/api/file_browser_handlers/file_browser_handler.cc
[24]
  extensions/shell/browser/shell_special_storage_policy.cc [19]
  extensions/shell/browser/shell_special_storage_policy.h [19]
sky:
  chrome/browser/platform_util_unittest.cc [10]
  content/public/test/mock_special_storage_policy.cc [16]
  content/public/test/mock_special_storage_policy.h [16]
tzik:
  storage/browser/quota/special_storage_policy.h

[mtomasz, 2015-03-25 10:03:24]

mtomasz@chromium.org changed reviewers:
+ sky@chromium.org, tzik@chromium.org, yoz@chromium.org

[mtomasz, 2015-03-25 10:03:41]

@yoz, @sky, @tzik: PTAL. Thanks.

yoz:
  chrome/browser/extensions/extension_special_storage_policy.cc [22]
  chrome/browser/extensions/extension_special_storage_policy.h [22]
  chrome/browser/extensions/mock_extension_special_storage_policy.cc [22]
  chrome/browser/extensions/mock_extension_special_storage_policy.h [22]
  chrome/common/extensions/api/file_browser_handlers/file_browser_handler.cc
[24]
  extensions/shell/browser/shell_special_storage_policy.cc [19]
  extensions/shell/browser/shell_special_storage_policy.h [19]
sky:
  chrome/browser/platform_util_unittest.cc [10]
  content/public/test/mock_special_storage_policy.cc [16]
  content/public/test/mock_special_storage_policy.h [16]
tzik:
  storage/browser/quota/special_storage_policy.h

[mtomasz, 2015-03-25 11:04:55]

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

[mtomasz, 2015-03-25 11:05:12]

@hirono: PTAL at *.json. Thanks.

[hirono, 2015-03-25 11:31:54]

lgtm for *.json.

[tzik, 2015-03-25 12:36:33]

lgtm

[sky, 2015-03-25 14:50:59]

LGTM

[Yoyo Zhou, 2015-03-25 20:37:09]

https://codereview.chromium.org/1030133002/diff/40001/chrome/common/extension...
File chrome/common/extensions/api/file_browser_handlers/file_browser_handler.cc
(right):

https://codereview.chromium.org/1030133002/diff/40001/chrome/common/extension...
chrome/common/extensions/api/file_browser_handlers/file_browser_handler.cc:120:
// Manifest is parsed before permissions are loaded, so file browser handlers
I'm not sure I follow. The manifest gets deserialized from JSON first, but the
permissions get loaded first (before parsing of manifest handlers), otherwise
you wouldn't have permissions_data. See Extension::InitFromValue.

[mtomasz, 2015-03-25 23:23:45]

On 2015/03/25 20:37:09, Yoyo Zhou wrote:
>
https://codereview.chromium.org/1030133002/diff/40001/chrome/common/extension...
> File
chrome/common/extensions/api/file_browser_handlers/file_browser_handler.cc
> (right):
> 
>
https://codereview.chromium.org/1030133002/diff/40001/chrome/common/extension...
>
chrome/common/extensions/api/file_browser_handlers/file_browser_handler.cc:120:
> // Manifest is parsed before permissions are loaded, so file browser handlers
> I'm not sure I follow. The manifest gets deserialized from JSON first, but the
> permissions get loaded first (before parsing of manifest handlers), otherwise
> you wouldn't have permissions_data. See Extension::InitFromValue.

I can't check now, but IIRC:
FileBrowserHandlerParser::Parse is called from here:
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...
so before:
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...
so it means, that we can't access Extension::permission_data in
FileBrowserHandlerParser::Parse, as it is null pointer.

[mtomasz, 2015-03-26 01:11:46]

https://codereview.chromium.org/1030133002/diff/40001/chrome/common/extension...
File chrome/common/extensions/api/file_browser_handlers/file_browser_handler.cc
(right):

https://codereview.chromium.org/1030133002/diff/40001/chrome/common/extension...
chrome/common/extensions/api/file_browser_handlers/file_browser_handler.cc:120:
// Manifest is parsed before permissions are loaded, so file browser handlers
On 2015/03/25 20:37:09, Yoyo Zhou wrote:
> I'm not sure I follow. The manifest gets deserialized from JSON first, but the
> permissions get loaded first (before parsing of manifest handlers), otherwise
> you wouldn't have permissions_data. See Extension::InitFromValue.

Done per offline discussion.

[Yoyo Zhou, 2015-03-26 01:31:32]

LGTM

https://codereview.chromium.org/1030133002/diff/60001/chrome/common/extension...
File chrome/common/extensions/api/file_browser_handlers/file_browser_handler.cc
(right):

https://codereview.chromium.org/1030133002/diff/60001/chrome/common/extension...
chrome/common/extensions/api/file_browser_handlers/file_browser_handler.cc:274:
// If the permission is missing, then skip parsing the list of handlers.
I think it would be helpful to add an install warning (AddInstallWarning) if we
have no permission but some handlers.

[mtomasz, 2015-03-26 01:35:32]

https://codereview.chromium.org/1030133002/diff/60001/chrome/common/extension...
File chrome/common/extensions/api/file_browser_handlers/file_browser_handler.cc
(right):

https://codereview.chromium.org/1030133002/diff/60001/chrome/common/extension...
chrome/common/extensions/api/file_browser_handlers/file_browser_handler.cc:274:
// If the permission is missing, then skip parsing the list of handlers.
On 2015/03/26 01:31:32, Yoyo Zhou wrote:
> I think it would be helpful to add an install warning (AddInstallWarning) if
we
> have no permission but some handlers.

Agreed. Let me do it in a separate CL, as this one is already very big.

[mtomasz, 2015-03-26 03:09:34]

The CQ bit was checked by mtomasz@chromium.org

[mtomasz, 2015-03-26 03:09:34]

The patchset sent to the CQ was uploaded after l-g-t-m from hirono@chromium.org,
sky@chromium.org, tzik@chromium.org, yoz@chromium.org
Link to the patchset: https://codereview.chromium.org/1030133002/#ps80001
(title: "Fixed the end-to-end test.")

[commit-bot: I haz the power, 2015-03-26 03:10:07]

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

[commit-bot: I haz the power, 2015-03-26 03:52:18]

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

[commit-bot: I haz the power, 2015-03-26 03:52:19]

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

[mtomasz, 2015-03-26 06:55:19]

On 2015/03/26 03:52:19, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

@yoz: The entire file browser handling code is executed on OS != Chrome OS,
which doesn't seem right. I applied quick fixes for now, but I think we should
not compile this code on non-Chrome-OS operating systems. I'll file a bug for
it.

[mtomasz, 2015-03-26 06:58:27]

The CQ bit was checked by mtomasz@chromium.org

[mtomasz, 2015-03-26 06:58:28]

The patchset sent to the CQ was uploaded after l-g-t-m from yoz@chromium.org,
hirono@chromium.org, sky@chromium.org, tzik@chromium.org
Link to the patchset: https://codereview.chromium.org/1030133002/#ps100001
(title: "Fixed test for OS != OS_CHROMEOS.")

[commit-bot: I haz the power, 2015-03-26 06:58:42]

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

[commit-bot: I haz the power, 2015-03-26 08:08:04]

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

[commit-bot: I haz the power, 2015-03-26 08:08:06]

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

[mtomasz, 2015-03-26 08:09:28]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2015-03-26 08:09:48]

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

[commit-bot: I haz the power, 2015-03-26 09:47:13]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-03-26 09:48:07]

Patchset 6 (id:??) landed as
https://crrev.com/c5d0b60cdf33ce7330d6b9ef352b8119e65ec9de
Cr-Commit-Position: refs/heads/master@{#322343}

[Yoyo Zhou, 2015-03-26 18:29:09]

On 2015/03/26 06:55:19, mtomasz wrote:
> On 2015/03/26 03:52:19, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> 
> @yoz: The entire file browser handling code is executed on OS != Chrome OS,
> which doesn't seem right. I applied quick fixes for now, but I think we should
> not compile this code on non-Chrome-OS operating systems. I'll file a bug for
> it.

Yes, that doesn't seem right. In _permission_features.json we require it to be
ChromeOS, anyhow. IIRC this code used to be under chrome/browser/chromeos, so
that was how it didn't get compiled elsewhere; we should make the gyp/gn rules
do the same now.

Please file such a bug.