Support remote installation of extensions and apps.

Support remote installation of extensions and apps.

To enable this support, the following things are changed:
* ExtensionSyncData, and the ExtensionSpecifics protobuf have a new
  remote_install field, to mark a remotely installed extension.
* A new DISABLE_REMOTE_INSTALL reason is added to track this state
  locally.
* CrxInstaller::allow_silent_install is changed from a bool to a
  three-state enum, where the options are:
    - install after showing a permission prompt,
    - install and grant all permissions without a prompt and
    - install without a prompt and don't grant any permissions
* AddExtensionDisabledError is modified to keep track of the disable
  reasons, and change the text that is displayed accordingly.
* ExtensionInstallPrompt has a new prompt type with different text
  for remotely installed extensions.

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

[Marijn Kruisselbrink, 2014-04-30 23:41:14]

Still TODO, tests tests tests... And actually somehow running it against a sync
server that supports this new field, instead of hacking up my local chromium
build to inject hard-coded sync data.

And an open question, currently when a user enabled an app/extension manually
(from the chrome://extensions page or by trying to launch an app), you'll only
get an install prompt if there are any permissions to show. Should this also
show a prompt for permissionless extensions if they were remotely installed?

https://codereview.chromium.org/264763002/diff/20001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/264763002/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:4112: +        The extension "<ph
name="EXTENSION_NAME">$1<ex>Flashblock</ex></ph>" was installed remotely.
These strings can probably be phrased in a way that makes more sense to users...

[Marijn Kruisselbrink, 2014-04-30 23:48:49]

Also nothing is currently behind a flag, but I'm not sure what the point would
be of having anything behind a flag? Nothing will change unless the sync server
actually sets this remote_install flag, and if the sync server does do that,
older chromes will silently install the extension, and grant it all permissions
anyway.

[not at google - send to devlin, 2014-05-01 18:55:21]

still processing this. it's more code than I expected and there are a lot of "&"
operations. i hope that there's a more central place that this sort of logic can
go, like ExtensionService for example.

pulling out that proto change into a separate CL will help me understand the
boilerplate sync changes vs the functional changes better.

also +yoz who I think owns this code, and has experience with sync/disable
subtleties.

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

https://codereview.chromium.org/264763002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/crx_installer.h:79: enum AllowSilent {
It's confusing me reading things like "allow_silent_install() ==
GRANT_AFTER_PROMPT".

Is there something more abstract that you could have? Like maybe
"InstallBehavior" (probably too generic) or... well, i'll leave you to it.

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

https://codereview.chromium.org/264763002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_disabled_ui.h:22: int disable_reasons);
mention that this is a bitmask of disable reasons

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

https://codereview.chromium.org/264763002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:1504: reasons &
Extension::DISABLE_REMOTE_INSTALL) {
seems odd to be doing both an == and a & operation on |reasons|.

https://codereview.chromium.org/264763002/diff/20001/sync/protocol/extension_...
File sync/protocol/extension_specifics.proto (right):

https://codereview.chromium.org/264763002/diff/20001/sync/protocol/extension_...
sync/protocol/extension_specifics.proto:40: optional bool remote_install = 7;
would you mind submitting this change separately; adding the proto field and
updating the sync model? that way the sync team can import this sooner.

[Marijn Kruisselbrink, 2014-05-01 22:36:56]

On 2014/05/01 18:55:21, kalman wrote:
> still processing this. it's more code than I expected and there are a lot of
"&"
> operations. i hope that there's a more central place that this sort of logic
can
> go, like ExtensionService for example.
A lot that's contributing to the amount of changed code is actually boilerplate
changes for the allow_silent_install change, or git cl format reformatting
entire blocks when I only added one line, and updating various UI elements to
have different text depending on the disable reasons... (and things like adding
an extra parameter to the ExtensionSyncData constructor required updating many
unit tests that call that constructor etc)

> pulling out that proto change into a separate CL will help me understand the
> boilerplate sync changes vs the functional changes better.
Maybe I should move a bit more of the boilerplate to that CL than I did now...
Although it's hard to decide what is "boiletplate sync changes" and what is
functional... Depending on how you look at it almost everything could be
considered boilerplate, or cosmetic (changing UI strings).

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

https://codereview.chromium.org/264763002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/crx_installer.h:79: enum AllowSilent {
On 2014/05/01 18:55:21, kalman wrote:
> It's confusing me reading things like "allow_silent_install() ==
> GRANT_AFTER_PROMPT".
It is clearly confusing me too, considering that I made at least two mistakes
replacing boolean allow_silent_install_ checks with checks for this enum...
 
> Is there something more abstract that you could have? Like maybe
> "InstallBehavior" (probably too generic) or... well, i'll leave you to it.
One way to go might be to just go back to an allow_silent_install_ bool and add
an additional grant_permissions_ bool. But it somehow seemed wrong to use two
bools for something that is only supposed to have three different states...

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

https://codereview.chromium.org/264763002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_disabled_ui.h:22: int disable_reasons);
On 2014/05/01 18:55:21, kalman wrote:
> mention that this is a bitmask of disable reasons

Done.

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

https://codereview.chromium.org/264763002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:1504: reasons &
Extension::DISABLE_REMOTE_INSTALL) {
On 2014/05/01 18:55:21, kalman wrote:
> seems odd to be doing both an == and a & operation on |reasons|.
yeah... Actually a more correct check would be to check if one of these two bits
is set, but all other bits are zero. Not sure if that check is any more readable
though.

https://codereview.chromium.org/264763002/diff/20001/sync/protocol/extension_...
File sync/protocol/extension_specifics.proto (right):

https://codereview.chromium.org/264763002/diff/20001/sync/protocol/extension_...
sync/protocol/extension_specifics.proto:40: optional bool remote_install = 7;
On 2014/05/01 18:55:21, kalman wrote:
> would you mind submitting this change separately; adding the proto field and
> updating the sync model? that way the sync team can import this sooner.
Done in https://codereview.chromium.org/264933005/

[Marijn Kruisselbrink, 2014-05-01 22:41:28]

On 2014/05/01 18:55:21, kalman wrote:
> still processing this. it's more code than I expected and there are a lot of
"&"
> operations. i hope that there's a more central place that this sort of logic
can
> go, like ExtensionService for example.
And more concretely to address your "&" concern, where essentially all the new
"&" operations are in ExtensionDisabledGlobalError, it might make sense to give
that method a ErrorType similar to ExtensionInstallPrompt::PromptType, which is
used to decide what strings to use instead of checking the reason bitmask
everywhere. But at some level checking bits in a bit mask is going to require
using operators like &...

[not at google - send to devlin, 2014-05-01 22:43:55]

On 2014/05/01 22:41:28, Marijn Kruisselbrink wrote:
> On 2014/05/01 18:55:21, kalman wrote:
> > still processing this. it's more code than I expected and there are a lot of
> "&"
> > operations. i hope that there's a more central place that this sort of logic
> can
> > go, like ExtensionService for example.
> And more concretely to address your "&" concern, where essentially all the new
> "&" operations are in ExtensionDisabledGlobalError, it might make sense to
give
> that method a ErrorType similar to ExtensionInstallPrompt::PromptType, which
is
> used to decide what strings to use instead of checking the reason bitmask
> everywhere. But at some level checking bits in a bit mask is going to require
> using operators like &...

I don't object to using &, it just looks odd to be seeing that used for control
in so many places.

[Yoyo Zhou, 2014-05-01 22:53:44]

https://chromiumcodereview.appspot.com/264763002/diff/40001/chrome/browser/ex...
File chrome/browser/extensions/crx_installer.h (right):

https://chromiumcodereview.appspot.com/264763002/diff/40001/chrome/browser/ex...
chrome/browser/extensions/crx_installer.h:80: GRANT_AFTER_PROMPT,  // No silent
install, show a prompt instead.
I think the "grant" here is superfluous. But as kalman has mentioned, the naming
here is suspect.

https://chromiumcodereview.appspot.com/264763002/diff/40001/chrome/browser/ex...
File chrome/browser/extensions/extension_install_prompt.cc (right):

https://chromiumcodereview.appspot.com/264763002/diff/40001/chrome/browser/ex...
chrome/browser/extensions/extension_install_prompt.cc:662:
prompt_.set_type(extension->is_ephemeral()
This is too complex for a ternary.

https://chromiumcodereview.appspot.com/264763002/diff/40001/chrome/browser/ex...
File chrome/browser/extensions/extension_sync_service.cc (right):

https://chromiumcodereview.appspot.com/264763002/diff/40001/chrome/browser/ex...
chrome/browser/extensions/extension_sync_service.cc:476: if
(extension_sync_data.remote_install())
I would prefer that this be written more clearly to indicate that REMOTE_INSTALL
and UNKNOWN_FROM_SYNC are mutually exclusive. You can expand the comments above
as well.

https://chromiumcodereview.appspot.com/264763002/diff/40001/sync/protocol/ext...
File sync/protocol/extension_specifics.proto (right):

https://chromiumcodereview.appspot.com/264763002/diff/40001/sync/protocol/ext...
sync/protocol/extension_specifics.proto:40: optional bool remote_install = 7;
When changing proto fields, you'll want to test the behavior when you sync
between new and old versions (in both directions). I think unexpected things can
happen when you sync a field to a client that doesn't recognize it, even though
proto are supposed to be able to cope.

https://chromiumcodereview.appspot.com/264763002/diff/60001/chrome/browser/ex...
File chrome/browser/extensions/extension_disabled_ui.cc (right):

https://chromiumcodereview.appspot.com/264763002/diff/60001/chrome/browser/ex...
chrome/browser/extensions/extension_disabled_ui.cc:144: int disable_reasons,
Why not make this a bool is_remote_install?

https://chromiumcodereview.appspot.com/264763002/diff/60001/chrome/browser/ex...
chrome/browser/extensions/extension_disabled_ui.cc:225: if (disable_reasons_ &
Extension::DISABLE_REMOTE_INSTALL)
braces for more than one line if statements

https://chromiumcodereview.appspot.com/264763002/diff/60001/chrome/browser/ex...
chrome/browser/extensions/extension_disabled_ui.cc:291:
messages.push_back(l10n_util::GetStringFUTF16(
There is some redundancy here. Consider making it less so.

https://chromiumcodereview.appspot.com/264763002/diff/60001/chrome/browser/ex...
File chrome/browser/extensions/extension_service.cc (right):

https://chromiumcodereview.appspot.com/264763002/diff/60001/chrome/browser/ex...
chrome/browser/extensions/extension_service.cc:1501: // installation is the
reason it was disabled, and no other reasons exist.
It shouldn't be possible to have PERMISSIONS_INCREASE and REMOTE_INSTALL at the
same time. (In fact, REMOTE_INSTALL should not be present with any other disable
reasons.)

[Marijn Kruisselbrink, 2014-05-05 20:45:35]

PTAL

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

https://codereview.chromium.org/264763002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/crx_installer.h:80: GRANT_AFTER_PROMPT,  // No silent
install, show a prompt instead.
On 2014/05/01 22:53:44, Yoyo Zhou wrote:
> I think the "grant" here is superfluous. But as kalman has mentioned, the
naming
> here is suspect.
Okay, I decided to skip the idea of having an enum, and instead went with two
separate bools instead.

https://codereview.chromium.org/264763002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_install_prompt.cc (right):

https://codereview.chromium.org/264763002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_install_prompt.cc:662:
prompt_.set_type(extension->is_ephemeral()
On 2014/05/01 22:53:44, Yoyo Zhou wrote:
> This is too complex for a ternary.

Done.

https://codereview.chromium.org/264763002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_sync_service.cc (right):

https://codereview.chromium.org/264763002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_sync_service.cc:476: if
(extension_sync_data.remote_install())
On 2014/05/01 22:53:44, Yoyo Zhou wrote:
> I would prefer that this be written more clearly to indicate that
REMOTE_INSTALL
> and UNKNOWN_FROM_SYNC are mutually exclusive. You can expand the comments
above
> as well.
Okay, I changed this around a bit.

https://codereview.chromium.org/264763002/diff/40001/sync/protocol/extension_...
File sync/protocol/extension_specifics.proto (right):

https://codereview.chromium.org/264763002/diff/40001/sync/protocol/extension_...
sync/protocol/extension_specifics.proto:40: optional bool remote_install = 7;
On 2014/05/01 22:53:44, Yoyo Zhou wrote:
> When changing proto fields, you'll want to test the behavior when you sync
> between new and old versions (in both directions). I think unexpected things
can
> happen when you sync a field to a client that doesn't recognize it, even
though
> proto are supposed to be able to cope.
I think this should be fine, assuming that old clients won't cause this field to
be deleted on the server...
One potentially problematic interaction happens when a user on an old client
would enable a remotely installed extension (in this case the user won't get to
see a permission prompt, since old chrome assumes that something in sync has
already prompted in the past). After this sync would have enabled == true and
remote_install == true at the same time (which means the next time a newer
client updates sync data, it will set remote_install to false, again without
ever prompting the user for permissions).

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

https://codereview.chromium.org/264763002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_disabled_ui.cc:144: int disable_reasons,
On 2014/05/01 22:53:44, Yoyo Zhou wrote:
> Why not make this a bool is_remote_install?

Done.

https://codereview.chromium.org/264763002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_disabled_ui.cc:225: if (disable_reasons_ &
Extension::DISABLE_REMOTE_INSTALL)
On 2014/05/01 22:53:44, Yoyo Zhou wrote:
> braces for more than one line if statements

Done.

https://codereview.chromium.org/264763002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_disabled_ui.cc:291:
messages.push_back(l10n_util::GetStringFUTF16(
On 2014/05/01 22:53:44, Yoyo Zhou wrote:
> There is some redundancy here. Consider making it less so.
I'm not sure what redundancy there is? Even ExtensionInstallPrompt, which mostly
uses lookup tables when different strings need to be used for different
situations still uses separate code for the few cases where the string also
varies for app vs extension.

I guess one thing that might be slightly cleaner is to add getters for the
caption and permission header strings. Is that what you mean?

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

https://codereview.chromium.org/264763002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:1501: // installation is the
reason it was disabled, and no other reasons exist.
On 2014/05/01 22:53:44, Yoyo Zhou wrote:
> It shouldn't be possible to have PERMISSIONS_INCREASE and REMOTE_INSTALL at
the
> same time. (In fact, REMOTE_INSTALL should not be present with any other
disable
> reasons.)
Currently any remotely installed extension with non trivial permissions will
have both PERMISSIONS_INCREASE and REMOTE_INSTALL at the same time. This also
ties in to my question in the original review request: "currently when a user
enabled an app/extension manually (from the chrome://extensions page or by
trying to launch an app), you'll only get an install prompt if there are any
permissions to show. Should this also show a prompt for permissionless
extensions if they were remotely installed?"

[not at google - send to devlin, 2014-05-05 21:52:51]

Yoyo has this, removing self as reviewer.

[Yoyo Zhou, 2014-05-05 22:35:16]

mostly LGTM, but I don't have confidence in the behavior of our protos

https://codereview.chromium.org/264763002/diff/40001/sync/protocol/extension_...
File sync/protocol/extension_specifics.proto (right):

https://codereview.chromium.org/264763002/diff/40001/sync/protocol/extension_...
sync/protocol/extension_specifics.proto:40: optional bool remote_install = 7;
On 2014/05/05 20:45:36, Marijn Kruisselbrink wrote:
> On 2014/05/01 22:53:44, Yoyo Zhou wrote:
> > When changing proto fields, you'll want to test the behavior when you sync
> > between new and old versions (in both directions). I think unexpected things
> can
> > happen when you sync a field to a client that doesn't recognize it, even
> though
> > proto are supposed to be able to cope.
> I think this should be fine, assuming that old clients won't cause this field
to
> be deleted on the server...

I'd strongly encourage you to test this (you may have to manually).

> One potentially problematic interaction happens when a user on an old client
> would enable a remotely installed extension (in this case the user won't get
to
> see a permission prompt, since old chrome assumes that something in sync has
> already prompted in the past). After this sync would have enabled == true and
> remote_install == true at the same time (which means the next time a newer
> client updates sync data, it will set remote_install to false, again without
> ever prompting the user for permissions).

So this would happen if the old client doesn't set remote_install (because it
doesn't know about it), right? As I mentioned, if protos worked exactly as
expected, this is what should happen, but I believe it's possible that the lack
of the field would be interpreted as "false".

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

https://codereview.chromium.org/264763002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_disabled_ui.cc:291:
messages.push_back(l10n_util::GetStringFUTF16(
On 2014/05/05 20:45:36, Marijn Kruisselbrink wrote:
> On 2014/05/01 22:53:44, Yoyo Zhou wrote:
> > There is some redundancy here. Consider making it less so.
> I'm not sure what redundancy there is? Even ExtensionInstallPrompt, which
mostly
> uses lookup tables when different strings need to be used for different
> situations still uses separate code for the few cases where the string also
> varies for app vs extension.
> 
> I guess one thing that might be slightly cleaner is to add getters for the
> caption and permission header strings. Is that what you mean?

Ah, I misread. These are not the same strings. Sorry about that.

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

https://codereview.chromium.org/264763002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:1501: // installation is the
reason it was disabled, and no other reasons exist.
On 2014/05/05 20:45:36, Marijn Kruisselbrink wrote:
> On 2014/05/01 22:53:44, Yoyo Zhou wrote:
> > It shouldn't be possible to have PERMISSIONS_INCREASE and REMOTE_INSTALL at
> the
> > same time. (In fact, REMOTE_INSTALL should not be present with any other
> disable
> > reasons.)
> Currently any remotely installed extension with non trivial permissions will
> have both PERMISSIONS_INCREASE and REMOTE_INSTALL at the same time. This also
> ties in to my question in the original review request: "currently when a user
> enabled an app/extension manually (from the chrome://extensions page or by
> trying to launch an app), you'll only get an install prompt if there are any
> permissions to show. Should this also show a prompt for permissionless
> extensions if they were remotely installed?"

It's too bad we don't have a good notifications area, because it seems like we
could just show a notification that X was installed remotely. But I'm not sure;
letting kalman answer this one.

https://codereview.chromium.org/264763002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/crx_installer.h (right):

https://codereview.chromium.org/264763002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/crx_installer.h:350: // is provided in the ctor) to
procede without showing the permissions prompt
nit: procede -> proceed

https://codereview.chromium.org/264763002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/extension_install_prompt.cc (right):

https://codereview.chromium.org/264763002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_prompt.cc:664: if
(extension->is_ephemeral()) {
nit: don't need braces if it's 1 line of text for both the conditional and the
body (yes, it's confusing =/ )

[not at google - send to devlin, 2014-05-09 14:58:52]

apparently I have a comment from days ago. don't know whether it's relevant
anymore but here you go.

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

https://codereview.chromium.org/264763002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:1501: // installation is the
reason it was disabled, and no other reasons exist.
On 2014/05/05 20:45:36, Marijn Kruisselbrink wrote:
> On 2014/05/01 22:53:44, Yoyo Zhou wrote:
> > It shouldn't be possible to have PERMISSIONS_INCREASE and REMOTE_INSTALL at
> the
> > same time. (In fact, REMOTE_INSTALL should not be present with any other
> disable
> > reasons.)
> Currently any remotely installed extension with non trivial permissions will
> have both PERMISSIONS_INCREASE and REMOTE_INSTALL at the same time. This also
> ties in to my question in the original review request: "currently when a user
> enabled an app/extension manually (from the chrome://extensions page or by
> trying to launch an app), you'll only get an install prompt if there are any
> permissions to show. Should this also show a prompt for permissionless
> extensions if they were remotely installed?"

Good question. Let's go with "whatever is easiest" and I'll follow up with the
extensions security people.

[Marijn Kruisselbrink, 2014-05-14 23:30:01]

https://codereview.chromium.org/264763002/diff/40001/sync/protocol/extension_...
File sync/protocol/extension_specifics.proto (right):

https://codereview.chromium.org/264763002/diff/40001/sync/protocol/extension_...
sync/protocol/extension_specifics.proto:40: optional bool remote_install = 7;
On 2014/05/05 22:35:16, Yoyo Zhou wrote:
> On 2014/05/05 20:45:36, Marijn Kruisselbrink wrote:
> > On 2014/05/01 22:53:44, Yoyo Zhou wrote:
> > > When changing proto fields, you'll want to test the behavior when you sync
> > > between new and old versions (in both directions). I think unexpected
things
> > can
> > > happen when you sync a field to a client that doesn't recognize it, even
> > though
> > > proto are supposed to be able to cope.
> > I think this should be fine, assuming that old clients won't cause this
field
> to
> > be deleted on the server...
> 
> I'd strongly encourage you to test this (you may have to manually).
Unfortunately I haven't been able to test chrome with a local sync server (that
actually understands this new field). In current chrome ToT logging with a
gaiastaging account and/or using a local sync server is completely broken (can't
log in, if I trick it to be logged in various sync things crash chrome).
(actually, I think I did manage to log in with an older chrome version, so I
should be able to test the behavior of an older chrome with this new field, but
not sure how I'd actually see what happens to this field if a client that
doesn't know about the field is the only one I can get to talk to my sync
server).

> 
> > One potentially problematic interaction happens when a user on an old client
> > would enable a remotely installed extension (in this case the user won't get
> to
> > see a permission prompt, since old chrome assumes that something in sync has
> > already prompted in the past). After this sync would have enabled == true
and
> > remote_install == true at the same time (which means the next time a newer
> > client updates sync data, it will set remote_install to false, again without
> > ever prompting the user for permissions).
> 
> So this would happen if the old client doesn't set remote_install (because it
> doesn't know about it), right? As I mentioned, if protos worked exactly as
> expected, this is what should happen, but I believe it's possible that the
lack
> of the field would be interpreted as "false".
Well, there isn't much we can do about old chrome versions... If the result of
an old chrome version being signed in to sync is that it will always set this
field to "false", than that is what happens, and nothing we can do about it. I
think this is more a question of how the sync server deals with missing fields
than how old clients do. If the server does it "right" (where right means not
change the field when a client doesn't mention the field at all), things will
work as expected.

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

https://codereview.chromium.org/264763002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:1501: // installation is the
reason it was disabled, and no other reasons exist.
On 2014/05/09 14:58:52, kalman wrote:
> On 2014/05/05 20:45:36, Marijn Kruisselbrink wrote:
> > On 2014/05/01 22:53:44, Yoyo Zhou wrote:
> > > It shouldn't be possible to have PERMISSIONS_INCREASE and REMOTE_INSTALL
at
> > the
> > > same time. (In fact, REMOTE_INSTALL should not be present with any other
> > disable
> > > reasons.)
> > Currently any remotely installed extension with non trivial permissions will
> > have both PERMISSIONS_INCREASE and REMOTE_INSTALL at the same time. This
also
> > ties in to my question in the original review request: "currently when a
user
> > enabled an app/extension manually (from the chrome://extensions page or by
> > trying to launch an app), you'll only get an install prompt if there are any
> > permissions to show. Should this also show a prompt for permissionless
> > extensions if they were remotely installed?"
> 
> Good question. Let's go with "whatever is easiest" and I'll follow up with the
> extensions security people.
Since the conclusion seems to have been that we should always show this prompt,
I changed the code to do that (which actually turned out to be a fairly trivial
change).

https://codereview.chromium.org/264763002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/crx_installer.h (right):

https://codereview.chromium.org/264763002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/crx_installer.h:350: // is provided in the ctor) to
procede without showing the permissions prompt
On 2014/05/05 22:35:17, Yoyo Zhou wrote:
> nit: procede -> proceed

Done.

https://codereview.chromium.org/264763002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/extension_install_prompt.cc (right):

https://codereview.chromium.org/264763002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_prompt.cc:664: if
(extension->is_ephemeral()) {
On 2014/05/05 22:35:17, Yoyo Zhou wrote:
> nit: don't need braces if it's 1 line of text for both the conditional and the
> body (yes, it's confusing =/ )

Done.

[Marijn Kruisselbrink, 2014-05-14 23:30:56]

+pkotwicz for chrome/browser/themes/

[not at google - send to devlin, 2014-05-14 23:33:49]

Joel and/or Mustafa there is a question for you in there.

[pkotwicz, 2014-05-14 23:37:02]

themes LGTM

[meacer, 2014-05-15 00:01:10]

> Should this also show a prompt for permissionless
> extensions if they were remotely installed?

Yes, let's show it. Looks like that's what Marijn implemented anyways, so lgtm.
Btw, can you install a theme or a hosted app this way?

https://codereview.chromium.org/264763002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/extension_disabled_ui.cc (right):

https://codereview.chromium.org/264763002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_disabled_ui.cc:314: return
l10n_util::GetStringUTF16(IDS_EXTENSION_PROMPT_RE_ENABLE_BUTTON);
nit: might want to use

return is_remote_install_ ? IDS_EXTENSION_PROMPT_REMOTE_INSTALL_BUTTON :
                                         IDS_EXTENSION_PROMPT_RE_ENABLE_BUTTON;

instead to avoid a possible "not all paths return value" error during compile.

https://codereview.chromium.org/264763002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/extension_install_prompt.cc (right):

https://codereview.chromium.org/264763002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_prompt.cc:659: is_remote_install =
is_remote_install = install_ui_->profile() && ... might be cleaner?

https://codereview.chromium.org/264763002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/extension_install_prompt.h (right):

https://codereview.chromium.org/264763002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_prompt.h:70: REMOTE_INSTALL_PROMPT,
ExtensionInstallPromptType in histograms.xml corresponds to this enum and also
needs to be updated.

[Marijn Kruisselbrink, 2014-05-15 17:05:02]

On 2014/05/15 00:01:10, Mustafa Emre Acer wrote:
> Btw, can you install a theme or a hosted app this way?
Themes, not at the moment (themes use completely separate sync protobufs,
although we could add a remote_install field to those too of course). Hosted
apps are treated the same as any other app for sync, so those would be
installable this way (assuming the server side webstore code lets you install
them).

https://codereview.chromium.org/264763002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/extension_install_prompt.cc (right):

https://codereview.chromium.org/264763002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_prompt.cc:659: is_remote_install =
On 2014/05/15 00:01:10, Mustafa Emre Acer wrote:
> is_remote_install = install_ui_->profile() && ... might be cleaner?

Done.

https://codereview.chromium.org/264763002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/extension_install_prompt.h (right):

https://codereview.chromium.org/264763002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_prompt.h:70: REMOTE_INSTALL_PROMPT,
On 2014/05/15 00:01:10, Mustafa Emre Acer wrote:
> ExtensionInstallPromptType in histograms.xml corresponds to this enum and also
> needs to be updated.
Done, and I also forgot to add the new histogram to histograms.xml.

[Marijn Kruisselbrink, 2014-05-15 17:16:06]

+mpearson for tools/metrics/histograms/histograms.xml
+zea for chrome/browser/sync/

[Mark P, 2014-05-15 21:59:51]

https://codereview.chromium.org/264763002/diff/190027/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/264763002/diff/190027/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:5919: +  <owner>Please list the metric's
owners. Add more owner tags as needed.</owner>
nit: put an appropriate username here.

[Nicolas Zea, 2014-05-15 22:00:10]

lgtm

[Marijn Kruisselbrink, 2014-05-15 22:16:56]

https://codereview.chromium.org/264763002/diff/190027/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/264763002/diff/190027/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:5919: +  <owner>Please list the metric's
owners. Add more owner tags as needed.</owner>
On 2014/05/15 21:59:52, Mark P wrote:
> nit: put an appropriate username here.

Done.

[Mark P, 2014-05-15 22:38:42]

histograms.xml lgtm

[Marijn Kruisselbrink, 2014-05-16 01:00:56]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2014-05-16 01:01:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mek@chromium.org/264763002/210001

[Marijn Kruisselbrink, 2014-05-16 01:01:49]

The CQ bit was unchecked by mek@chromium.org

[Marijn Kruisselbrink, 2014-05-16 01:01:51]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2014-05-16 01:03:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mek@chromium.org/264763002/210001

[commit-bot: I haz the power, 2014-05-16 02:06:32]

Change committed as 270900

[aurimas (slooooooooow), 2014-05-16 23:22:11]

On 2014/05/16 02:06:32, I haz the power (commit-bot) wrote:
> Change committed as 270900

This broke Android ARM64 builder:
http://build.chromium.org/p/chromium.fyi/builders/Android%20ARM64%20Builder%2...

extension_service_unittest.cc:3045

[Marijn Kruisselbrink, 2014-05-16 23:51:26]

On 2014/05/16 23:22:11, aurimas wrote:
> On 2014/05/16 02:06:32, I haz the power (commit-bot) wrote:
> > Change committed as 270900
> 
> This broke Android ARM64 builder:
>
http://build.chromium.org/p/chromium.fyi/builders/Android%20ARM64%20Builder%2...
> 
> extension_service_unittest.cc:3045
From other places in the code, this seems to be a compiler bug... I'll change it
the same way it got changed in that other spot. Is there a trybot for whatever
configuration this is, or will I just have to hope I'll have fixed it?