Deprecate direct use of legacy extension management preference

Deprecate direct use of legacy extension management preference

Use ExtensionManagement instead.

BUG=177351
Committed: https://crrev.com/311ecdf7e9ba01b0a8ccbd1488e312e13cf398c5
Cr-Commit-Position: refs/heads/master@{#294688}

[binjin, 2014-09-09 23:57:50]

binjin@chromium.org changed reviewers:
+ joaodasilva@chromium.org

[binjin, 2014-09-09 23:57:51]

https://codereview.chromium.org/555323002/diff/1/chrome/browser/download/down...
File chrome/browser/download/download_crx_util.cc (right):

https://codereview.chromium.org/555323002/diff/1/chrome/browser/download/down...
chrome/browser/download/download_crx_util.cc:137: // No allowed install sites
specified, disallow by default.
It turns out that it's how ExtensionInstallSources policy is being handled now.

The preference is not forced to be managed before, so overriding the default
behavior with user preference is possible. It might be a good idea to set
forced-managed for this preference to false in this CL as well.

But I would like to eventually drop all these legacy policies and preferences,
after that, disallowing be default looks too strict to me.

What do you think?

[Joao da Silva, 2014-09-10 10:05:55]

This change looks good to me as is. Just a suggestion inline to move the logic
to ExtensionManagement, I think it's the natural place to make those checks.
WDYT?

https://codereview.chromium.org/555323002/diff/1/chrome/browser/download/down...
File chrome/browser/download/download_crx_util.cc (right):

https://codereview.chromium.org/555323002/diff/1/chrome/browser/download/down...
chrome/browser/download/download_crx_util.cc:137: // No allowed install sites
specified, disallow by default.
On 2014/09/09 23:57:51, bjin wrote:
> It turns out that it's how ExtensionInstallSources policy is being handled
now.
> 
> The preference is not forced to be managed before, so overriding the default
> behavior with user preference is possible. It might be a good idea to set
> forced-managed for this preference to false in this CL as well.
> 
> But I would like to eventually drop all these legacy policies and preferences,
> after that, disallowing be default looks too strict to me.
> 
> What do you think?

I think this is fine. This preference was never meant to work as a user
preference, only as a managed setting.

https://codereview.chromium.org/555323002/diff/1/chrome/browser/download/down...
chrome/browser/download/download_crx_util.cc:155: item.GetURL().SchemeIsFile();
What do you think of moving all this logic to ExtensionManagement?

Then the code from here would be just:

  return ExtensionManagementFactory::GetForBrowserContext(profile)
      ->IsOffstoreInstallAllowed(item.GetURL(), item.GetReferrerUrl());

Having all the logic around the policies in ExtensionManagement would make it
easier to unit test and to see how all the policies affect the browser.

[asanka, 2014-09-10 16:14:01]

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

[asanka, 2014-09-10 16:14:02]

https://codereview.chromium.org/555323002/diff/1/chrome/browser/download/down...
File chrome/browser/download/download_crx_util.cc (right):

https://codereview.chromium.org/555323002/diff/1/chrome/browser/download/down...
chrome/browser/download/download_crx_util.cc:155: item.GetURL().SchemeIsFile();
On 2014/09/10 10:05:54, Joao da Silva wrote:
> What do you think of moving all this logic to ExtensionManagement?
> 
> Then the code from here would be just:
> 
>   return ExtensionManagementFactory::GetForBrowserContext(profile)
>       ->IsOffstoreInstallAllowed(item.GetURL(), item.GetReferrerUrl());
> 
> Having all the logic around the policies in ExtensionManagement would make it
> easier to unit test and to see how all the policies affect the browser.

+1. The only use of this function is in
DownloadTargetDeterminer::IsDangerousFile() which is trying to decide whether a
given download is considered dangerous or not (for the purpose of displaying a
warning to the user).

The logic being enforced by invoking this method is "extension downloads from
pref-whitelisted sources are not considered dangerous". I'd be happy if the
question asked by downloads is "does the extension subsystem consider this
extension download to be dangerous?" and have ExtensionManagement (or somewhere
else) encapsulate the details of how that determination is made.

[binjin, 2014-09-12 11:46:14]

https://codereview.chromium.org/555323002/diff/1/chrome/browser/download/down...
File chrome/browser/download/download_crx_util.cc (right):

https://codereview.chromium.org/555323002/diff/1/chrome/browser/download/down...
chrome/browser/download/download_crx_util.cc:155: item.GetURL().SchemeIsFile();
On 2014/09/10 10:05:54, Joao da Silva wrote:
> What do you think of moving all this logic to ExtensionManagement?
> 
> Then the code from here would be just:
> 
>   return ExtensionManagementFactory::GetForBrowserContext(profile)
>       ->IsOffstoreInstallAllowed(item.GetURL(), item.GetReferrerUrl());
> 
> Having all the logic around the policies in ExtensionManagement would make it
> easier to unit test and to see how all the policies affect the browser.

Done.

[Joao da Silva, 2014-09-12 13:18:41]

lgtm, some simple comments left

https://codereview.chromium.org/555323002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_management.cc (right):

https://codereview.chromium.org/555323002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_management.cc:112: return
url_patterns.MatchesURL(referrer_url) || url.SchemeIsFile();
invert the order of these 2 tests: if the scheme is file then there is no need
to call MatchesURL

https://codereview.chromium.org/555323002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_management.h (right):

https://codereview.chromium.org/555323002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_management.h:121: bool
IsOffstoreInstallAllowed(const GURL& url, const GURL& referrer_url);
GURL needs a forward declaration

[binjin, 2014-09-12 13:35:43]

binjin@chromium.org changed reviewers:
+ finnur@chromium.org

[binjin, 2014-09-12 13:35:44]

asanka: chrome/browser/download/*
finnur: chrome/browser/extensions/*
        chrome/browser/ui/webui/extensions/*
        extensions/browser/*

https://codereview.chromium.org/555323002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_management.h (right):

https://codereview.chromium.org/555323002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_management.h:121: bool
IsOffstoreInstallAllowed(const GURL& url, const GURL& referrer_url);
On 2014/09/12 13:18:40, Joao da Silva wrote:
> GURL needs a forward declaration

Done.

[Joao da Silva, 2014-09-12 13:39:17]

Still lgtm, one more nit

https://codereview.chromium.org/555323002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_management.h (right):

https://codereview.chromium.org/555323002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_management.h:26: class GURL;
nit: order (GURL before PrefService)

[binjin, 2014-09-12 13:41:23]

https://codereview.chromium.org/555323002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_management.h (right):

https://codereview.chromium.org/555323002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_management.h:26: class GURL;
On 2014/09/12 13:39:16, Joao da Silva wrote:
> nit: order (GURL before PrefService)

Done.

[Finnur, 2014-09-12 13:55:34]

LGTM, one nit.

https://codereview.chromium.org/555323002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/extension_management.cc (right):

https://codereview.chromium.org/555323002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_management.cc:103: }
no braces (single line if).

[binjin, 2014-09-12 13:57:26]

https://codereview.chromium.org/555323002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/extension_management.cc (right):

https://codereview.chromium.org/555323002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_management.cc:103: }
On 2014/09/12 13:55:34, Finnur wrote:
> no braces (single line if).

Done.

[asanka, 2014-09-12 21:39:47]

/download/ lgtm

[binjin, 2014-09-12 21:42:20]

The CQ bit was checked by binjin@chromium.org

[commit-bot: I haz the power, 2014-09-12 21:43:12]

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

[commit-bot: I haz the power, 2014-09-12 22:17:11]

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

[commit-bot: I haz the power, 2014-09-12 22:17:12]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[binjin, 2014-09-12 22:18:39]

The CQ bit was checked by binjin@chromium.org

[commit-bot: I haz the power, 2014-09-12 22:19:19]

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

[commit-bot: I haz the power, 2014-09-12 22:58:17]

Committed patchset #5 (id:80001) as 419338c16599750c05eb463cf55107790182d3dd

[commit-bot: I haz the power, 2014-09-12 23:01:35]

Patchset 5 (id:??) landed as
https://crrev.com/311ecdf7e9ba01b0a8ccbd1488e312e13cf398c5
Cr-Commit-Position: refs/heads/master@{#294688}