Disable sync for default apps

Disable sync for default apps

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

[Gaurav, 2012-08-23 23:17:34]

This CL is on top of default_apps cl.

Locally it shows the correct diff, but when I upload it shows the diff between
trunk and my branch

Anyways I can specify the branch with which to take the diff ?

I will rebase it once the other diff is committed, and publish again for review.

Thanks.

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

Based on http://src.chromium.org/svn/trunk/tools/depot_tools/README.git-cl,
you can specify a different branch to use as a command line parameter to
git cl upload.

Mihai

On Thu, Aug 23, 2012 at 4:17 PM, <grv@chromium.org> wrote:

> Reviewers: Mihai Parparita,
>
> Message:
> This CL is on top of default_apps cl.
>
> Locally it shows the correct diff, but when I upload it shows the diff
> between
> trunk and my branch
>
> Anyways I can specify the branch with which to take the diff ?
>
> I will rebase it once the other diff is committed, and publish again for
> review.
>
> Thanks.
>
> Description:
> Disable sync for default apps
>
>
> new default apps
>
>
> new default apps
>
>
> BUG=138614
>
>
> Please review this at
https://chromiumcodereview.**appspot.com/10855190/<https://chromiumcodereview...
>
> SVN Base:
http://git.chromium.org/git/**chromium.git@trunk<http://git.chromium.org/git/...
>
> Affected files:
>   M chrome/browser/extensions/**default_apps.h
>   M chrome/browser/extensions/**default_apps.cc
>   M chrome/browser/extensions/**extension_prefs.h
>   M chrome/browser/extensions/**extension_prefs.cc
>   M chrome/browser/extensions/**extension_prefs_unittest.cc
>   M chrome/browser/extensions/**extension_service.cc
>   M chrome/browser/extensions/**extension_service_unittest.cc
>   M chrome/browser/extensions/**external_provider_impl.cc
>   M chrome/common/extensions/**extension.h
>   M chrome/common/extensions/**extension.cc
>   M chrome/common/extensions/**extension_unittest.cc
>
>
>

[Mihai Parparita -not on Chrome, 2012-08-24 21:05:23]

https://chromiumcodereview.appspot.com/10855190/diff/7001/chrome/browser/exte...
File chrome/browser/extensions/extension_prefs.h (right):

https://chromiumcodereview.appspot.com/10855190/diff/7001/chrome/browser/exte...
chrome/browser/extensions/extension_prefs.h:432: // Returns true if the
extension was installed as default app.
"as a default app" is more correct.

https://chromiumcodereview.appspot.com/10855190/diff/7001/chrome/browser/exte...
chrome/browser/extensions/extension_prefs.h:433: bool IsFromDefault(const
std::string& extension_id) const;
"from default" doesn't quite sound right. WasInstalledByDefaultApps perhaps?
Though Aaron asked for a more generic name than "apps", so perhaps just
WasInstalledByDefault. Whatever name you choose should be used for the key name
and other variable names.

https://chromiumcodereview.appspot.com/10855190/diff/7001/chrome/common/exten...
File chrome/common/extensions/extension.cc (right):

https://chromiumcodereview.appspot.com/10855190/diff/7001/chrome/common/exten...
chrome/common/extensions/extension.cc:3779: // Sync  internal externsions except
default apps.
Extra space before internal, and there's a typo in "externsions".

Also, this comment just says what the code does, which makes it redundant. It's
better to explain why we don't sync default apps, Something like: "Default apps
are not synced because otherwise they will pollute profiles that don't already
have them. Specifically, if a user doesn't have default apps, creates a new
profile (which gets default apps) and then enables sync for it, then their
profile everywhere gets the default apps."

https://chromiumcodereview.appspot.com/10855190/diff/7001/chrome/common/exten...
chrome/common/extensions/extension.cc:3781: // sync the chrome web store.
Another redundant comment. Based on http://crrev.com/132065, it looks like we
sync the Web Store because we want its position on the new tab page to be
synced. Maybe say that instead?

https://chromiumcodereview.appspot.com/10855190/diff/7001/chrome/common/exten...
File chrome/common/extensions/extension_unittest.cc (right):

https://chromiumcodereview.appspot.com/10855190/diff/7001/chrome/common/exten...
chrome/common/extensions/extension_unittest.cc:1059: int
creation_flags=Extension::NO_FLAGS) {
Our style guide forbids default arguments:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments

[Gaurav, 2012-08-25 01:43:16]

Addressed comments

[Mihai Parparita -not on Chrome, 2012-08-27 18:38:00]

LGTM

https://chromiumcodereview.appspot.com/10855190/diff/8002/chrome/common/exten...
File chrome/common/extensions/extension.cc (right):

https://chromiumcodereview.appspot.com/10855190/diff/8002/chrome/common/exten...
chrome/common/extensions/extension.cc:3782: // for it, then their profile
everywhere gets the default_apps.
Remove underscore in default_apps.

https://chromiumcodereview.appspot.com/10855190/diff/8002/chrome/common/exten...
chrome/common/extensions/extension.cc:3783: bool is_syncable = (location() == 
Extension::INTERNAL &&
Nit: extra space after ==.

https://chromiumcodereview.appspot.com/10855190/diff/8002/chrome/common/exten...
chrome/common/extensions/extension.cc:3785: // sync the chrome web store to
maintain its position on the new tab page.
Capitalize "sync".

https://chromiumcodereview.appspot.com/10855190/diff/8002/chrome/common/exten...
File chrome/common/extensions/extension.h (right):

https://chromiumcodereview.appspot.com/10855190/diff/8002/chrome/common/exten...
chrome/common/extensions/extension.h:251: // |FROM_DEFAULT| indicates the
extension was created as default.
Update comment to refer to new constant name.

Also, "created as default" doesn't quite sound right. "installed by default when
the profile was created" perhaps?

[commit-bot: I haz the power, 2012-08-27 19:46:37]

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

[commit-bot: I haz the power, 2012-08-27 22:05:48]

Change committed as 153554