Make sure the google wallet in-app payment support app is always installed.

Make sure the google wallet in-app payment support app is always installed.

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

[Marijn Kruisselbrink, 2013-05-21 21:46:25]

This I think is the most straight forward way of having the wallet app installed
as a default app.
I'm not entirely happy about hardcoding the ID of the app when determining which
apps to show in the chrome://extensions page, maybe I should change that instead
to hide all apps for which ShouldDisplayInAppLauncher and
ShouldDisplayInNewTabPage are false (which is a subset of component apps + the
wallet app, so it should be safe to do that).

[asargent_no_longer_on_chrome, 2013-05-22 00:07:13]

Mostly fine with a couple of nits.

I'm slightly wary of using the term "component" for these for 2 reasons: we tend
to conflate that in other areas of the code with "things that have super
powers", and we also have "chrome components" like the PDF reader, Flash plugin,
etc. that are updated via the component updater. 

Can we come up with a different name? Maybe "external default" or something?

https://codereview.chromium.org/15612002/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/external_component_loader.cc (right):

https://codereview.chromium.org/15612002/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/external_component_loader.cc:17:
prefs_->SetString(appId + ".name", "Google Wallet Service");
Does this string ever get shown to users? If so, does it need to be localized?
If not, can we make it optional (I assume this is just for placeholder until the
actual app gets installed from the webstore)?

https://codereview.chromium.org/15612002/diff/3001/chrome/common/extensions/e...
File chrome/common/extensions/extension_constants.h (right):

https://codereview.chromium.org/15612002/diff/3001/chrome/common/extensions/e...
chrome/common/extensions/extension_constants.h:164: extern const char
kIAPSupportAppId[];
nit: probably better not to abbreviate "in app payments" since that isn't a
well-known acronym. Better to make this something like:

kInAppPaymentSupportAppId

[Marijn Kruisselbrink, 2013-05-22 21:29:37]

On 2013/05/22 00:07:13, Antony Sargent wrote:
> I'm slightly wary of using the term "component" for these for 2 reasons: we
tend
> to conflate that in other areas of the code with "things that have super
> powers", and we also have "chrome components" like the PDF reader, Flash
plugin,
> etc. that are updated via the component updater. 
> 
> Can we come up with a different name? Maybe "external default" or something?
I'm not sure if "external default" isn't confusing too, since there already is
something with that name too (the default apps on chrome os for example) and
those are different in that they are user visible, uninstallable and only apply
to new profiles.
Just like regular component extensions, this app is also hidden in the UI, not
uninstallable, etc, but I agree that component is a term that is too overloaded.

I think ExternalComponentLoader more accurately reflects the fact that this
class loads a list of extensions/apps that are a fundamental part of chrome than
something like ExternalDefaultLoader would, but if you want, I can change it to
that.

https://codereview.chromium.org/15612002/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/external_component_loader.cc (right):

https://codereview.chromium.org/15612002/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/external_component_loader.cc:17:
prefs_->SetString(appId + ".name", "Google Wallet Service");
On 2013/05/22 00:07:13, Antony Sargent wrote:
> Does this string ever get shown to users? If so, does it need to be localized?
> If not, can we make it optional (I assume this is just for placeholder until
the
> actual app gets installed from the webstore)?
It seems this string isn't used at all, I added it because it's also there for
all the apps in
src/chrome/browser/extensions/default_extensions/chromeos/external_extensions.json,
but since it isn't actually used afaict I've just removed it.

https://codereview.chromium.org/15612002/diff/3001/chrome/common/extensions/e...
File chrome/common/extensions/extension_constants.h (right):

https://codereview.chromium.org/15612002/diff/3001/chrome/common/extensions/e...
chrome/common/extensions/extension_constants.h:164: extern const char
kIAPSupportAppId[];
On 2013/05/22 00:07:13, Antony Sargent wrote:
> nit: probably better not to abbreviate "in app payments" since that isn't a
> well-known acronym. Better to make this something like:
> 
> kInAppPaymentSupportAppId
> 

Done.

[asargent_no_longer_on_chrome, 2013-05-22 23:42:33]

Ok, you've convinced me that "external component" is the least bad. =)

lgtm

[commit-bot: I haz the power, 2013-05-22 23:44:40]

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

[commit-bot: I haz the power, 2013-05-23 00:08:11]

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

[commit-bot: I haz the power, 2013-05-23 04:47:08]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-05-23 04:51:49]

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

[commit-bot: I haz the power, 2013-05-23 12:57:52]

Change committed as 201773

[Dmitry Polukhin, 2013-08-14 19:39:12]

https://chromiumcodereview.appspot.com/15612002/diff/21001/chrome/browser/ext...
File chrome/browser/extensions/external_provider_impl.cc (right):

https://chromiumcodereview.appspot.com/15612002/diff/21001/chrome/browser/ext...
chrome/browser/extensions/external_provider_impl.cc:463:
provider_list->push_back(
FYI, I'm going to move this provider under "if (!is_managed_profile)" because
Chrome OS supervised user don't have GAIA and there is no sense to install this
app for them (all other default apps are excluded for supervised users). Please
let me know if you have any concerns.

[Marijn Kruisselbrink, 2013-08-14 19:44:58]

On 2013/08/14 19:39:12, Dmitry Polukhin wrote:
>
https://chromiumcodereview.appspot.com/15612002/diff/21001/chrome/browser/ext...
> File chrome/browser/extensions/external_provider_impl.cc (right):
> 
>
https://chromiumcodereview.appspot.com/15612002/diff/21001/chrome/browser/ext...
> chrome/browser/extensions/external_provider_impl.cc:463:
> provider_list->push_back(
> FYI, I'm going to move this provider under "if (!is_managed_profile)" because
> Chrome OS supervised user don't have GAIA and there is no sense to install
this
> app for them (all other default apps are excluded for supervised users).
Please
> let me know if you have any concerns.
I'm not entirely sure what Chrome OS supervised users are, but I guess that's
fine. If a user doesn't have GAIA there is indeed no way for them to make in-app
payments anyway, so there doesn't seem to be much point in having this
installed.

[Mattias Nissler (ping if slow), 2013-09-05 08:19:06]

https://chromiumcodereview.appspot.com/15612002/diff/21001/chrome/browser/ext...
File chrome/browser/extensions/external_provider_impl.cc (right):

https://chromiumcodereview.appspot.com/15612002/diff/21001/chrome/browser/ext...
chrome/browser/extensions/external_provider_impl.cc:469:
Manifest::EXTERNAL_POLICY_DOWNLOAD,
Is the EXTERNAL_POLICY_DOWNLOAD location intentional here? That may trigger
various policy-specific extension restrictions and messaging, which may not be
appropriate for the payment app.

[asargent_no_longer_on_chrome, 2013-09-05 21:19:15]

https://chromiumcodereview.appspot.com/15612002/diff/21001/chrome/browser/ext...
File chrome/browser/extensions/external_provider_impl.cc (right):

https://chromiumcodereview.appspot.com/15612002/diff/21001/chrome/browser/ext...
chrome/browser/extensions/external_provider_impl.cc:469:
Manifest::EXTERNAL_POLICY_DOWNLOAD,
On 2013/09/05 08:19:06, Mattias Nissler wrote:
> Is the EXTERNAL_POLICY_DOWNLOAD location intentional here? That may trigger
> various policy-specific extension restrictions and messaging, which may not be
> appropriate for the payment app.

Whether it was intentional or not, it is incorrect as you point out (also it
messes up our UMA metrics). I've opened crbug.com/285983 to fix it.