Plumbing for login apps device policy to extensions.

Plumbing for login apps device policy to extensions.

This CL succeeeds the CL #2150483004

ChromeProcessManagerDelegate:
* Enable background pages for apps in the login profile when
command-line switch --enable-login-apps is set.

ExtensionManagement:
* Factor out common code into GetInstallListByMode from
GetForceInstallList and GetRecommendedInstallList.
* Common function UpdateForcedExtensions.

ExtensionSystemImpl:
* Enable extensions in login profile when command-line switch
--enable-login-apps is set.

ExternalProviderImpl:
* In CreateExternalProviders, create an ExternalPolicyLoader for login
apps in the login profile.

ExtensionInstallListPolicyHandler:
* Common base class to parse login and force extension install lists.

* Switch --enable-login-apps to enable login apps.
* Extension pref extensions.install.loginlist.

BUG=576464
Committed: https://crrev.com/3bbfabe266cd5138393d3e87229de567d19da005
Cr-Commit-Position: refs/heads/master@{#432917}

[Denis Kuznetsov (DE-MUC), 2016-09-02 17:44:43]

Description was changed from

==========
Plumbing for login apps device policy to extensions.

This CL superceeds the CL #2150483004

ChromeProcessManagerDelegate:
* Enable background pages for apps in the login profile when
command-line switch --enable-login-apps is set.

ExtensionManagement:
* Factor out common code into GetInstallListByMode from
GetForceInstallList and GetRecommendedInstallList.
* Common function UpdateForcedExtensions.

ExtensionSystemImpl:
* Enable extensions in login profile when command-line switch
--enable-login-apps is set.

ExternalProviderImpl:
* In CreateExternalProviders, create an ExternalPolicyLoader for login
apps in the login profile.

ExtensionInstallListPolicyHandler:
* Common base class to parse login and force extension install lists.

* Switch --enable-login-apps to enable login apps.
* Extension pref extensions.install.loginlist.

BUG=576464
==========

to

==========
Plumbing for login apps device policy to extensions.

This CL succeeeds the CL #2150483004

ChromeProcessManagerDelegate:
* Enable background pages for apps in the login profile when
command-line switch --enable-login-apps is set.

ExtensionManagement:
* Factor out common code into GetInstallListByMode from
GetForceInstallList and GetRecommendedInstallList.
* Common function UpdateForcedExtensions.

ExtensionSystemImpl:
* Enable extensions in login profile when command-line switch
--enable-login-apps is set.

ExternalProviderImpl:
* In CreateExternalProviders, create an ExternalPolicyLoader for login
apps in the login profile.

ExtensionInstallListPolicyHandler:
* Common base class to parse login and force extension install lists.

* Switch --enable-login-apps to enable login apps.
* Extension pref extensions.install.loginlist.

BUG=576464
==========

[achuithb, 2016-09-02 23:14:19]

The CQ bit was checked by achuith@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-02 23:14:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-03 00:07:12]

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

[commit-bot: I haz the power, 2016-09-03 00:07:19]

Dry run: This issue passed the CQ dry run.

[Denis Kuznetsov (DE-MUC), 2016-09-05 12:55:53]

antrim@chromium.org changed reviewers:
+ achuithb@chromium.org, emaxx@chromium.org
- achuith@chromium.org

[Denis Kuznetsov (DE-MUC), 2016-09-05 12:56:44]

antrim@chromium.org changed reviewers:
+ achuith@google.com
- achuithb@chromium.org

[emaxx, 2016-11-04 13:54:51]

Did you consider renaming _all_ occurrences of words "login screen", "login"
with "signin profile"? I feel this would be more consistent.

https://codereview.chromium.org/2306143002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_process_manager_delegate.cc (right):

https://codereview.chromium.org/2306143002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/chrome_process_manager_delegate.cc:62:
!ExtensionManagementFactory::GetForBrowserContext(profile)
nit: There's a variable "context" in this scope, so it'd be a bit cleaner to
pass it instead of the profile here.

https://codereview.chromium.org/2306143002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/chrome_process_manager_delegate.cc:65:
is_normal_session =
I don't actually understand the logic here.
Looks like this function will return true for the sign-in profile regardless of
all other conditions (like command line flags) after the user logs in (as
IsUserLoggedIn() will return true).
Is it really the intended behavior?

I would guess that the cases of sign-in profile vs. normal user profile should
be differentiated stricter here.

https://codereview.chromium.org/2306143002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/chrome_process_manager_delegate.cc:67:
(is_signin_profile && login_apps_enabled && login_apps_installed);
I'll repeat my comment from the original CL:
> I think it's better to use another boolean variable for this
> login-related stuff, as the name "is_normal_session" doesn't
> really correspond to the case of the login screen.

https://codereview.chromium.org/2306143002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/chrome_process_manager_delegate.cc:75: return
is_normal_session || profile->IsOffTheRecord();
Shouldn't this condition be updated too? Is there a reason to allow background
page for the off-the-record profile derived from the sign-in profile?

I also don't understand the reason to make the disjunction with
profile->IsOffTheRecord() here. The comment above doesn't sound clarifying
things much. And the referenced bug was fixed back in 2014. Maybe this part can
be dropped completely now?

https://codereview.chromium.org/2306143002/diff/20001/chrome/common/chrome_sw...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/2306143002/diff/20001/chrome/common/chrome_sw...
chrome/common/chrome_switches.cc:385: // Enables apps on the login screen.
s/on the login screen/in the login profile/?
This would probably sound more technically correct.

https://codereview.chromium.org/2306143002/diff/20001/chrome/common/chrome_sw...
File chrome/common/chrome_switches.h (right):

https://codereview.chromium.org/2306143002/diff/20001/chrome/common/chrome_sw...
chrome/common/chrome_switches.h:125: extern const char kEnableLoginApps[];
There's a separate section for "if defined(OS_CHROMEOS)" items below.

https://codereview.chromium.org/2306143002/diff/20001/chrome/common/chrome_sw...
chrome/common/chrome_switches.h:125: extern const char kEnableLoginApps[];
Maybe replace "login" with "signin" here too?

[Denis Kuznetsov (DE-MUC), 2016-11-08 18:47:49]

https://codereview.chromium.org/2306143002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_process_manager_delegate.cc (right):

https://codereview.chromium.org/2306143002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/chrome_process_manager_delegate.cc:62:
!ExtensionManagementFactory::GetForBrowserContext(profile)
On 2016/11/04 13:54:51, emaxx wrote:
> nit: There's a variable "context" in this scope, so it'd be a bit cleaner to
> pass it instead of the profile here.

Done.

https://codereview.chromium.org/2306143002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/chrome_process_manager_delegate.cc:65:
is_normal_session =
On 2016/11/04 13:54:51, emaxx wrote:
> I don't actually understand the logic here.
> Looks like this function will return true for the sign-in profile regardless
of
> all other conditions (like command line flags) after the user logs in (as
> IsUserLoggedIn() will return true).
> Is it really the intended behavior?
> 
> I would guess that the cases of sign-in profile vs. normal user profile should
> be differentiated stricter here.

Done.

https://codereview.chromium.org/2306143002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/chrome_process_manager_delegate.cc:65:
is_normal_session =
On 2016/11/04 13:54:51, emaxx wrote:
> I don't actually understand the logic here.
> Looks like this function will return true for the sign-in profile regardless
of
> all other conditions (like command line flags) after the user logs in (as
> IsUserLoggedIn() will return true).
> Is it really the intended behavior?
> 
> I would guess that the cases of sign-in profile vs. normal user profile should
> be differentiated stricter here.

If you follow the crbug link below, the whole purpose of the old code was to
disable background pages for non-off-the-record profile in a guest mode.

https://codereview.chromium.org/2306143002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/chrome_process_manager_delegate.cc:67:
(is_signin_profile && login_apps_enabled && login_apps_installed);
On 2016/11/04 13:54:51, emaxx wrote:
> I'll repeat my comment from the original CL:
> > I think it's better to use another boolean variable for this
> > login-related stuff, as the name "is_normal_session" doesn't
> > really correspond to the case of the login screen.

Done.

https://codereview.chromium.org/2306143002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/chrome_process_manager_delegate.cc:75: return
is_normal_session || profile->IsOffTheRecord();
On 2016/11/04 13:54:51, emaxx wrote:
> Shouldn't this condition be updated too? Is there a reason to allow background
> page for the off-the-record profile derived from the sign-in profile?
> 
> I also don't understand the reason to make the disjunction with
> profile->IsOffTheRecord() here. The comment above doesn't sound clarifying
> things much. And the referenced bug was fixed back in 2014. Maybe this part
can
> be dropped completely now?
The bug was fixed by adding this code.

https://codereview.chromium.org/2306143002/diff/20001/chrome/common/chrome_sw...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/2306143002/diff/20001/chrome/common/chrome_sw...
chrome/common/chrome_switches.cc:385: // Enables apps on the login screen.
On 2016/11/04 13:54:51, emaxx wrote:
> s/on the login screen/in the login profile/?
> This would probably sound more technically correct.

Done.

https://codereview.chromium.org/2306143002/diff/20001/chrome/common/chrome_sw...
File chrome/common/chrome_switches.h (right):

https://codereview.chromium.org/2306143002/diff/20001/chrome/common/chrome_sw...
chrome/common/chrome_switches.h:125: extern const char kEnableLoginApps[];
On 2016/11/04 13:54:51, emaxx wrote:
> There's a separate section for "if defined(OS_CHROMEOS)" items below.

Done.

[emaxx, 2016-11-09 01:09:30]

Looks good to me.
But is there a plan to write tests for the new functionality? Apart from the
usual benefits, it would be useful to prove some security guarantees. For
instance, that an extension that was manually unpacked into the profile
directory is denied from loading.

https://codereview.chromium.org/2306143002/diff/40001/chrome/common/chrome_sw...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/2306143002/diff/40001/chrome/common/chrome_sw...
chrome/common/chrome_switches.cc:388: const char kEnableSigninApps[] =
"enable-login-apps";
nit: The constant name differs from the command line here.

https://codereview.chromium.org/2306143002/diff/40001/extensions/browser/pref...
File extensions/browser/pref_names.h (right):

https://codereview.chromium.org/2306143002/diff/40001/extensions/browser/pref...
extensions/browser/pref_names.h:72: // screen
nit: Something is wrong with the alignment.

[Denis Kuznetsov (DE-MUC), 2016-11-09 18:25:26]

https://codereview.chromium.org/2306143002/diff/40001/chrome/common/chrome_sw...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/2306143002/diff/40001/chrome/common/chrome_sw...
chrome/common/chrome_switches.cc:388: const char kEnableSigninApps[] =
"enable-login-apps";
On 2016/11/09 01:09:30, emaxx wrote:
> nit: The constant name differs from the command line here.

Done.

https://codereview.chromium.org/2306143002/diff/40001/extensions/browser/pref...
File extensions/browser/pref_names.h (right):

https://codereview.chromium.org/2306143002/diff/40001/extensions/browser/pref...
extensions/browser/pref_names.h:72: // screen
On 2016/11/09 01:09:30, emaxx wrote:
> nit: Something is wrong with the alignment.

Done.
Was caused by "git cl format"

[Denis Kuznetsov (DE-MUC), 2016-11-10 14:21:03]

antrim@chromium.org changed reviewers:
+ asargent@chromium.org, bartfab@chromium.org

[Denis Kuznetsov (DE-MUC), 2016-11-10 14:21:03]

+asargent
+bartfab as owner of
chrome/browser/policy/configuration_policy_handler_list_factory.cc

[emaxx, 2016-11-10 15:03:44]

https://codereview.chromium.org/2306143002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_process_manager_delegate.cc (right):

https://codereview.chromium.org/2306143002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/chrome_process_manager_delegate.cc:80:
allow_for_signin_profile;
I think there's still something wrong with this expression.

Suppose this is called for sign-in profile after the user has logged in, i.e.:
1. profile->IsGuestSession()==false;
2. profile->IsSystemProfile()==false;
3. UserManager->IsUserLoggedIn()==true;
4. IsSigninProfile(profile)==true.
Then we have is_normal_session evaluated to true in line #52, re-assigned to
true in line #58, and therefore the whole IsBackgroundPageAllowed method will
return true regardless of the value of allow_for_signin_profile.


I understand you intention to write the integration tests after all critical
pieces land, but I think that this CL would benefit a lot from having tests at
least for critical pieces like this.

[Denis Kuznetsov (DE-MUC), 2016-11-10 17:33:37]

The CQ bit was checked by antrim@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-10 17:34:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-10 18:15:07]

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

[commit-bot: I haz the power, 2016-11-10 18:15:07]

Dry run: This issue passed the CQ dry run.

[achuithb, 2016-11-11 00:40:47]

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

[achuithb, 2016-11-11 00:40:47]

https://codereview.chromium.org/2306143002/diff/80001/chrome/common/chrome_sw...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/2306143002/diff/80001/chrome/common/chrome_sw...
chrome/common/chrome_switches.cc:389: #endif  // defined(OS_CHROMEOS)
nit, can probably drop the comment here

[bartfab (slow), 2016-11-11 13:06:14]

chrome/browser/policy/configuration_policy_handler_list_factory.cc LGTM

[Denis Kuznetsov (DE-MUC), 2016-11-11 13:55:47]

antrim@chromium.org changed reviewers:
- achuith@google.com

[emaxx, 2016-11-14 19:32:23]

lgtm

[asargent_no_longer_on_chrome, 2016-11-14 21:30:25]

lgtm

https://codereview.chromium.org/2306143002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/2306143002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1464: VLOG(1) << "AddExtension "
<< extension->name() << ", " << extension->id();
nit: should this logging be checked in?

https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

https://codereview.chromium.org/2306143002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/policy_handlers.cc (right):

https://codereview.chromium.org/2306143002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/policy_handlers.cc:185: VLOG(1) << "ParseList ext=" <<
extension_id << ", url=" << update_url;
nit: does this logging meet the criteria for getting checked in?

https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

[achuithb, 2016-11-14 21:35:13]

On 2016/11/14 21:30:25, Antony Sargent wrote:
> lgtm
> 
>
https://codereview.chromium.org/2306143002/diff/80001/chrome/browser/extensio...
> File chrome/browser/extensions/extension_service.cc (right):
> 
>
https://codereview.chromium.org/2306143002/diff/80001/chrome/browser/extensio...
> chrome/browser/extensions/extension_service.cc:1464: VLOG(1) << "AddExtension
"
> << extension->name() << ", " << extension->id();
> nit: should this logging be checked in?
> 
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
> 
>
https://codereview.chromium.org/2306143002/diff/80001/chrome/browser/extensio...
> File chrome/browser/extensions/policy_handlers.cc (right):
> 
>
https://codereview.chromium.org/2306143002/diff/80001/chrome/browser/extensio...
> chrome/browser/extensions/policy_handlers.cc:185: VLOG(1) << "ParseList ext="
<<
> extension_id << ", url=" << update_url;
> nit: does this logging meet the criteria for getting checked in?
> 
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

Antony - I think we're more ok with VLOG's since they have to be explicitly
turned on for debugging. We run release chrome on chromeos devices, so DVLOGs
are not useful.

[achuithb, 2016-11-14 21:36:09]

lgtm

[asargent_no_longer_on_chrome, 2016-11-14 21:53:37]

On 2016/11/14 21:35:13, achuithb wrote:
> On 2016/11/14 21:30:25, Antony Sargent wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/2306143002/diff/80001/chrome/browser/extensio...
> > File chrome/browser/extensions/extension_service.cc (right):
> > 
> >
>
https://codereview.chromium.org/2306143002/diff/80001/chrome/browser/extensio...
> > chrome/browser/extensions/extension_service.cc:1464: VLOG(1) <<
"AddExtension
> "
> > << extension->name() << ", " << extension->id();
> > nit: should this logging be checked in?
> > 
> >
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
> > 
> >
>
https://codereview.chromium.org/2306143002/diff/80001/chrome/browser/extensio...
> > File chrome/browser/extensions/policy_handlers.cc (right):
> > 
> >
>
https://codereview.chromium.org/2306143002/diff/80001/chrome/browser/extensio...
> > chrome/browser/extensions/policy_handlers.cc:185: VLOG(1) << "ParseList
ext="
> <<
> > extension_id << ", url=" << update_url;
> > nit: does this logging meet the criteria for getting checked in?
> > 
> >
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
> 
> Antony - I think we're more ok with VLOG's since they have to be explicitly
> turned on for debugging. We run release chrome on chromeos devices, so DVLOGs
> are not useful.

I believe the reason the styleguide rule is written that way is for binary size
considerations - the sum total of logging is already fairly significant, and
when people only think about this locally ("this logging could potentially be
useful someday"), it results in a ever-worsening tragedy of the commons.

[achuithb, 2016-11-14 21:59:00]

Sure, I'm fine with getting rid of these, though I think the first one is
generally more useful.

https://codereview.chromium.org/2306143002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/2306143002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1464: VLOG(1) << "AddExtension "
<< extension->name() << ", " << extension->id();
On 2016/11/14 21:30:25, Antony Sargent wrote:
> nit: should this logging be checked in?
> 
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

This is the only place where we log all the extensions that are loaded in a
session.

https://codereview.chromium.org/2306143002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/policy_handlers.cc (right):

https://codereview.chromium.org/2306143002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/policy_handlers.cc:185: VLOG(1) << "ParseList ext=" <<
extension_id << ", url=" << update_url;
On 2016/11/14 21:30:25, Antony Sargent wrote:
> nit: does this logging meet the criteria for getting checked in?
> 
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

We can probably get rid of this

[Denis Kuznetsov (DE-MUC), 2016-11-17 14:59:09]

The CQ bit was checked by antrim@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 14:59:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 17:31:18]

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

[commit-bot: I haz the power, 2016-11-17 17:31:20]

Dry run: This issue passed the CQ dry run.

[Denis Kuznetsov (DE-MUC), 2016-11-17 18:13:01]

https://codereview.chromium.org/2306143002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/2306143002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1464: VLOG(1) << "AddExtension "
<< extension->name() << ", " << extension->id();
On 2016/11/14 21:30:25, Antony Sargent wrote:
> nit: should this logging be checked in?
> 
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

Done.

https://codereview.chromium.org/2306143002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/policy_handlers.cc (right):

https://codereview.chromium.org/2306143002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/policy_handlers.cc:185: VLOG(1) << "ParseList ext=" <<
extension_id << ", url=" << update_url;
On 2016/11/14 21:59:00, achuithb wrote:
> On 2016/11/14 21:30:25, Antony Sargent wrote:
> > nit: does this logging meet the criteria for getting checked in?
> > 
> >
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
> 
> We can probably get rid of this

Done.

[Denis Kuznetsov (DE-MUC), 2016-11-17 18:13:10]

The CQ bit was checked by antrim@chromium.org

[Denis Kuznetsov (DE-MUC), 2016-11-17 18:13:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
bartfab@chromium.org, achuith@chromium.org, asargent@chromium.org,
emaxx@chromium.org
Link to the patchset: https://codereview.chromium.org/2306143002/#ps100001
(title: "Removed unnecessary logging")

[commit-bot: I haz the power, 2016-11-17 18:14:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 18:24:52]

Description was changed from

==========
Plumbing for login apps device policy to extensions.

This CL succeeeds the CL #2150483004

ChromeProcessManagerDelegate:
* Enable background pages for apps in the login profile when
command-line switch --enable-login-apps is set.

ExtensionManagement:
* Factor out common code into GetInstallListByMode from
GetForceInstallList and GetRecommendedInstallList.
* Common function UpdateForcedExtensions.

ExtensionSystemImpl:
* Enable extensions in login profile when command-line switch
--enable-login-apps is set.

ExternalProviderImpl:
* In CreateExternalProviders, create an ExternalPolicyLoader for login
apps in the login profile.

ExtensionInstallListPolicyHandler:
* Common base class to parse login and force extension install lists.

* Switch --enable-login-apps to enable login apps.
* Extension pref extensions.install.loginlist.

BUG=576464
==========

to

==========
Plumbing for login apps device policy to extensions.

This CL succeeeds the CL #2150483004

ChromeProcessManagerDelegate:
* Enable background pages for apps in the login profile when
command-line switch --enable-login-apps is set.

ExtensionManagement:
* Factor out common code into GetInstallListByMode from
GetForceInstallList and GetRecommendedInstallList.
* Common function UpdateForcedExtensions.

ExtensionSystemImpl:
* Enable extensions in login profile when command-line switch
--enable-login-apps is set.

ExternalProviderImpl:
* In CreateExternalProviders, create an ExternalPolicyLoader for login
apps in the login profile.

ExtensionInstallListPolicyHandler:
* Common base class to parse login and force extension install lists.

* Switch --enable-login-apps to enable login apps.
* Extension pref extensions.install.loginlist.

BUG=576464
==========

[commit-bot: I haz the power, 2016-11-17 18:24:53]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-11-17 18:32:53]

Description was changed from

==========
Plumbing for login apps device policy to extensions.

This CL succeeeds the CL #2150483004

ChromeProcessManagerDelegate:
* Enable background pages for apps in the login profile when
command-line switch --enable-login-apps is set.

ExtensionManagement:
* Factor out common code into GetInstallListByMode from
GetForceInstallList and GetRecommendedInstallList.
* Common function UpdateForcedExtensions.

ExtensionSystemImpl:
* Enable extensions in login profile when command-line switch
--enable-login-apps is set.

ExternalProviderImpl:
* In CreateExternalProviders, create an ExternalPolicyLoader for login
apps in the login profile.

ExtensionInstallListPolicyHandler:
* Common base class to parse login and force extension install lists.

* Switch --enable-login-apps to enable login apps.
* Extension pref extensions.install.loginlist.

BUG=576464
==========

to

==========
Plumbing for login apps device policy to extensions.

This CL succeeeds the CL #2150483004

ChromeProcessManagerDelegate:
* Enable background pages for apps in the login profile when
command-line switch --enable-login-apps is set.

ExtensionManagement:
* Factor out common code into GetInstallListByMode from
GetForceInstallList and GetRecommendedInstallList.
* Common function UpdateForcedExtensions.

ExtensionSystemImpl:
* Enable extensions in login profile when command-line switch
--enable-login-apps is set.

ExternalProviderImpl:
* In CreateExternalProviders, create an ExternalPolicyLoader for login
apps in the login profile.

ExtensionInstallListPolicyHandler:
* Common base class to parse login and force extension install lists.

* Switch --enable-login-apps to enable login apps.
* Extension pref extensions.install.loginlist.

BUG=576464

Committed: https://crrev.com/3bbfabe266cd5138393d3e87229de567d19da005
Cr-Commit-Position: refs/heads/master@{#432917}
==========

[commit-bot: I haz the power, 2016-11-17 18:32:55]

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

[hashimoto, 2016-11-18 07:41:51]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/2509753006/ by hashimoto@chromium.org.

The reason for reverting is: After this change, Chrome keeps crashing on the
login screen (reproduced on a minnie device)..

[Devlin, 2017-03-01 16:14:59]

rdevlin.cronin@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Devlin, 2017-03-01 16:15:01]

https://codereview.chromium.org/2306143002/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/extension_management.cc (right):

https://codereview.chromium.org/2306143002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_management.cc:118: new
base::DictionaryValue);
prefer base::MakeUnique

https://codereview.chromium.org/2306143002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_management.cc:119: for (const
SettingsIdMap::value_type& it : settings_by_id_) {
nit: I'd say using const auto& here is just as readable (since
SettingsIdMap::value_type doesn't tell me much more about the type).

https://codereview.chromium.org/2306143002/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/extension_system_impl.cc (right):

https://codereview.chromium.org/2306143002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_system_impl.cc:212: extensions_enabled =
true;
Seems like this should instead go where we call Init from in ProfileManager(),
since that has a bit more knowledge of profiles.

https://codereview.chromium.org/2306143002/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/external_provider_impl.cc (right):

https://codereview.chromium.org/2306143002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/external_provider_impl.cc:502: if
(chromeos::ProfileHelper::IsSigninProfile(profile)) {
Should we check if the command line switch is present?

https://codereview.chromium.org/2306143002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/external_provider_impl.cc:512: return;
Are we certain we don't want any other providers in this case?

https://codereview.chromium.org/2306143002/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/policy_handlers.cc (right):

https://codereview.chromium.org/2306143002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/policy_handlers.cc:201: // -----------------------
nit: no need for a new line with this.

https://codereview.chromium.org/2306143002/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/policy_handlers.h (right):

https://codereview.chromium.org/2306143002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/policy_handlers.h:67: const char* pref_name() const {
return pref_name_; }
Is this needed?