Disable install of new default_apps for existing users

Disable install of new default apps for existing users.
Also moves the DefaultAppsTest in extension_service_unittest.cc to a separate file.

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

[Mihai Parparita -not on Chrome, 2012-08-27 19:31:55]

https://chromiumcodereview.appspot.com/10875065/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/external_provider_impl.cc (right):

https://chromiumcodereview.appspot.com/10875065/diff/1/chrome/browser/extensi...
chrome/browser/extensions/external_provider_impl.cc:230: if
((!default_apps::isMigration) || IsOldDefaultApp(extension_id)) {
Reading global variables like this is a pretty big hack. It also spreads
knowledge of default apps across more files, instead of being localized in
default_apps.cc.

How about an alterate design where there ExternalProviderImpl subclass in
default_apps.h|cc overrides SetPrefs, and if a migration is happening, filters
out the preference keys before invoking the default SetPrefs implementation?

[Mihai Parparita -not on Chrome, 2012-08-27 23:36:21]

https://chromiumcodereview.appspot.com/10875065/diff/5001/chrome/browser/exte...
File chrome/browser/extensions/default_apps.cc (right):

https://chromiumcodereview.appspot.com/10875065/diff/5001/chrome/browser/exte...
chrome/browser/extensions/default_apps.cc:22: bool isMigration;
It's a bit better now that this global is not exposed outside the file, but it's
still icky to have this separate state.

Can you instead move ShouldInstallInProfile to be a method of the Provider? Then
you can store the migration bool as a member, to later check in SetPrefs.

https://chromiumcodereview.appspot.com/10875065/diff/5001/chrome/browser/exte...
File chrome/browser/extensions/default_apps.h (right):

https://chromiumcodereview.appspot.com/10875065/diff/5001/chrome/browser/exte...
chrome/browser/extensions/default_apps.h:38: bool isOldDefaultApp(const
std::string& extension_id);
This doesn't need to be in the header file/in the default_apps namespace, since
it's only used in the .cc file. You can move it to an anonymous namespace there.

https://chromiumcodereview.appspot.com/10875065/diff/5001/chrome/browser/exte...
chrome/browser/extensions/default_apps.h:60: 
No need for this empty line.

[Gaurav, 2012-08-28 00:58:36]

https://chromiumcodereview.appspot.com/10875065/diff/2003/chrome/browser/exte...
File chrome/browser/extensions/default_apps.h (right):

https://chromiumcodereview.appspot.com/10875065/diff/2003/chrome/browser/exte...
chrome/browser/extensions/default_apps.h:28: bool isOldDefaultApp(const
std::string& extension_id);
I tried removing it and putting it in anonymous namespace but get a link error.

[Mihai Parparita -not on Chrome, 2012-08-28 18:22:59]

LGTM

https://chromiumcodereview.appspot.com/10875065/diff/11001/chrome/browser/ext...
File chrome/browser/extensions/default_apps.cc (right):

https://chromiumcodereview.appspot.com/10875065/diff/11001/chrome/browser/ext...
chrome/browser/extensions/default_apps.cc:24: const std::string gmail_id =
"pjkljhegncpnkpknbcohdijeoejaedia";
These can be moved to const char[]'s directly in the anonymous namespace,
instead of being inside the function.

https://chromiumcodereview.appspot.com/10875065/diff/11001/chrome/browser/ext...
chrome/browser/extensions/default_apps.cc:44: }
Add blank newline after this.

https://chromiumcodereview.appspot.com/10875065/diff/11001/chrome/browser/ext...
chrome/browser/extensions/default_apps.cc:45: }
It's customary to add a "  // namespace" end marker here.

https://chromiumcodereview.appspot.com/10875065/diff/11001/chrome/browser/ext...
chrome/browser/extensions/default_apps.cc:49: bool
Provider::ShouldInstallInProfile() {
Move this down to be next to the other Provider methods.

https://chromiumcodereview.appspot.com/10875065/diff/11001/chrome/browser/ext...
File chrome/browser/extensions/external_provider_impl.h (right):

https://chromiumcodereview.appspot.com/10875065/diff/11001/chrome/browser/ext...
chrome/browser/extensions/external_provider_impl.h:55: virtual void
SetPrefs(base::DictionaryValue* prefs) OVERRIDE;
This doesn't override anything (SetPrefs is not defined in its superclass), so
it doesn't need that annotation.

[commit-bot: I haz the power, 2012-08-28 18:40:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grv@chromium.org/10875065/17001

[commit-bot: I haz the power, 2012-08-28 19:15:04]

Try job failure for 10875065-17001 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2012-08-28 20:01:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grv@chromium.org/10875065/13002

[Mihai Parparita -not on Chrome, 2012-08-28 20:23:50]

A simple rebase isn't enough. Looking at the tryserver failures (
http://build.chromium.org/p/tryserver.chromium/builders/mac_rel/builds/51128/...),
you'll need to update the test to call ShouldInstallInProfile in the right
class:

../../chrome/browser/extensions/extension_service_unittest.cc:3838:29:error:
no member named 'ShouldInstallInProfile' in namespace 'default_apps'
  EXPECT_TRUE(default_apps::ShouldInstallInProfile(profile.get()));
  ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This might also be a good time to move those tests to
default_apps_unittest.cc

Mihai

On Tue, Aug 28, 2012 at 1:01 PM, <commit-bot@chromium.org> wrote:

> CQ is trying da patch. Follow status at
>
https://chromium-status.**appspot.com/cq/grv@chromium.**org/10875065/13002<ht...
>
>
>
https://chromiumcodereview.**appspot.com/10875065/<https://chromiumcodereview...
>

[commit-bot: I haz the power, 2012-08-28 20:39:20]

Try job failure for 10875065-13002 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[Nico, 2012-08-29 00:49:30]

This just appeared in my inbox. If I'm supposed to look at foo/bar/*,
please say "thakis: foo/bar/*"

On Tue, Aug 28, 2012 at 5:42 PM, <grv@chromium.org> wrote:

>
https://chromiumcodereview.**appspot.com/10875065/<https://chromiumcodereview...
>

[Mihai Parparita -not on Chrome, 2012-08-29 00:51:18]

The CL description should also be updated to mention the test move.

https://chromiumcodereview.appspot.com/10875065/diff/16008/chrome/browser/ext...
File chrome/browser/extensions/default_apps.cc (right):

https://chromiumcodereview.appspot.com/10875065/diff/16008/chrome/browser/ext...
chrome/browser/extensions/default_apps.cc:22: const char gmail_id[] =
"pjkljhegncpnkpknbcohdijeoejaedia";
See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Consta...
for how to name constants. This should be called kGmailAppId.

https://chromiumcodereview.appspot.com/10875065/diff/16008/chrome/browser/ext...
File chrome/browser/extensions/default_apps.h (right):

https://chromiumcodereview.appspot.com/10875065/diff/16008/chrome/browser/ext...
chrome/browser/extensions/default_apps.h:51: static bool
ShouldInstallInProfile(Profile* profile);
Making these static just so you don't have to instantiate a Provider in test
code is a bad idea. There can be multiple Provider instances alive in a Chrome
process (one per profile), and they will clobber each other's isMigration flags.

You should be able to instantiate a Provider in the test, there's a
TestingProfile, VisitorInterface should be easy to mock, as is ExternalLoader.

https://chromiumcodereview.appspot.com/10875065/diff/16008/chrome/browser/ext...
chrome/browser/extensions/default_apps.h:59: static bool isMigration_;
This should be called is_migration_.

[Gaurav, 2012-08-29 07:45:46]

thakis: file chrome/chrome_tests.gypi

https://chromiumcodereview.appspot.com/10875065/diff/16008/chrome/browser/ext...
File chrome/browser/extensions/default_apps.h (right):

https://chromiumcodereview.appspot.com/10875065/diff/16008/chrome/browser/ext...
chrome/browser/extensions/default_apps.h:51: static bool
ShouldInstallInProfile(Profile* profile);
I just tried implementing it by mocking both Provider and Loader. However the
destructor of Provider contains a check
"CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI))" which is FATAL for
unittest. Any idea how should I deal with this

Thanks

On 2012/08/29 00:51:18, Mihai Parparita wrote:
> Making these static just so you don't have to instantiate a Provider in test
> code is a bad idea. There can be multiple Provider instances alive in a Chrome
> process (one per profile), and they will clobber each other's isMigration
flags.
> 
> You should be able to instantiate a Provider in the test, there's a
> TestingProfile, VisitorInterface should be easy to mock, as is ExternalLoader.

[Nico, 2012-08-29 14:11:09]

gypi change lgtm

[Mihai Parparita -not on Chrome, 2012-08-29 21:21:58]

BTW, when upload for re-review, it's customary to say what what has
changed, or at least PTAL (Please Take Another Look).

Mihai

On Wed, Aug 29, 2012 at 2:13 PM, <grv@chromium.org> wrote:

>
https://chromiumcodereview.**appspot.com/10875065/<https://chromiumcodereview...
>

[Mihai Parparita -not on Chrome, 2012-08-29 21:23:36]

LGTM

[commit-bot: I haz the power, 2012-08-29 21:24:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grv@chromium.org/10875065/19005

[Gaurav, 2012-08-29 21:26:02]

On 2012/08/29 21:24:59, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/grv%40chromium.org/10875065/19005

Thanks Mihai

[commit-bot: I haz the power, 2012-08-29 21:55:45]

Try job failure for 10875065-19005 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2012-08-29 22:14:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grv@chromium.org/10875065/14009

[commit-bot: I haz the power, 2012-08-29 22:46:25]

Try job failure for 10875065-14009 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[Mihai Parparita -not on Chrome, 2012-08-29 22:50:13]

The error before was:

../../chrome/browser/extensions/default_apps_unittest.cc:25:3:error:
[chromium-style] Classes that are ref-counted should not have public
destructors.
  virtual ~MockExternalLoader() {}
  ^
1 error generated.

The fix wasn't to remove the MockExtensionLoader destructor, but to make it
private.

Mihai

On Wed, Aug 29, 2012 at 3:46 PM, <commit-bot@chromium.org> wrote:

> Try job failure for 10875065-14009 (retry) on linux_clang for step
> "compile"
>
> (clobber build).
> It's a second try, previously, step "compile" failed.
> http://build.chromium.org/p/**tryserver.chromium/**
>
buildstatus?builder=linux_**clang&number=45264<http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clang&number=45264>
>
>
>
https://chromiumcodereview.**appspot.com/10875065/<https://chromiumcodereview...
>

[Mihai Parparita -not on Chrome, 2012-08-29 22:50:14]

The error before was:

../../chrome/browser/extensions/default_apps_unittest.cc:25:3:error:
[chromium-style] Classes that are ref-counted should not have public
destructors.
  virtual ~MockExternalLoader() {}
  ^
1 error generated.

The fix wasn't to remove the MockExtensionLoader destructor, but to make it
private.

Mihai

On Wed, Aug 29, 2012 at 3:46 PM, <commit-bot@chromium.org> wrote:

> Try job failure for 10875065-14009 (retry) on linux_clang for step
> "compile"
>
> (clobber build).
> It's a second try, previously, step "compile" failed.
> http://build.chromium.org/p/**tryserver.chromium/**
>
buildstatus?builder=linux_**clang&number=45264<http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clang&number=45264>
>
>
>
https://chromiumcodereview.**appspot.com/10875065/<https://chromiumcodereview...
>

[commit-bot: I haz the power, 2012-08-29 23:16:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grv@chromium.org/10875065/19010

[Mihai Parparita -not on Chrome, 2012-08-30 00:31:24]

https://chromiumcodereview.appspot.com/10875065/diff/19010/chrome/browser/ext...
File chrome/browser/extensions/default_apps_unittest.cc (right):

https://chromiumcodereview.appspot.com/10875065/diff/19010/chrome/browser/ext...
chrome/browser/extensions/default_apps_unittest.cc:24: void StartLoading()
{return;}
Nit: unnecessary return statement.

(can be fixed in a follow-up CL)

[commit-bot: I haz the power, 2012-08-30 03:52:00]

Change committed as 154064