Implement new strategy for default apps

Implement new strategy for default apps. Instead of using 
the component extension system, simply install them as 
normal apps the first time Chrome runs. Remove the old
style  default apps.

This also removes support for --disable-apps, which doesn't 
seem necessary since we already have --disable-extensions.

BUG=53972
TEST=Uninstall any installed apps. Remove the 
"default_apps_installed" and "app_promo_counter" keys from 
Preferences if they are present.

Start Chrome with the --enable-default-apps flag, navigate
to chrome://extensions/, and click "update now" in the
developer tools. This is to make the  updater grab the
apps from the store quicker, so that you don't have to wait
while testing. Normally Chrome would do this by itself
after 5-40 mins.

Go to NTP. You should see gmail, calendar, and docs default 
apps show up along with the promo (the text that says "New! 
A world of ...").

Refresh the NTP 10 times. The promo should go away on the 
10th time.

There are other details. To test each of these, remove all 
installed apps and remove the keys from the preferences to 
reset the state.

* If you close Chrome before getting the default apps. They 
should still show up the next time Chrome is started (after 
clicking 'Update now')

* Clicking "hide this message" should hide the promo and it 
shouldn't come back on refresh.

* Installing or uninstalling any app should hide the promo, 
and it shouldn't come back.

* If the keys from the preferences are removed, but apps
are left installed, the default apps should not be
installed  when clicking 'update now'.

* The default apps should only be installed in locale
en-US.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=62084

[Aaron Boodman, 2010-10-08 07:56:27]

Looks scarier than it is.

Start at default_apps*, then look at extensions_service*. The rest is just
tearing out the old component apps stuff and --disable-apps.

[Erik does not do reviews, 2010-10-08 18:33:40]

some initial comments to get started (I'm not done reviewing yet)

http://codereview.chromium.org/3522015/diff/2001/1058
File chrome/browser/extensions/default_apps.cc (right):

http://codereview.chromium.org/3522015/diff/2001/1058#newcode38
chrome/browser/extensions/default_apps.cc:38:
std::includes(installed_ids.begin(), installed_ids.end(),
My best guess is that this is here to catch the case of people who already have
other apps installed.  If so, then the installed_ids == ids_ test in
CheckShouldShowPromo is wrong, since it will immediately cause the promo to be
hidden.  Given that, either it should be changed to includes() as well, or this
code should just early out if there are other ids present.

In either case, I'd add a unit test to exercise this case.

http://codereview.chromium.org/3522015/diff/2001/1058#newcode44
chrome/browser/extensions/default_apps.cc:44: bool
DefaultApps::CheckShouldShowPromo(const ExtensionIdSet& installed_ids) {
With this name, it seems odd that this method has side effects.  Maybe something
like UpdateShouldShowPromo?

http://codereview.chromium.org/3522015/diff/2001/1058#newcode66
chrome/browser/extensions/default_apps.cc:66:
prefs_->SetInteger(prefs::kAppsPromoCounter, kAppsPromoCounterMax);
replace these two with SetPromoCounter(kAppsPromoCounterMax)

http://codereview.chromium.org/3522015/diff/2001/1059
File chrome/browser/extensions/default_apps.h (right):

http://codereview.chromium.org/3522015/diff/2001/1059#newcode74
chrome/browser/extensions/default_apps.h:74: };
add DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/3522015/diff/2001/1063
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/3522015/diff/2001/1063#newcode745
chrome/browser/extensions/extensions_service.cc:745: // We do not know if the id
refers to a theme, so make the expected type
We do not know the expected type, so...

http://codereview.chromium.org/3522015/diff/2001/1063#newcode777
chrome/browser/extensions/extensions_service.cc:777: LOG(DFATAL) << "Trying to
add extension " << id
so if I have gmail installed already it's a fatal error?

http://codereview.chromium.org/3522015/diff/2001/1063#newcode785
chrome/browser/extensions/extensions_service.cc:785: Extension::INTERNAL);
For histograms, it would be really useful to be able to identify which apps were
installed via this mechanism (we can't just test the id list since that might
change over time).  I can understand why we want to use INTERNAL here, so if we
can't change that, maybe we need to add another flag somewhere?

http://codereview.chromium.org/3522015/diff/2001/1066
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/3522015/diff/2001/1066#newcode234
chrome/browser/profile.cc:234: virtual DefaultApps* GetDefaultApps() {
why not hang this off of ExtensionsService?

http://codereview.chromium.org/3522015/diff/2001/1068
File chrome/browser/profile_impl.cc (right):

http://codereview.chromium.org/3522015/diff/2001/1068#newcode423
chrome/browser/profile_impl.cc:423: if
(g_browser_process->GetApplicationLocale() != "en-US")
I wonder if we need to always enable ChromeOS as well?

[Aaron Boodman, 2010-10-08 19:37:46]

http://codereview.chromium.org/3522015/diff/2001/1058
File chrome/browser/extensions/default_apps.cc (right):

http://codereview.chromium.org/3522015/diff/2001/1058#newcode38
chrome/browser/extensions/default_apps.cc:38:
std::includes(installed_ids.begin(), installed_ids.end(),
On 2010/10/08 18:33:40, Erik Kay wrote:
> My best guess is that this is here to catch the case of people who already
have
> other apps installed.  If so, then the installed_ids == ids_ test in
> CheckShouldShowPromo is wrong, since it will immediately cause the promo to be
> hidden.  Given that, either it should be changed to includes() as well, or
this
> code should just early out if there are other ids present.

I don't follow. This code is just trying to know when the default apps have been
successfully installed (as opposed to just being kicked off -- there's a chance
not install even though they are queued). The reason it uses includes() instead
of == is because there could be apps installed whie the default apps are
queued/installing. If this used ==, in that case we'd never mark the default
apps as installed and we'd keep trying forever.

> In either case, I'd add a unit test to exercise this case.

Ok.

http://codereview.chromium.org/3522015/diff/2001/1058#newcode44
chrome/browser/extensions/default_apps.cc:44: bool
DefaultApps::CheckShouldShowPromo(const ExtensionIdSet& installed_ids) {
On 2010/10/08 18:33:40, Erik Kay wrote:
> With this name, it seems odd that this method has side effects.  Maybe
something
> like UpdateShouldShowPromo?

I know. I had trouble coming up with a good name. That usually means that the
method does too much. At one point, I had the method split into
ShouldShowPromo() and DidShowPromo(), and it didn't work for some reason. I'll
investigate.

[Erik does not do reviews, 2010-10-08 19:44:55]

http://codereview.chromium.org/3522015/diff/2001/1058
File chrome/browser/extensions/default_apps.cc (right):

http://codereview.chromium.org/3522015/diff/2001/1058#newcode38
chrome/browser/extensions/default_apps.cc:38:
std::includes(installed_ids.begin(), installed_ids.end(),
On 2010/10/08 19:37:46, Aaron Boodman wrote:
> On 2010/10/08 18:33:40, Erik Kay wrote:
> > My best guess is that this is here to catch the case of people who already
> have
> > other apps installed.  If so, then the installed_ids == ids_ test in
> > CheckShouldShowPromo is wrong, since it will immediately cause the promo to
be
> > hidden.  Given that, either it should be changed to includes() as well, or
> this
> > code should just early out if there are other ids present.
> 
> I don't follow. This code is just trying to know when the default apps have
been
> successfully installed (as opposed to just being kicked off -- there's a
chance
> not install even though they are queued). The reason it uses includes()
instead
> of == is because there could be apps installed whie the default apps are
> queued/installing. If this used ==, in that case we'd never mark the default
> apps as installed and we'd keep trying forever.

After our chat, I get this now.  Whether the apps have been installed and
whether the promo is shown are two separate concepts, but they're somewhat
interdependent.  In this method, we're ensuring that the default apps all get
installed once.  Could you improve the comment above to clarify?

[Erik does not do reviews, 2010-10-08 21:13:21]

+skerner, dpolukhin FYI

[Aaron Boodman, 2010-10-08 22:33:38]

http://codereview.chromium.org/3522015/diff/2001/1058
File chrome/browser/extensions/default_apps.cc (right):

http://codereview.chromium.org/3522015/diff/2001/1058#newcode38
chrome/browser/extensions/default_apps.cc:38:
std::includes(installed_ids.begin(), installed_ids.end(),
On 2010/10/08 19:44:55, Erik Kay wrote:
> On 2010/10/08 19:37:46, Aaron Boodman wrote:
> > On 2010/10/08 18:33:40, Erik Kay wrote:
> > > My best guess is that this is here to catch the case of people who already
> > have
> > > other apps installed.  If so, then the installed_ids == ids_ test in
> > > CheckShouldShowPromo is wrong, since it will immediately cause the promo
to
> be
> > > hidden.  Given that, either it should be changed to includes() as well, or
> > this
> > > code should just early out if there are other ids present.
> > 
> > I don't follow. This code is just trying to know when the default apps have
> been
> > successfully installed (as opposed to just being kicked off -- there's a
> chance
> > not install even though they are queued). The reason it uses includes()
> instead
> > of == is because there could be apps installed whie the default apps are
> > queued/installing. If this used ==, in that case we'd never mark the default
> > apps as installed and we'd keep trying forever.
> 
> After our chat, I get this now.  Whether the apps have been installed and
> whether the promo is shown are two separate concepts, but they're somewhat
> interdependent.  In this method, we're ensuring that the default apps all get
> installed once.  Could you improve the comment above to clarify?

Done, and added a test of this case.

http://codereview.chromium.org/3522015/diff/2001/1058#newcode44
chrome/browser/extensions/default_apps.cc:44: bool
DefaultApps::CheckShouldShowPromo(const ExtensionIdSet& installed_ids) {
On 2010/10/08 19:37:46, Aaron Boodman wrote:
> On 2010/10/08 18:33:40, Erik Kay wrote:
> > With this name, it seems odd that this method has side effects.  Maybe
> something
> > like UpdateShouldShowPromo?
> 
> I know. I had trouble coming up with a good name. That usually means that the
> method does too much. At one point, I had the method split into
> ShouldShowPromo() and DidShowPromo(), and it didn't work for some reason. I'll
> investigate.

I split it into ShouldShowPromo() and DidShowPromo(). I remembered that what was
weird about it is that ShouldShowPromo() still need to mutate state to
immediately expire the promo if manual changes have been made. This feels less
offensive than incrementing the counter though.

http://codereview.chromium.org/3522015/diff/2001/1058#newcode66
chrome/browser/extensions/default_apps.cc:66:
prefs_->SetInteger(prefs::kAppsPromoCounter, kAppsPromoCounterMax);
On 2010/10/08 18:33:40, Erik Kay wrote:
> replace these two with SetPromoCounter(kAppsPromoCounterMax)

Done.

http://codereview.chromium.org/3522015/diff/2001/1063
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/3522015/diff/2001/1063#newcode745
chrome/browser/extensions/extensions_service.cc:745: // We do not know if the id
refers to a theme, so make the expected type
On 2010/10/08 18:33:40, Erik Kay wrote:
> We do not know the expected type, so...

Comment wasn't really adding much. Just removed it.

http://codereview.chromium.org/3522015/diff/2001/1063#newcode777
chrome/browser/extensions/extensions_service.cc:777: LOG(DFATAL) << "Trying to
add extension " << id
On 2010/10/08 18:33:40, Erik Kay wrote:
> so if I have gmail installed already it's a fatal error?

Good point. Removed.

http://codereview.chromium.org/3522015/diff/2001/1063#newcode785
chrome/browser/extensions/extensions_service.cc:785: Extension::INTERNAL);
On 2010/10/08 18:33:40, Erik Kay wrote:
> For histograms, it would be really useful to be able to identify which apps
were
> installed via this mechanism (we can't just test the id list since that might
> change over time).  I can understand why we want to use INTERNAL here, so if
we
> can't change that, maybe we need to add another flag somewhere?

Yeah, I can see that. I think we need a cleanup pass on the Location flags,
because really this is exactly what it should be used for. Maybe it should be a
bitmask or something. Is this something that can wait for a future CL?

http://codereview.chromium.org/3522015/diff/2001/1066
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/3522015/diff/2001/1066#newcode234
chrome/browser/profile.cc:234: virtual DefaultApps* GetDefaultApps() {
On 2010/10/08 18:33:40, Erik Kay wrote:
> why not hang this off of ExtensionsService?

It seemed weird having this knowledge "inside" extensions service, and I didn't
want it running during tests.

But pulling it into ES does simplify things. I've split the difference and
pulled the object into ES, but still have the actual installation of default
apps in Profile.

http://codereview.chromium.org/3522015/diff/2001/1068
File chrome/browser/profile_impl.cc (right):

http://codereview.chromium.org/3522015/diff/2001/1068#newcode423
chrome/browser/profile_impl.cc:423: if
(g_browser_process->GetApplicationLocale() != "en-US")
On 2010/10/08 18:33:40, Erik Kay wrote:
> I wonder if we need to always enable ChromeOS as well?

I checked with Kan. He said it was ok to only do this in for en-US in Chrome OS
too.

[Erik does not do reviews, 2010-10-08 22:42:22]

LGTM

[Dmitry Polukhin, 2010-10-09 05:23:56]

Few comments:
- It looks like on Chrome OS we need more predictable behavior i.e. extensions
should be installed on first user login as soon as possible. Will need to call
"force update" just after first login.
- Why default apps are only for en-US? As far as I understand for Chrome OS we
expect them for all locales.

[Dmitry Polukhin, 2010-10-09 05:27:54]

Please ignore my second comment, since Kan is OK with it. But it is strange for
me...

[Aaron Boodman, 2010-10-09 06:28:18]

On 2010/10/09 05:23:56, Dmitry Polukhin wrote:
> Few comments:
> - It looks like on Chrome OS we need more predictable behavior i.e. extensions
> should be installed on first user login as soon as possible. Will need to call
> "force update" just after first login.

I can see how that's important for Chrome OS. It would also be nice for desktop
Chrome, so I will look into it.

However it's a bit more complex than it seems since we want to avoid the apps
plopping onto the screen one by one, followed by the promo.

I can't think of any good way to achieve this. We either check the app
definitions into source (which reduces flexibility), put up a progress bar on
the NTP while apps are downloaded (seems lame), or have a ploppy experience
(also lame).

[Dmitry Polukhin, 2010-10-09 10:05:53]

On 2010/10/09 06:28:18, Aaron Boodman wrote:
> However it's a bit more complex than it seems since we want to avoid the apps
> plopping onto the screen one by one, followed by the promo.
> 
> I can't think of any good way to achieve this. We either check the app
> definitions into source (which reduces flexibility), put up a progress bar on
> the NTP while apps are downloaded (seems lame), or have a ploppy experience
> (also lame).

What about following semantic? Initially default applications are installed as
invisible and inactive (i.e. just download and copy to user's profile). When all
apps are ready, commit takes place. It makes all app simultaneously user visible
and active so we can show promo when everything is ready.

[Aaron Boodman, 2010-10-09 16:22:10]

On 2010/10/09 10:05:53, Dmitry Polukhin wrote:
> On 2010/10/09 06:28:18, Aaron Boodman wrote:
> > However it's a bit more complex than it seems since we want to avoid the
apps
> > plopping onto the screen one by one, followed by the promo.
> > 
> > I can't think of any good way to achieve this. We either check the app
> > definitions into source (which reduces flexibility), put up a progress bar
on
> > the NTP while apps are downloaded (seems lame), or have a ploppy experience
> > (also lame).
> 
> What about following semantic? Initially default applications are installed as
> invisible and inactive (i.e. just download and copy to user's profile). When
all
> apps are ready, commit takes place. It makes all app simultaneously user
visible
> and active so we can show promo when everything is ready.

With that approach, at least some users won't see the default apps and promo
when they first start Chrome OS. Is that acceptable?

[Dmitry Polukhin, 2010-10-11 08:01:32]

On 2010/10/09 16:22:10, Aaron Boodman wrote:
> With that approach, at least some users won't see the default apps and promo
> when they first start Chrome OS. Is that acceptable?

I'll need to talk with Kan about it. But for me it looks better than 5-40
minutes delay. Until we get all apps installed we may have label like  "your app
will come soon".

[Dmitry Polukhin, 2010-10-14 14:57:24]

http://codereview.chromium.org/3522015/diff/2001/1068
File chrome/browser/profile_impl.cc (right):

http://codereview.chromium.org/3522015/diff/2001/1068#newcode423
chrome/browser/profile_impl.cc:423: if
(g_browser_process->GetApplicationLocale() != "en-US")
On 2010/10/08 22:33:38, Aaron Boodman wrote:
> On 2010/10/08 18:33:40, Erik Kay wrote:
> > I wonder if we need to always enable ChromeOS as well?
> 
> I checked with Kan. He said it was ok to only do this in for en-US in Chrome
OS
> too.

What will happen if user changes language? It looks like installed apps will
continue work, is it OK?