Plumbing for login apps device policy to extensions.

Plumbing for login apps device policy to extensions.

Device policy kDeviceLoginScreenAppInstallList is parsed and stored as the pref kInstallLoginScreenAppList.

ExtensionManagement:
* Factor out common code into GetInstallListByMode from
GetForceInstallList and GetRecommendedInstallList.
* Common function UpdateForcedExtensions.
* is_signin_profile so kInstallLoginScreenAppList is only set in the chromeos signin profile.

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

ExtensionPref:
* extensions.install.login_screen_app_list.

BUG=576464
Review-Url: https://codereview.chromium.org/2144313002
Cr-Commit-Position: refs/heads/master@{#455434}
Committed: https://chromium.googlesource.com/chromium/src/+/4607f0768d8b4ab1d3856d886ed1d5e2e0dd3766

[achuithb, 2016-07-14 15:35:01]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2016-07-14 15:35:17]

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

[achuithb, 2016-07-14 15:35:55]

[email protected] changed reviewers:
+ [email protected]

[achuithb, 2016-07-14 15:35:55]

Max, let me know what you think. This has just the parts that convert policy to
prefs and then settings in ExtensionManagement.

[achuithb, 2016-07-14 15:36:27]

The complete CL is here:
https://codereview.chromium.org/2150483004/

[achuithb, 2016-07-14 15:45:39]

On 2016/07/14 15:36:27, achuithb wrote:
> The complete CL is here:
> https://codereview.chromium.org/2150483004/

Part II of this CL is here: https://codereview.chromium.org/2150483004/

[commit-bot: I haz the power, 2016-07-14 16:39:45]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2016-07-14 16:39:45]

Dry run: This issue passed the CQ dry run.

[achuithb, 2016-07-15 08:45:22]

[email protected] changed reviewers:
+ [email protected]

[emaxx, 2016-07-15 13:22:31]

https://codereview.chromium.org/2144313002/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_management.cc (right):

https://codereview.chromium.org/2144313002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_management.cc:319:
UpdateForcedExtensions(login_list_pref);
I think this line should be performed only if the current profile is the login
profile. Otherwise, these login-screen apps would be applied to all users in the
system, which isn't really a good thing.

https://codereview.chromium.org/2144313002/diff/1/chrome/browser/extensions/p...
File chrome/browser/extensions/policy_handlers.h (right):

https://codereview.chromium.org/2144313002/diff/1/chrome/browser/extensions/p...
chrome/browser/extensions/policy_handlers.h:84:
~ExtensionInstallForcelistPolicyHandler() override;
I guess, defining the destructor explicitly isn't necessary in these cases
(which just result in calling parent's destructor).

[achuithb, 2016-07-21 00:16:05]

Please excuse the rebase.

There's also a CHECK that's triggered that I need to fix at
schema_registry.cc:58

https://codereview.chromium.org/2144313002/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_management.cc (right):

https://codereview.chromium.org/2144313002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_management.cc:319:
UpdateForcedExtensions(login_list_pref);
On 2016/07/15 13:22:31, emaxx wrote:
> I think this line should be performed only if the current profile is the login
> profile. Otherwise, these login-screen apps would be applied to all users in
the
> system, which isn't really a good thing.

Done.

https://codereview.chromium.org/2144313002/diff/1/chrome/browser/extensions/p...
File chrome/browser/extensions/policy_handlers.h (right):

https://codereview.chromium.org/2144313002/diff/1/chrome/browser/extensions/p...
chrome/browser/extensions/policy_handlers.h:84:
~ExtensionInstallForcelistPolicyHandler() override;
On 2016/07/15 13:22:31, emaxx wrote:
> I guess, defining the destructor explicitly isn't necessary in these cases
> (which just result in calling parent's destructor).

I'd like to keep this for consistency with the rest of the classes in this file.

[achuithb, 2017-02-21 14:35:00]

Patchset #4 (id:60001) has been deleted

[achuithb, 2017-02-21 14:35:09]

Patchset #3 (id:40001) has been deleted

[achuithb, 2017-02-21 15:50:41]

Patchset #4 (id:100001) has been deleted

[achuithb, 2017-02-21 16:06:28]

Description was changed from

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

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

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

ExtensionPref:
* extensions.install.loginlist.

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

to

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

Device policy kLoginApps is parsed and stored as the pref kInstallLoginList.

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

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

ExtensionPref:
* extensions.install.loginlist.

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

[achuithb, 2017-02-21 16:07:15]

[email protected] changed reviewers:
- [email protected]

[achuithb, 2017-02-21 16:17:25]

Maksim, could you PTAL?

I've reconciled this CL with Denis's work. The points of difference are:
1. I keep a Profile* instead of PrefService* in ExtensionManagement 
2. I keep a single pref instead of the 2 prefs that Denis maintains.
3. I continue to use loginlist instead of signinlist. The options I can think of
are loginlist, signinlist, loginappslist, loginscreenlist (list of extensions is
implicit), loginscreenappslist. Maybe we should pick one of the longer words to
be consistent with the German way of doing things.
3. Minor changes to incorporate the latest fashions like MakeUnique and
c++11isms.

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

https://codereview.chromium.org/2144313002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/extension_management.cc:327: if
(chromeos::ProfileHelper::IsSigninProfile(profile_))
This is a change from Denis's CL.

Please note that I need a Profile* for this.

[emaxx, 2017-02-21 19:43:50]

Looks good in general.


On 2017/02/21 16:17:25, achuithb wrote:
> 3. I continue to use loginlist instead of signinlist. The options I can think
of
> are loginlist, signinlist, loginappslist, loginscreenlist (list of extensions
is
> implicit), loginscreenappslist. Maybe we should pick one of the longer words
to
> be consistent with the German way of doing things.

Yes, let's maybe wait for the policy renaming before deciding on that?
I'd personally prefer a longer name for clarity purposes, but let's see.


On 2017/02/21 16:17:25, achuithb wrote:
> Maksim, could you PTAL?
> 
> I've reconciled this CL with Denis's work. The points of difference are:
> 1. I keep a Profile* instead of PrefService* in ExtensionManagement 
> 2. I keep a single pref instead of the 2 prefs that Denis maintains.
> 3. I continue to use loginlist instead of signinlist. The options I can think
of
> are loginlist, signinlist, loginappslist, loginscreenlist (list of extensions
is
> implicit), loginscreenappslist. Maybe we should pick one of the longer words
to
> be consistent with the German way of doing things.
> 3. Minor changes to incorporate the latest fashions like MakeUnique and
> c++11isms.
> 
>
https://codereview.chromium.org/2144313002/diff/120001/chrome/browser/extensi...
> File chrome/browser/extensions/extension_management.cc (right):
> 
>
https://codereview.chromium.org/2144313002/diff/120001/chrome/browser/extensi...
> chrome/browser/extensions/extension_management.cc:327: if
> (chromeos::ProfileHelper::IsSigninProfile(profile_))
> This is a change from Denis's CL.
> 
> Please note that I need a Profile* for this.

[emaxx, 2017-02-21 19:44:10]

https://codereview.chromium.org/2144313002/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/2144313002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/extension_service.cc:1494: DVLOG(1) << "AddExtension "
<< extension->name() << ", " << extension->id();
nit: Maybe format it differently, so that it doesn't look like a list of two
items? E.g. "Adding extension <name> (<id>)" or something like that.

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

https://codereview.chromium.org/2144313002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/policy_handlers.h:52: class
ExtensionInstallListPolicyHandler
nit: Maybe add comments to explain what this class and the next two ones do?

https://codereview.chromium.org/2144313002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/policy_handlers.h:62:
ExtensionInstallListPolicyHandler(const char* policy_name,
nit: I believe std::string is used much more often in the Chromium code base.

[achuithb, 2017-02-22 14:39:34]

PTAL, Max.

https://codereview.chromium.org/2144313002/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/2144313002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/extension_service.cc:1494: DVLOG(1) << "AddExtension "
<< extension->name() << ", " << extension->id();
On 2017/02/21 19:44:10, emaxx wrote:
> nit: Maybe format it differently, so that it doesn't look like a list of two
> items? E.g. "Adding extension <name> (<id>)" or something like that.

I got rid of this change; it's useful for my debugging but may not be something
that the extension team wants.

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

https://codereview.chromium.org/2144313002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/policy_handlers.h:26: ExtensionListPolicyHandler(const
char* policy_name,
Note policy_name is of type const char* here.

https://codereview.chromium.org/2144313002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/policy_handlers.h:52: class
ExtensionInstallListPolicyHandler
On 2017/02/21 19:44:10, emaxx wrote:
> nit: Maybe add comments to explain what this class and the next two ones do?

Done.

https://codereview.chromium.org/2144313002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/policy_handlers.h:62:
ExtensionInstallListPolicyHandler(const char* policy_name,
On 2017/02/21 19:44:10, emaxx wrote:
> nit: I believe std::string is used much more often in the Chromium code base.

I'm happy to change this if you insist, but this is not consistent with the type
of policy_name in ExtensionListPolicyHandler. The net effect is that we convert
a const char* to a std::string and need to use c_str() to get a const char* back
immediately.

There's a stronger argument for pref_name, since we're storing this value as an
unowned pointer, but again the type of pref_names::kInstallForceList is const
char[]. It's also a bit weird to have one arg be const char* and the other be
const std::string&.

Let me know what you'd like me to do. My personal inclination is to leave this
as is.

[emaxx, 2017-02-24 04:12:40]

Now that the policy rename in http://crrev.com/2711553002 has landed, could you
please rebase and update the CL to include the new (longer, but I hope clearer)
naming everywhere?

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

https://codereview.chromium.org/2144313002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/policy_handlers.h:62:
ExtensionInstallListPolicyHandler(const char* policy_name,
On 2017/02/22 14:39:34, achuithb wrote:
> On 2017/02/21 19:44:10, emaxx wrote:
> > nit: I believe std::string is used much more often in the Chromium code
base.
> 
> I'm happy to change this if you insist, but this is not consistent with the
type
> of policy_name in ExtensionListPolicyHandler. The net effect is that we
convert
> a const char* to a std::string and need to use c_str() to get a const char*
back
> immediately.
> 
> There's a stronger argument for pref_name, since we're storing this value as
an
> unowned pointer, but again the type of pref_names::kInstallForceList is const
> char[]. It's also a bit weird to have one arg be const char* and the other be
> const std::string&.
> 
> Let me know what you'd like me to do. My personal inclination is to leave this
> as is.

Acknowledged.

[achuithb, 2017-02-27 14:01:51]

The CQ bit was checked by [email protected] to run a CQ dry run

[achuithb, 2017-02-27 14:02:03]

Patchset #6 (id:160001) has been deleted

[commit-bot: I haz the power, 2017-02-27 14:02:07]

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

[achuithb, 2017-02-27 14:05:03]

I've rebased and renamed the policy/pref. PTAL Max

https://codereview.chromium.org/2144313002/diff/140001/extensions/browser/ext...
File extensions/browser/extension_prefs.cc (right):

https://codereview.chromium.org/2144313002/diff/140001/extensions/browser/ext...
extensions/browser/extension_prefs.cc:320: base::debug::SetCrashKeyValue(
This change is due to the rebase.

[commit-bot: I haz the power, 2017-02-27 14:13:57]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2017-02-27 14:13:58]

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

[emaxx, 2017-02-27 17:29:59]

On 2017/02/27 14:05:03, achuithb wrote:
> I've rebased and renamed the policy/pref. PTAL Max

Looks good to me, but seems that some tests need fixing.

[achuithb, 2017-03-01 10:57:22]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 10:57:38]

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, 2017-03-01 11:44:08]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2017-03-01 11:44:08]

Dry run: This issue passed the CQ dry run.

[achuithb, 2017-03-01 13:13:19]

The CQ bit was checked by [email protected] to run a CQ dry run

[achuithb, 2017-03-01 13:13:29]

Patchset #7 (id:200001) has been deleted

[commit-bot: I haz the power, 2017-03-01 13:13:36]

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

[achuithb, 2017-03-01 13:13:46]

On 2017/02/27 17:29:59, emaxx wrote:
> On 2017/02/27 14:05:03, achuithb wrote:
> > I've rebased and renamed the policy/pref. PTAL Max
> 
> Looks good to me, but seems that some tests need fixing.

PTAL Max

[emaxx, 2017-03-01 13:54:36]

LGTM

https://codereview.chromium.org/2144313002/diff/220001/extensions/browser/pre...
File extensions/browser/pref_names.h (right):

https://codereview.chromium.org/2144313002/diff/220001/extensions/browser/pre...
extensions/browser/pref_names.h:72: // on ChromeOS at startup time. It is a list
of strings, each string contains
nit: s/ChromeOS/Chrome OS/

[commit-bot: I haz the power, 2017-03-01 14:00:34]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2017-03-01 14:00:35]

Dry run: This issue passed the CQ dry run.

[achuithb, 2017-03-01 14:09:12]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 14:09:19]

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

[achuithb, 2017-03-01 14:11:28]

[email protected] changed reviewers:
+ [email protected]

[achuithb, 2017-03-01 14:11:29]

Devlin, could you PTAL as owner.

Note that a similar CL was landed a while ago that Anthony reviewed:
https://codereview.chromium.org/2306143002

Feel free to ping me if you have any questions.

https://codereview.chromium.org/2144313002/diff/220001/extensions/browser/pre...
File extensions/browser/pref_names.h (right):

https://codereview.chromium.org/2144313002/diff/220001/extensions/browser/pre...
extensions/browser/pref_names.h:72: // on ChromeOS at startup time. It is a list
of strings, each string contains
On 2017/03/01 13:54:36, emaxx wrote:
> nit: s/ChromeOS/Chrome OS/

Done.

[commit-bot: I haz the power, 2017-03-01 15:06:52]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2017-03-01 15:06:54]

Dry run: This issue passed the CQ dry run.

[achuithb, 2017-03-03 11:13:13]

Devlin - gentle ping. You inadvertently left feedback on the closed CL.

[Devlin, 2017-03-03 17:24:48]

On 2017/03/03 11:13:13, achuithb wrote:
> Devlin - gentle ping. You inadvertently left feedback on the closed CL.

D'oh!  Opening codereview links from emails strikes again!  Thanks for the ping,
and sorry for the delay.


The original CL hid some of this behavior behind a commandline flag; is that
intentionally omitted?

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

https://codereview.chromium.org/2144313002/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/extension_management.cc:118:
ExtensionManagement::GetInstallListByMode(
implementation order should (try) to match the declaration order in the header.

https://codereview.chromium.org/2144313002/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/extension_management.cc:263:
LoadPreference(pref_names::kInstallLoginScreenAppList, true,
Can we gate loading this on whether or not this is a sign-in profile? 
Presumably it won't succeed otherwise, but it'd be cheaper to check a bool than
perform a dictionary lookup.

https://codereview.chromium.org/2144313002/diff/240001/chrome/browser/extensi...
File chrome/browser/extensions/extension_management.h (right):

https://codereview.chromium.org/2144313002/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/extension_management.h:73: explicit
ExtensionManagement(Profile* profile);
nit: I think I'd prefer we just pass in a bool is_signin_profile here, rather
than keeping the full profile.  This would also reduce a bit of the churn
elsewhere in the CL.

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

https://codereview.chromium.org/2144313002/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/policy_handlers.h:68: const char* pref_name() const {
return pref_name_; }
needed?

[achuithb, 2017-03-06 15:14:41]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2017-03-06 15:15:07]

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

[achuithb, 2017-03-06 15:24:42]

On 2017/03/03 17:24:48, Devlin wrote:
> On 2017/03/03 11:13:13, achuithb wrote:
> > Devlin - gentle ping. You inadvertently left feedback on the closed CL.
> 
> D'oh!  Opening codereview links from emails strikes again!  Thanks for the
ping,
> and sorry for the delay.
> 
> 
> The original CL hid some of this behavior behind a commandline flag; is that
> intentionally omitted?
> 

I've broken the original CL into 2 parts. This 'safe' CL handles the parsing and
the wiring up of device policy to extension prefs. It doesn't actually install
apps into the signin profile. The CL that actually does that is here:
https://codereview.chromium.org/2149953002/
This CL has the flag that gates loading login screen apps, and it depends on the
current CL.

As a heads up, this will be coming your way shortly after I've worked out one
more kink, and gone through a review with Maksim :)

[achuithb, 2017-03-06 15:25:32]

Thanks for the review, Devlin. PTAL!

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

https://codereview.chromium.org/2144313002/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/extension_management.cc:118:
ExtensionManagement::GetInstallListByMode(
On 2017/03/03 17:24:47, Devlin wrote:
> implementation order should (try) to match the declaration order in the
header.

Done.

https://codereview.chromium.org/2144313002/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/extension_management.cc:263:
LoadPreference(pref_names::kInstallLoginScreenAppList, true,
On 2017/03/03 17:24:48, Devlin wrote:
> Can we gate loading this on whether or not this is a sign-in profile? 
> Presumably it won't succeed otherwise, but it'd be cheaper to check a bool
than
> perform a dictionary lookup.

Done. I've moved up the IsSigninProfile check to here instead of
UpdateForcedExtensions (we don't need to check in both places), and this removes
one #if defined(OS_CHROMEOS) block as well.

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

https://codereview.chromium.org/2144313002/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/policy_handlers.h:68: const char* pref_name() const {
return pref_name_; }
On 2017/03/03 17:24:48, Devlin wrote:
> needed?

Done.

[achuithb, 2017-03-06 15:26:04]

https://codereview.chromium.org/2144313002/diff/240001/chrome/browser/extensi...
File chrome/browser/extensions/extension_management.h (right):

https://codereview.chromium.org/2144313002/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/extension_management.h:73: explicit
ExtensionManagement(Profile* profile);
On 2017/03/03 17:24:48, Devlin wrote:
> nit: I think I'd prefer we just pass in a bool is_signin_profile here, rather
> than keeping the full profile.  This would also reduce a bit of the churn
> elsewhere in the CL.

Done.

[commit-bot: I haz the power, 2017-03-06 16:11:01]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2017-03-06 16:11:02]

Dry run: This issue passed the CQ dry run.

[achuithb, 2017-03-06 17:19:47]

Patchset #10 (id:280001) has been deleted

[Devlin, 2017-03-07 19:42:13]

lgtm

[achuithb, 2017-03-08 00:14:16]

On 2017/03/07 19:42:13, Devlin wrote:
> lgtm

Thank you!

[achuithb, 2017-03-08 00:14:26]

The CQ bit was checked by [email protected]

[achuithb, 2017-03-08 00:14:26]

The patchset sent to the CQ was uploaded after l-g-t-m from [email protected]
Link to the patchset: https://codereview.chromium.org/2144313002/#ps260001
(title: "Devlin feedback")

[commit-bot: I haz the power, 2017-03-08 00:15:11]

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

[emaxx, 2017-03-08 00:17:38]

Sorry, but if it's still not too late - please update the CL description (new
pref names, etc.).

[achuithb, 2017-03-08 00:20:21]

The CQ bit was unchecked by [email protected]

[achuithb, 2017-03-08 00:20:37]

On 2017/03/08 00:17:38, emaxx wrote:
> Sorry, but if it's still not too late - please update the CL description (new
> pref names, etc.).

Ok, will do!

[achuithb, 2017-03-08 10:34:40]

Description was changed from

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

Device policy kLoginApps is parsed and stored as the pref kInstallLoginList.

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

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

ExtensionPref:
* extensions.install.loginlist.

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

to

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

Device policy kDeviceLoginScreenAppInstallList is parsed and stored as the pref
kInstallLoginScreenAppList.

ExtensionManagement:
* Factor out common code into GetInstallListByMode from
GetForceInstallList and GetRecommendedInstallList.
* Common function UpdateForcedExtensions.
* is_signin_profile so kInstallLoginScreenAppList is only set in the chromeos
signin profile.

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

ExtensionPref:
* extensions.install.login_screen_app_list.

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

[achuithb, 2017-03-08 10:34:52]

The CQ bit was checked by [email protected]

[commit-bot: I haz the power, 2017-03-08 10:35:12]

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

[achuithb, 2017-03-08 10:35:52]

On 2017/03/08 00:17:38, emaxx wrote:
> Sorry, but if it's still not too late - please update the CL description (new
> pref names, etc.).

Done

[commit-bot: I haz the power, 2017-03-08 11:49:17]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1488969292379190,
"parent_rev": "bdc24416fa0ca3fa946506f299d3d36d39cb36a1", "commit_rev":
"4607f0768d8b4ab1d3856d886ed1d5e2e0dd3766"}

[commit-bot: I haz the power, 2017-03-08 11:49:54]

Description was changed from

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

Device policy kDeviceLoginScreenAppInstallList is parsed and stored as the pref
kInstallLoginScreenAppList.

ExtensionManagement:
* Factor out common code into GetInstallListByMode from
GetForceInstallList and GetRecommendedInstallList.
* Common function UpdateForcedExtensions.
* is_signin_profile so kInstallLoginScreenAppList is only set in the chromeos
signin profile.

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

ExtensionPref:
* extensions.install.login_screen_app_list.

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

to

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

Device policy kDeviceLoginScreenAppInstallList is parsed and stored as the pref
kInstallLoginScreenAppList.

ExtensionManagement:
* Factor out common code into GetInstallListByMode from
GetForceInstallList and GetRecommendedInstallList.
* Common function UpdateForcedExtensions.
* is_signin_profile so kInstallLoginScreenAppList is only set in the chromeos
signin profile.

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

ExtensionPref:
* extensions.install.login_screen_app_list.

BUG=576464

Review-Url: https://codereview.chromium.org/2144313002
Cr-Commit-Position: refs/heads/master@{#455434}
Committed:
https://chromium.googlesource.com/chromium/src/+/4607f0768d8b4ab1d3856d886ed1...
==========

[commit-bot: I haz the power, 2017-03-08 11:49:56]

Committed patchset #9 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/4607f0768d8b4ab1d3856d886ed1...