Fix bug in disabling sync for default apps

Fix bug in disabling sync for default apps

We added a new creation_flag for default apps was_installed_by_default,
 but every time we add a new creation_flag, we need to update the installedLoader
 as it does not directly pick the creation_flags but re-creates them
 through other preferences.
To fix this, I added a new preference to save creation_flags in preference,
 and read that directly.

BUG=152582
TBR=nick@chromium.org

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

[Mihai Parparita -not on Chrome, 2012-09-27 18:02:06]

Can you make the CL description more descriptive (e.g. say that we weren't
actually setting the default app flag when installing the extension)?

Why didn't our tests catch this? Can we add tests for this?

[Gaurav, 2012-09-29 00:05:56]

We added a new creation_flag for default apps was_installed_by_default, but
every time we add a new creation_flag, we need to update the installedLoader as
it does not directly pick the creation_flags but re-creates them through other
preferences.

To fix this, I added a new preference to save creation_flags in preference, and
read that directly.

Also, added the check in ReloadExtenions unittest.


On 2012/09/27 18:02:06, Mihai Parparita wrote:
> Can you make the CL description more descriptive (e.g. say that we weren't
> actually setting the default app flag when installing the extension)?
> 
> Why didn't our tests catch this? Can we add tests for this?

[Mihai Parparita -not on Chrome, 2012-09-29 00:22:25]

I appreciate the attempt at fixing the underlying problem, by making the
preservation of creation flags more automatic.

However, the CL is now more complex (and has a few bugs).

Since the fix for the bug needs to get merged into M23, I'd rather that you
switch to the previous approach of a two-line fix in
InstalledLoader::GetCreationFlags. Then, once that's merged, you can fix the
problem in this better way (which we'll ship for M24).

https://codereview.chromium.org/10977048/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.cc (right):

https://codereview.chromium.org/10977048/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1448: bool from_webstore,
This parameter should be equivalent to extension->creation_flags() &
FROM_WEBSTORE. It would be good to verify that it is, and if so, remove it.

https://codereview.chromium.org/10977048/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1875: int creation_flags = 0;
There's an Extension::NO_FLAGS  value that you can use instead.

https://codereview.chromium.org/10977048/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1877:
dictionary->GetInteger(kPrefCreationFlags, &creation_flags);
This can still fail (the extension preference dictionary is present, but it
doesn't have a kPrefCreationFlags key).

You should use ReadExtensionPrefInteger, and if that fails, fall back to
synthesizing the value from the old prefs.

https://codereview.chromium.org/10977048/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1881: creation_flags |=
IsFromBookmark(extension_id);
This won't quite work. You're trying to say:
creation_flags |= Extension::FROM_BOOKMARK

but you're instead saying 
creation_flags |= true (or false)

The same is true for the all the other flags.

Something like:
if (IsFromBookmark(extension_id))
  creation_flags |= Extension::FROM_BOOKMARK;

Should work.

https://codereview.chromium.org/10977048/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service_unittest.cc (right):

https://codereview.chromium.org/10977048/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service_unittest.cc:3409: EXPECT_EQ(true,
extension->from_bookmark());
Nit: These can be EXPECT_TRUE/EXPECT_FALSE.

[Mihai Parparita -not on Chrome, 2012-09-29 00:23:06]

On Fri, Sep 28, 2012 at 5:05 PM, <grv@chromium.org> wrote:

> We added a new creation_flag for default apps was_installed_by_default, but
> every time we add a new creation_flag, we need to update the
> installedLoader as
> it does not directly pick the creation_flags but re-creates them through
> other
> preferences.
>
> To fix this, I added a new preference to save creation_flags in
> preference, and
> read that directly.
>

This is a very nice description, but it should actually go in the CL
description (you can edit it by using the "Edit Issue" link on the review
page).

Mihai

[Gaurav, 2012-09-29 00:30:28]

Thanks Mihai

Will fix the issues and update.

https://codereview.chromium.org/10977048/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.cc (right):

https://codereview.chromium.org/10977048/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1881: creation_flags |=
IsFromBookmark(extension_id);
My mad, will fix this.

On 2012/09/29 00:22:25, Mihai Parparita wrote:
> This won't quite work. You're trying to say:
> creation_flags |= Extension::FROM_BOOKMARK
> 
> but you're instead saying 
> creation_flags |= true (or false)
> 
> The same is true for the all the other flags.
> 
> Something like:
> if (IsFromBookmark(extension_id))
>   creation_flags |= Extension::FROM_BOOKMARK;
> 
> Should work.

[Gaurav, 2012-10-01 22:09:36]

https://codereview.chromium.org/10977048/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.cc (right):

https://codereview.chromium.org/10977048/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1448: bool from_webstore,
On 2012/09/29 00:22:25, Mihai Parparita wrote:
> This parameter should be equivalent to extension->creation_flags() &
> FROM_WEBSTORE. It would be good to verify that it is, and if so, remove it.

Done.

https://codereview.chromium.org/10977048/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1875: int creation_flags = 0;
On 2012/09/29 00:22:25, Mihai Parparita wrote:
> There's an Extension::NO_FLAGS  value that you can use instead.

Done.

https://codereview.chromium.org/10977048/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1877:
dictionary->GetInteger(kPrefCreationFlags, &creation_flags);
On 2012/09/29 00:22:25, Mihai Parparita wrote:
> This can still fail (the extension preference dictionary is present, but it
> doesn't have a kPrefCreationFlags key).
> 
> You should use ReadExtensionPrefInteger, and if that fails, fall back to
> synthesizing the value from the old prefs.

Done.

https://codereview.chromium.org/10977048/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1881: creation_flags |=
IsFromBookmark(extension_id);
On 2012/09/29 00:22:25, Mihai Parparita wrote:
> This won't quite work. You're trying to say:
> creation_flags |= Extension::FROM_BOOKMARK
> 
> but you're instead saying 
> creation_flags |= true (or false)
> 
> The same is true for the all the other flags.
> 
> Something like:
> if (IsFromBookmark(extension_id))
>   creation_flags |= Extension::FROM_BOOKMARK;
> 
> Should work.

Done.

https://codereview.chromium.org/10977048/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service_unittest.cc (right):

https://codereview.chromium.org/10977048/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service_unittest.cc:3409: EXPECT_EQ(true,
extension->from_bookmark());
On 2012/09/29 00:22:25, Mihai Parparita wrote:
> Nit: These can be EXPECT_TRUE/EXPECT_FALSE.

Done.

[Mihai Parparita -not on Chrome, 2012-10-02 17:00:54]

LGTM

https://codereview.chromium.org/10977048/diff/11002/chrome/browser/extensions...
File chrome/browser/extensions/extension_service_unittest.cc (right):

https://codereview.chromium.org/10977048/diff/11002/chrome/browser/extensions...
chrome/browser/extensions/extension_service_unittest.cc:3411:
EXPECT_FALSE(extension->was_installed_by_default());
Add a test that the was_installed_by_default flag is preserved (i.e. for the bug
that you're actually fixing).

[Gaurav, 2012-10-02 23:08:57]

Thanks Mihai

https://codereview.chromium.org/10977048/diff/11002/chrome/browser/extensions...
File chrome/browser/extensions/extension_service_unittest.cc (right):

https://codereview.chromium.org/10977048/diff/11002/chrome/browser/extensions...
chrome/browser/extensions/extension_service_unittest.cc:3411:
EXPECT_FALSE(extension->was_installed_by_default());
On 2012/10/02 17:00:54, Mihai Parparita wrote:
> Add a test that the was_installed_by_default flag is preserved (i.e. for the
bug
> that you're actually fixing).

Done.

[commit-bot: I haz the power, 2012-10-02 23:10:11]

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

[commit-bot: I haz the power, 2012-10-02 23:10:20]

Failed to apply patch for chrome/browser/extensions/installed_loader.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/extensions/installed_loader.cc
Hunk #1 FAILED at 303.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/extensions/installed_loader.cc.rej

[commit-bot: I haz the power, 2012-10-02 23:26:24]

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

[commit-bot: I haz the power, 2012-10-02 23:26:35]

Presubmit check for 10977048-5021 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/media_gallery
    chrome/browser/ui/panels

Presubmit checks took 2.1s to calculate.

[Gaurav, 2012-10-02 23:32:19]

@jennb : chrome/browser/ui/panels/base_panel_browser_test.cc

@estade: chrome/browser/media_gallery/

Thanks

[jennb, 2012-10-03 00:38:25]

LGTM for panels

[Evan Stade, 2012-10-04 08:40:03]

lgtm for media galleries (feel free to TBR such changes in the future)

[commit-bot: I haz the power, 2012-10-04 14:25:10]

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

[commit-bot: I haz the power, 2012-10-04 14:58:07]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[Gaurav, 2012-10-04 20:06:35]

@nick : need LGTM for chrome/browser/sync/test/integration/

[commit-bot: I haz the power, 2012-10-04 20:19:36]

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

[commit-bot: I haz the power, 2012-10-05 01:00:22]

Failed to apply the patch.
Sending        chrome/browser/extensions/app_process_apitest.cc
Sending        chrome/browser/extensions/crx_installer.cc
Sending        chrome/browser/extensions/extension_browsertest.cc
Sending        chrome/browser/extensions/extension_prefs.cc
Sending        chrome/browser/extensions/extension_prefs.h
Sending        chrome/browser/extensions/extension_prefs_unittest.cc
Sending        chrome/browser/extensions/extension_service.cc
Sending        chrome/browser/extensions/extension_service.h
Sending        chrome/browser/extensions/extension_service_unittest.cc
Sending        chrome/browser/extensions/extension_sorting_unittest.cc
Sending        chrome/browser/extensions/installed_loader.cc
Sending        chrome/browser/extensions/test_extension_prefs.cc
Sending        chrome/browser/extensions/unpacked_installer.cc
Sending       
chrome/browser/media_gallery/media_galleries_preferences_unittest.cc
Sending        chrome/browser/sync/test/integration/sync_extension_helper.cc
Sending        chrome/browser/ui/panels/base_panel_browser_test.cc
Sending        chrome/common/extensions/extension.h
Transmitting file data .................svn: Commit failed (details follow):
svn: Can't read from connection: Connection reset by peer

[commit-bot: I haz the power, 2012-10-05 16:57:02]

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

[commit-bot: I haz the power, 2012-10-05 16:57:08]

Failed to apply patch for chrome/browser/extensions/app_process_apitest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/extensions/app_process_apitest.cc
  Hunk #1 FAILED at 263.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/extensions/app_process_apitest.cc.rej

Patch:       chrome/browser/extensions/app_process_apitest.cc
Index: chrome/browser/extensions/app_process_apitest.cc
diff --git a/chrome/browser/extensions/app_process_apitest.cc
b/chrome/browser/extensions/app_process_apitest.cc
index
cc5b3ee568607d352120207d5602913f9fb15892..f1cdebf8c9babff6a0ffa2cf198e401930b06b7e
100644
--- a/chrome/browser/extensions/app_process_apitest.cc
+++ b/chrome/browser/extensions/app_process_apitest.cc
@@ -263,7 +263,7 @@ IN_PROC_BROWSER_TEST_F(AppApiTest,
BookmarkAppGetsNormalProcess) {
       Extension::LOAD,
       Extension::FROM_BOOKMARK,
       &error));
-  service->OnExtensionInstalled(extension, false,
+  service->OnExtensionInstalled(extension,
                                 syncer::StringOrdinal::CreateInitialOrdinal(),
                                 false /* no requirement errors */);
   ASSERT_TRUE(extension.get());