Apps&Extensions for supervised users: Add Extension::WAS_INSTALLED_BY_CUSTODIAN flag and proto entry

Apps&Extensions for supervised users: Add Extension::WAS_INSTALLED_BY_CUSTODIAN flag and proto entry.

Currently, supervised users can not use Apps/Extensions at all. In the future, their custodian will be able to remotely install Apps/Extensions for them. This CL introduces a new extension flag and corresponding proto entry that marks these extensions, so we can allow only those in the client.

Relevant design doc section: https://docs.google.com/a/google.com/document/d/1rqahhrErR_HiZ04v-Skb_kvXeyOfnTvBPFvKwAxuf10/edit#heading=h.ptivtbjwlda5

BUG=390520
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283522

[Marc Treib, 2014-07-01 12:50:19]

PTAL!

bauerb: Please look at c/b/supervised_user/
kalman: Please look at c/b/extensions/ and extensions/common/
pkotwicz: Please look at c/b/themes/
zea: Please look at sync/protocol/

[Bernhard Bauer, 2014-07-01 13:25:42]

Description nit: Please add the title as the first line of the description (the
description is the only thing that will later become the commit message).

supervised_user/ LGTM

[not at google - send to devlin, 2014-07-01 17:22:47]

https://codereview.chromium.org/353493002/diff/20001/extensions/common/extens...
File extensions/common/extension.h (right):

https://codereview.chromium.org/353493002/diff/20001/extensions/common/extens...
extensions/common/extension.h:174: // custodian of a supervised user.
this would be better as a flag in ExtensionPrefs

[not at google - send to devlin, 2014-07-01 17:23:30]

https://codereview.chromium.org/353493002/diff/20001/extensions/common/extens...
File extensions/common/extension.h (right):

https://codereview.chromium.org/353493002/diff/20001/extensions/common/extens...
extensions/common/extension.h:174: // custodian of a supervised user.
On 2014/07/01 17:22:47, kalman wrote:
> this would be better as a flag in ExtensionPrefs

like
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

[Nicolas Zea, 2014-07-01 17:39:14]

sync lgtm

[Marc Treib, 2014-07-02 11:58:12]

https://codereview.chromium.org/353493002/diff/20001/extensions/common/extens...
File extensions/common/extension.h (right):

https://codereview.chromium.org/353493002/diff/20001/extensions/common/extens...
extensions/common/extension.h:174: // custodian of a supervised user.
On 2014/07/01 17:22:47, kalman wrote:
> this would be better as a flag in ExtensionPrefs

The problem with that approach is that ExtensionPrefs are populated only after a
successful installation, but I want to use this flag to check if the
installation should be allowed in the first place (in
SupervisedUserService::UserMayLoad). Populating just this one pref before the
installation is probably possible, but seems like a bad idea - WDYT?

[not at google - send to devlin, 2014-07-02 16:18:56]

https://codereview.chromium.org/353493002/diff/20001/chrome/browser/themes/th...
File chrome/browser/themes/theme_syncable_service.cc (right):

https://codereview.chromium.org/353493002/diff/20001/chrome/browser/themes/th...
chrome/browser/themes/theme_syncable_service.cc:245: kRemoteInstall,
(this seems like another candidate for using InstallFlags)

https://codereview.chromium.org/353493002/diff/20001/extensions/common/extens...
File extensions/common/extension.h (right):

https://codereview.chromium.org/353493002/diff/20001/extensions/common/extens...
extensions/common/extension.h:174: // custodian of a supervised user.
On 2014/07/02 11:58:12, treib wrote:
> On 2014/07/01 17:22:47, kalman wrote:
> > this would be better as a flag in ExtensionPrefs
> 
> The problem with that approach is that ExtensionPrefs are populated only after
a
> successful installation, but I want to use this flag to check if the
> installation should be allowed in the first place (in
> SupervisedUserService::UserMayLoad). Populating just this one pref before the
> installation is probably possible, but seems like a bad idea - WDYT?

I see. I agree that writing something to preferences *before* installation would
be wrong.

Ideally many of these flags wouldn't be here, though. They're concerned with how
the environment interacts with the extension, not anything to do with the
extension model. I'd rather prevent this InitFromValueFlags enum from steadily
growing as people need to attach flags for their feature. 

Perhaps you can pass through the InstallFlag to the UserMayLoad method? I put a
comment on an example of where another place it'd be nice. I'm a little worried
that the end result here is threading through an install flag basically
everywhere, and that would be bad.

Perhaps we should be passing around PendingExtensionInfo objects (and
incidentally PendingExtensionInfo should be holding onto an InstallFlag not all
of those bools), not Extensions? That actually seems more correct in the case of
deciding if an Extension can be installed.

Any ideas?

[Marc Treib, 2014-07-07 08:58:17]

https://codereview.chromium.org/353493002/diff/20001/extensions/common/extens...
File extensions/common/extension.h (right):

https://codereview.chromium.org/353493002/diff/20001/extensions/common/extens...
extensions/common/extension.h:174: // custodian of a supervised user.
On 2014/07/02 16:18:56, kalman wrote:
> On 2014/07/02 11:58:12, treib wrote:
> > On 2014/07/01 17:22:47, kalman wrote:
> > > this would be better as a flag in ExtensionPrefs
> > 
> > The problem with that approach is that ExtensionPrefs are populated only
after
> a
> > successful installation, but I want to use this flag to check if the
> > installation should be allowed in the first place (in
> > SupervisedUserService::UserMayLoad). Populating just this one pref before
the
> > installation is probably possible, but seems like a bad idea - WDYT?
> 
> I see. I agree that writing something to preferences *before* installation
would
> be wrong.
> 
> Ideally many of these flags wouldn't be here, though. They're concerned with
how
> the environment interacts with the extension, not anything to do with the
> extension model. I'd rather prevent this InitFromValueFlags enum from steadily
> growing as people need to attach flags for their feature. 
> 
> Perhaps you can pass through the InstallFlag to the UserMayLoad method? I put
a
> comment on an example of where another place it'd be nice. I'm a little
worried
> that the end result here is threading through an install flag basically
> everywhere, and that would be bad.
> 
> Perhaps we should be passing around PendingExtensionInfo objects (and
> incidentally PendingExtensionInfo should be holding onto an InstallFlag not
all
> of those bools), not Extensions? That actually seems more correct in the case
of
> deciding if an Extension can be installed.
> 
> Any ideas?

At the time UserMayLoad is called, the extension may or may not be already
installed, so passing around PendingExtensionInfos or InstallFlags doesn't seem
right either.
I can't see a good way forward here - maybe you can add someone more
knowledgeable about the extension system than me, so we'll get some ideas?

[Bernhard Bauer, 2014-07-07 09:11:58]

https://codereview.chromium.org/353493002/diff/20001/extensions/common/extens...
File extensions/common/extension.h (right):

https://codereview.chromium.org/353493002/diff/20001/extensions/common/extens...
extensions/common/extension.h:174: // custodian of a supervised user.
On 2014/07/07 08:58:17, treib wrote:
> On 2014/07/02 16:18:56, kalman wrote:
> > On 2014/07/02 11:58:12, treib wrote:
> > > On 2014/07/01 17:22:47, kalman wrote:
> > > > this would be better as a flag in ExtensionPrefs
> > > 
> > > The problem with that approach is that ExtensionPrefs are populated only
> after
> > a
> > > successful installation, but I want to use this flag to check if the
> > > installation should be allowed in the first place (in
> > > SupervisedUserService::UserMayLoad). Populating just this one pref before
> the
> > > installation is probably possible, but seems like a bad idea - WDYT?
> > 
> > I see. I agree that writing something to preferences *before* installation
> would
> > be wrong.
> > 
> > Ideally many of these flags wouldn't be here, though. They're concerned with
> how
> > the environment interacts with the extension, not anything to do with the
> > extension model. I'd rather prevent this InitFromValueFlags enum from
steadily
> > growing as people need to attach flags for their feature. 
> > 
> > Perhaps you can pass through the InstallFlag to the UserMayLoad method? I
put
> a
> > comment on an example of where another place it'd be nice. I'm a little
> worried
> > that the end result here is threading through an install flag basically
> > everywhere, and that would be bad.
> > 
> > Perhaps we should be passing around PendingExtensionInfo objects (and
> > incidentally PendingExtensionInfo should be holding onto an InstallFlag not
> all
> > of those bools), not Extensions? That actually seems more correct in the
case
> of
> > deciding if an Extension can be installed.
> > 
> > Any ideas?
> 
> At the time UserMayLoad is called, the extension may or may not be already
> installed, so passing around PendingExtensionInfos or InstallFlags doesn't
seem
> right either.
> I can't see a good way forward here - maybe you can add someone more
> knowledgeable about the extension system than me, so we'll get some ideas?

Does it actually ever make sense to call UserMayLoad for an extension that is
already installed? Maybe we could catch those cases before calling out to
UserMayLoad.

[Marc Treib, 2014-07-07 09:20:04]

https://codereview.chromium.org/353493002/diff/20001/extensions/common/extens...
File extensions/common/extension.h (right):

https://codereview.chromium.org/353493002/diff/20001/extensions/common/extens...
extensions/common/extension.h:174: // custodian of a supervised user.
On 2014/07/07 09:11:58, Bernhard Bauer wrote:
> On 2014/07/07 08:58:17, treib wrote:
> > On 2014/07/02 16:18:56, kalman wrote:
> > > On 2014/07/02 11:58:12, treib wrote:
> > > > On 2014/07/01 17:22:47, kalman wrote:
> > > > > this would be better as a flag in ExtensionPrefs
> > > > 
> > > > The problem with that approach is that ExtensionPrefs are populated only
> > after
> > > a
> > > > successful installation, but I want to use this flag to check if the
> > > > installation should be allowed in the first place (in
> > > > SupervisedUserService::UserMayLoad). Populating just this one pref
before
> > the
> > > > installation is probably possible, but seems like a bad idea - WDYT?
> > > 
> > > I see. I agree that writing something to preferences *before* installation
> > would
> > > be wrong.
> > > 
> > > Ideally many of these flags wouldn't be here, though. They're concerned
with
> > how
> > > the environment interacts with the extension, not anything to do with the
> > > extension model. I'd rather prevent this InitFromValueFlags enum from
> steadily
> > > growing as people need to attach flags for their feature. 
> > > 
> > > Perhaps you can pass through the InstallFlag to the UserMayLoad method? I
> put
> > a
> > > comment on an example of where another place it'd be nice. I'm a little
> > worried
> > > that the end result here is threading through an install flag basically
> > > everywhere, and that would be bad.
> > > 
> > > Perhaps we should be passing around PendingExtensionInfo objects (and
> > > incidentally PendingExtensionInfo should be holding onto an InstallFlag
not
> > all
> > > of those bools), not Extensions? That actually seems more correct in the
> case
> > of
> > > deciding if an Extension can be installed.
> > > 
> > > Any ideas?
> > 
> > At the time UserMayLoad is called, the extension may or may not be already
> > installed, so passing around PendingExtensionInfos or InstallFlags doesn't
> seem
> > right either.
> > I can't see a good way forward here - maybe you can add someone more
> > knowledgeable about the extension system than me, so we'll get some ideas?
> 
> Does it actually ever make sense to call UserMayLoad for an extension that is
> already installed? Maybe we could catch those cases before calling out to
> UserMayLoad.

E.g. on a policy change, all existing (installed) extensions should be checked
again. And maybe there are other cases?

[not at google - send to devlin, 2014-07-07 15:03:51]

On 2014/07/07 09:20:04, treib wrote:
>
https://codereview.chromium.org/353493002/diff/20001/extensions/common/extens...
> File extensions/common/extension.h (right):
> 
>
https://codereview.chromium.org/353493002/diff/20001/extensions/common/extens...
> extensions/common/extension.h:174: // custodian of a supervised user.
> On 2014/07/07 09:11:58, Bernhard Bauer wrote:
> > On 2014/07/07 08:58:17, treib wrote:
> > > On 2014/07/02 16:18:56, kalman wrote:
> > > > On 2014/07/02 11:58:12, treib wrote:
> > > > > On 2014/07/01 17:22:47, kalman wrote:
> > > > > > this would be better as a flag in ExtensionPrefs
> > > > > 
> > > > > The problem with that approach is that ExtensionPrefs are populated
only
> > > after
> > > > a
> > > > > successful installation, but I want to use this flag to check if the
> > > > > installation should be allowed in the first place (in
> > > > > SupervisedUserService::UserMayLoad). Populating just this one pref
> before
> > > the
> > > > > installation is probably possible, but seems like a bad idea - WDYT?
> > > > 
> > > > I see. I agree that writing something to preferences *before*
installation
> > > would
> > > > be wrong.
> > > > 
> > > > Ideally many of these flags wouldn't be here, though. They're concerned
> with
> > > how
> > > > the environment interacts with the extension, not anything to do with
the
> > > > extension model. I'd rather prevent this InitFromValueFlags enum from
> > steadily
> > > > growing as people need to attach flags for their feature. 
> > > > 
> > > > Perhaps you can pass through the InstallFlag to the UserMayLoad method?
I
> > put
> > > a
> > > > comment on an example of where another place it'd be nice. I'm a little
> > > worried
> > > > that the end result here is threading through an install flag basically
> > > > everywhere, and that would be bad.
> > > > 
> > > > Perhaps we should be passing around PendingExtensionInfo objects (and
> > > > incidentally PendingExtensionInfo should be holding onto an InstallFlag
> not
> > > all
> > > > of those bools), not Extensions? That actually seems more correct in the
> > case
> > > of
> > > > deciding if an Extension can be installed.
> > > > 
> > > > Any ideas?
> > > 
> > > At the time UserMayLoad is called, the extension may or may not be already
> > > installed, so passing around PendingExtensionInfos or InstallFlags doesn't
> > seem
> > > right either.
> > > I can't see a good way forward here - maybe you can add someone more
> > > knowledgeable about the extension system than me, so we'll get some ideas?
> > 
> > Does it actually ever make sense to call UserMayLoad for an extension that
is
> > already installed? Maybe we could catch those cases before calling out to
> > UserMayLoad.
> 
> E.g. on a policy change, all existing (installed) extensions should be checked
> again. And maybe there are other cases?

I'm not sure whether UserMayLoad is checked on policy change, though I imagine
it is (at least transitively). We certainly check it every time the extension
system starts up though, whether or not those extensions are installed.

Anyway, I see your point about the PendingExtensionInfo, that doesn't seem
right. InstallFlags seems fine, if it's possible to pass that through to
UserMayLoad, if you think of it as "flags that are set on installation" versus
"flags that only exist during installation".

I'll CC yoz@ on this thread for thoughts.

[not at google - send to devlin, 2014-07-07 15:04:04]

and mek@

[Marc Treib, 2014-07-07 15:24:11]

On 2014/07/07 15:03:51, kalman wrote:
> On 2014/07/07 09:20:04, treib wrote:
> >
>
https://codereview.chromium.org/353493002/diff/20001/extensions/common/extens...
> > File extensions/common/extension.h (right):
> > 
> >
>
https://codereview.chromium.org/353493002/diff/20001/extensions/common/extens...
> > extensions/common/extension.h:174: // custodian of a supervised user.
> > On 2014/07/07 09:11:58, Bernhard Bauer wrote:
> > > On 2014/07/07 08:58:17, treib wrote:
> > > > On 2014/07/02 16:18:56, kalman wrote:
> > > > > On 2014/07/02 11:58:12, treib wrote:
> > > > > > On 2014/07/01 17:22:47, kalman wrote:
> > > > > > > this would be better as a flag in ExtensionPrefs
> > > > > > 
> > > > > > The problem with that approach is that ExtensionPrefs are populated
> only
> > > > after
> > > > > a
> > > > > > successful installation, but I want to use this flag to check if the
> > > > > > installation should be allowed in the first place (in
> > > > > > SupervisedUserService::UserMayLoad). Populating just this one pref
> > before
> > > > the
> > > > > > installation is probably possible, but seems like a bad idea - WDYT?
> > > > > 
> > > > > I see. I agree that writing something to preferences *before*
> installation
> > > > would
> > > > > be wrong.
> > > > > 
> > > > > Ideally many of these flags wouldn't be here, though. They're
concerned
> > with
> > > > how
> > > > > the environment interacts with the extension, not anything to do with
> the
> > > > > extension model. I'd rather prevent this InitFromValueFlags enum from
> > > steadily
> > > > > growing as people need to attach flags for their feature. 
> > > > > 
> > > > > Perhaps you can pass through the InstallFlag to the UserMayLoad
method?
> I
> > > put
> > > > a
> > > > > comment on an example of where another place it'd be nice. I'm a
little
> > > > worried
> > > > > that the end result here is threading through an install flag
basically
> > > > > everywhere, and that would be bad.
> > > > > 
> > > > > Perhaps we should be passing around PendingExtensionInfo objects (and
> > > > > incidentally PendingExtensionInfo should be holding onto an
InstallFlag
> > not
> > > > all
> > > > > of those bools), not Extensions? That actually seems more correct in
the
> > > case
> > > > of
> > > > > deciding if an Extension can be installed.
> > > > > 
> > > > > Any ideas?
> > > > 
> > > > At the time UserMayLoad is called, the extension may or may not be
already
> > > > installed, so passing around PendingExtensionInfos or InstallFlags
doesn't
> > > seem
> > > > right either.
> > > > I can't see a good way forward here - maybe you can add someone more
> > > > knowledgeable about the extension system than me, so we'll get some
ideas?
> > > 
> > > Does it actually ever make sense to call UserMayLoad for an extension that
> is
> > > already installed? Maybe we could catch those cases before calling out to
> > > UserMayLoad.
> > 
> > E.g. on a policy change, all existing (installed) extensions should be
checked
> > again. And maybe there are other cases?
> 
> I'm not sure whether UserMayLoad is checked on policy change, though I imagine
> it is (at least transitively). We certainly check it every time the extension
> system starts up though, whether or not those extensions are installed.
> 
> Anyway, I see your point about the PendingExtensionInfo, that doesn't seem
> right. InstallFlags seems fine, if it's possible to pass that through to
> UserMayLoad, if you think of it as "flags that are set on installation" versus
> "flags that only exist during installation".

InstallFlags in this form (bit mask) do only exist during installation. After
installation, some of them are mapped to ExtensionPrefs and can be queried from
there, others are dropped. So to pass them to UserMayLoad, we'd have to recreate
the bit mask out of ExtensionPrefs for already installed extensions. That also
seems awkward and fragile. :(

> I'll CC yoz@ on this thread for thoughts.

[not at google - send to devlin, 2014-07-07 15:26:08]

I filed crbug.com/391749 a few minutes ago. Maybe that's a pre-requisite after
all. An approximation you could use would be to re-create it from the existing
boolean flags we have, I suppose.

[Yoyo Zhou, 2014-07-08 01:55:11]

https://codereview.chromium.org/353493002/diff/20001/extensions/common/extens...
File extensions/common/extension.h (right):

https://codereview.chromium.org/353493002/diff/20001/extensions/common/extens...
extensions/common/extension.h:174: // custodian of a supervised user.
On 2014/07/07 09:20:04, treib wrote:
> On 2014/07/07 09:11:58, Bernhard Bauer wrote:
> > On 2014/07/07 08:58:17, treib wrote:
> > > On 2014/07/02 16:18:56, kalman wrote:
> > > > On 2014/07/02 11:58:12, treib wrote:
> > > > > On 2014/07/01 17:22:47, kalman wrote:
> > > > > > this would be better as a flag in ExtensionPrefs
> > > > > 
> > > > > The problem with that approach is that ExtensionPrefs are populated
only
> > > after
> > > > a
> > > > > successful installation, but I want to use this flag to check if the
> > > > > installation should be allowed in the first place (in
> > > > > SupervisedUserService::UserMayLoad). Populating just this one pref
> before
> > > the
> > > > > installation is probably possible, but seems like a bad idea - WDYT?
> > > > 
> > > > I see. I agree that writing something to preferences *before*
installation
> > > would
> > > > be wrong.
> > > > 
> > > > Ideally many of these flags wouldn't be here, though. They're concerned
> with
> > > how
> > > > the environment interacts with the extension, not anything to do with
the
> > > > extension model. I'd rather prevent this InitFromValueFlags enum from
> > steadily
> > > > growing as people need to attach flags for their feature. 
> > > > 
> > > > Perhaps you can pass through the InstallFlag to the UserMayLoad method?
I
> > put
> > > a
> > > > comment on an example of where another place it'd be nice. I'm a little
> > > worried
> > > > that the end result here is threading through an install flag basically
> > > > everywhere, and that would be bad.
> > > > 
> > > > Perhaps we should be passing around PendingExtensionInfo objects (and
> > > > incidentally PendingExtensionInfo should be holding onto an InstallFlag
> not
> > > all
> > > > of those bools), not Extensions? That actually seems more correct in the
> > case
> > > of
> > > > deciding if an Extension can be installed.
> > > > 
> > > > Any ideas?
> > > 
> > > At the time UserMayLoad is called, the extension may or may not be already
> > > installed, so passing around PendingExtensionInfos or InstallFlags doesn't
> > seem
> > > right either.
> > > I can't see a good way forward here - maybe you can add someone more
> > > knowledgeable about the extension system than me, so we'll get some ideas?
> > 

> > Does it actually ever make sense to call UserMayLoad for an extension that
is
> > already installed? Maybe we could catch those cases before calling out to
> > UserMayLoad.
> 
> E.g. on a policy change, all existing (installed) extensions should be checked
> again. And maybe there are other cases?

This already happens. We re-check on startup in case the policy blacklist has
changed. See installed_loader.cc:172. We also check whenever we call
ExtensionService::CheckManagementPolicy (which apparently is also only at
startup).

Generally speaking, the install flag seems fine to me for install time from sync
(there are no real good alternatives), and as kalman says, a pref in
ExtensionPrefs is what we should check at load time.

[Marijn Kruisselbrink, 2014-07-08 15:23:07]

I'm not sure I quite follow what this flag is supposed to do? If only custodians
are allowed to installs apps/extensions, doesn't that mean that by definition
all installed apps/extensions where custodian installed? And if you'd somehow
wanted to enforce this more strictly you could have changes on the sync server
side to only allow apps/extensions to be added via this special
installed-by-custodian path?
I'm just really not sure what you gain by having a flag on the chrome side for
this. What will this allow chrome to do differently?

[Marc Treib, 2014-07-09 15:43:24]

On 2014/07/08 15:23:07, Marijn Kruisselbrink wrote:
> I'm not sure I quite follow what this flag is supposed to do? If only
custodians
> are allowed to installs apps/extensions, doesn't that mean that by definition
> all installed apps/extensions where custodian installed? And if you'd somehow
> wanted to enforce this more strictly you could have changes on the sync server
> side to only allow apps/extensions to be added via this special
> installed-by-custodian path?
> I'm just really not sure what you gain by having a flag on the chrome side for
> this. What will this allow chrome to do differently?

Let's keep that discussion in the design doc
(https://docs.google.com/a/google.com/document/d/1rqahhrErR_HiZ04v-Skb_kvXeyOf...)
and come back here if we decide that we do need the flag.

[Marc Treib, 2014-07-10 11:59:55]

Alright, since there's agreement now that we actually want the flag, we're back
to the question of where to put it.

The last suggestion was to put it into the install_flags
(extensions::InstallFlag), which are stored in ExtensionPrefs. Since these are
populated only after the extension is installed, we would then have to
explicitly pass the install_flags around in UserMayLoad. My concern with this is
the following: Most places that call ManagementPolicy::UserMayLoad don't know or
care whether the extension is already installed, and they shouldn't have to. To
pass in the install_flags, however, they would need to know so that they can get
them from the right place.

Given all that, it seems to me that the original proposal with a flag in
creation_flags (aka Extension::InitFromValueFlags) might be the least evil after
all. Note also that the creation_flags are stored in ExtensionPrefs as well, so
after installation, the flag will end up there either way.

[not at google - send to devlin, 2014-07-10 15:17:50]

On 2014/07/10 11:59:55, treib wrote:
> Alright, since there's agreement now that we actually want the flag, we're
back
> to the question of where to put it.
> 
> The last suggestion was to put it into the install_flags
> (extensions::InstallFlag), which are stored in ExtensionPrefs. Since these are
> populated only after the extension is installed, we would then have to
> explicitly pass the install_flags around in UserMayLoad. My concern with this
is
> the following: Most places that call ManagementPolicy::UserMayLoad don't know
or
> care whether the extension is already installed, and they shouldn't have to.
To
> pass in the install_flags, however, they would need to know so that they can
get
> them from the right place.
> 
> Given all that, it seems to me that the original proposal with a flag in
> creation_flags (aka Extension::InitFromValueFlags) might be the least evil
after
> all. Note also that the creation_flags are stored in ExtensionPrefs as well,
so
> after installation, the flag will end up there either way.

InstallFlag isn't populated after extension installation, it's used to determine
how extensions should be installed, then parts of it are stored in extension
prefs. The reason I don't like it in InitFromValue is because it has nothing to
do with how an extension should be parsed, and that's what those flags are
supposed to be for.

[Marc Treib, 2014-07-11 09:03:22]

On 2014/07/10 15:17:50, kalman (OOO July 11-14) wrote:
> On 2014/07/10 11:59:55, treib wrote:
> > Alright, since there's agreement now that we actually want the flag, we're
> back
> > to the question of where to put it.
> > 
> > The last suggestion was to put it into the install_flags
> > (extensions::InstallFlag), which are stored in ExtensionPrefs. Since these
are
> > populated only after the extension is installed, we would then have to
> > explicitly pass the install_flags around in UserMayLoad. My concern with
this
> is
> > the following: Most places that call ManagementPolicy::UserMayLoad don't
know
> or
> > care whether the extension is already installed, and they shouldn't have to.
> To
> > pass in the install_flags, however, they would need to know so that they can
> get
> > them from the right place.
> > 
> > Given all that, it seems to me that the original proposal with a flag in
> > creation_flags (aka Extension::InitFromValueFlags) might be the least evil
> after
> > all. Note also that the creation_flags are stored in ExtensionPrefs as well,
> so
> > after installation, the flag will end up there either way.
> 
> InstallFlag isn't populated after extension installation, it's used to
determine
> how extensions should be installed, then parts of it are stored in extension
> prefs. The reason I don't like it in InitFromValue is because it has nothing
to
> do with how an extension should be parsed, and that's what those flags are
> supposed to be for.

Then I guess most of the flags in InitFromValueFlags shouldn't really be there?
So maybe their purpose needs to be redefined?
But okay, if you are convinced that passing the install_flags around explicitly
in UserMayLoad is the way to go, then I'll implement that.

[Marc Treib, 2014-07-14 13:41:42]

On 2014/07/11 09:03:22, treib wrote:
> On 2014/07/10 15:17:50, kalman (OOO July 11-14) wrote:
> > On 2014/07/10 11:59:55, treib wrote:
> > > Alright, since there's agreement now that we actually want the flag, we're
> > back
> > > to the question of where to put it.
> > > 
> > > The last suggestion was to put it into the install_flags
> > > (extensions::InstallFlag), which are stored in ExtensionPrefs. Since these
> are
> > > populated only after the extension is installed, we would then have to
> > > explicitly pass the install_flags around in UserMayLoad. My concern with
> this
> > is
> > > the following: Most places that call ManagementPolicy::UserMayLoad don't
> know
> > or
> > > care whether the extension is already installed, and they shouldn't have
to.
> > To
> > > pass in the install_flags, however, they would need to know so that they
can
> > get
> > > them from the right place.
> > > 
> > > Given all that, it seems to me that the original proposal with a flag in
> > > creation_flags (aka Extension::InitFromValueFlags) might be the least evil
> > after
> > > all. Note also that the creation_flags are stored in ExtensionPrefs as
well,
> > so
> > > after installation, the flag will end up there either way.
> > 
> > InstallFlag isn't populated after extension installation, it's used to
> determine
> > how extensions should be installed, then parts of it are stored in extension
> > prefs. The reason I don't like it in InitFromValue is because it has nothing
> to
> > do with how an extension should be parsed, and that's what those flags are
> > supposed to be for.
> 
> Then I guess most of the flags in InitFromValueFlags shouldn't really be
there?
> So maybe their purpose needs to be redefined?
> But okay, if you are convinced that passing the install_flags around
explicitly
> in UserMayLoad is the way to go, then I'll implement that.

CL that adds install_flags param to UserMayLoad:
https://codereview.chromium.org/384423002/

[not at google - send to devlin, 2014-07-15 16:11:23]

> Then I guess most of the flags in InitFromValueFlags shouldn't really be
there?

Yeah.

> So maybe their purpose needs to be redefined?

That's an option. I just filed crbug.com/393982 for this, sorry about all of the
back and forth.

> But okay, if you are convinced that passing the install_flags around
explicitly
> in UserMayLoad is the way to go, then I'll implement that.

Thanks. I'll comment there.

[not at google - send to devlin, 2014-07-15 16:21:38]

lgtm

[pkotwicz, 2014-07-16 02:20:38]

chrome/browser/themes/theme_syncable_service.cc LGTM

[Marc Treib, 2014-07-16 08:04:22]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2014-07-16 08:07:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/treib@chromium.org/353493002/40001

[commit-bot: I haz the power, 2014-07-16 21:42:14]

Change committed as 283522