Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2054773002:
Replace the WAS_INSTALLED_BY_CUSTODIAN creation flag with a pref (Closed)
|
Created:
4 years, 6 months ago by mamir Modified:
4 years, 6 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReplace the WAS_INSTALLED_BY_CUSTODIAN creation flag with a pref
Before this CL:
If a custodian-installed extension was already installed in the
browser before the child signs in, the "WasInstalledByCustodian" flag is
stored in Sync was ignored and the default value (false) is used giving
the child the ability to uninstall the extension for example.
After this CL:
The creation flag has been replaced with a pref which is updated
properly upon receiving Sync data.
BUG=619526
Committed: https://crrev.com/192d78833ff9297c754f59f20cb4e590d693a7c2
Cr-Commit-Position: refs/heads/master@{#401318}
Patch Set 1 #Patch Set 2 : Fix the bug of delegated installs for pre-existing extensions #
Total comments: 21
Patch Set 3 : Response to code review by treib@ #
Total comments: 8
Patch Set 4 : Response to code review by treib@ #
Total comments: 16
Patch Set 5 : Response to code review #Patch Set 6 : Response to code review by Devlin #
Total comments: 5
Patch Set 7 : Response to code review by Devlin #Patch Set 8 : Remove extension check from util::SetWasInstalledByCustodian #Patch Set 9 : Removing the creation flag completely #Patch Set 10 : Commenting out WAS_INSTALLED_BY_CUSTODIAN flag #
Total comments: 12
Patch Set 11 : Response to code review by Marc #
Total comments: 10
Patch Set 12 : Response to code review by Marc #
Total comments: 12
Patch Set 13 : Response to code review by Marc #Patch Set 14 : reload if installed_by_custodian becomes false. #
Total comments: 12
Patch Set 15 : Response to code review by Marc #
Total comments: 11
Patch Set 16 : Response to code review by Devlin. #Patch Set 17 : rebase #Patch Set 18 : Fixing the build #Dependent Patchsets: Messages
Total messages: 68 (21 generated)
Description was changed from ========== Fixing the bug of custodian-installed extensions that are preinstalled. BUG= ========== to ========== Fixing the bug of custodian-installed extensions that are preinstalled. BUG= ==========
mamir@chromium.org changed reviewers: + rdevlin.cronin@chromium.org, treib@chromium.org
Please add a bug, and update CL description and title. https://codereview.chromium.org/2054773002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_service_sync_unittest.cc (right): https://codereview.chromium.org/2054773002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service_sync_unittest.cc:1699: sync_pb::EntitySpecifics specifics; Add a comment about what this does please. https://codereview.chromium.org/2054773002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service_sync_unittest.cc:1712: // Now make the profile supervised. What about the other way around - first it becomes supervised, then the sync data arrives? That should be the more common case, so can we add another test for that? https://codereview.chromium.org/2054773002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service_sync_unittest.cc:1717: EXPECT_TRUE(registry()->enabled_extensions().Contains(id)); Also check that is has the installed_by_custodian flag? https://codereview.chromium.org/2054773002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_sync_service.cc (right): https://codereview.chromium.org/2054773002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.cc:203: Please avoid unnecessary changes like this. https://codereview.chromium.org/2054773002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.cc:320: #if defined(ENABLE_SUPERVISED_USERS) I don't think this needs an #ifdef. All the other code that deals with that flag isn't #ifdef'd either. https://codereview.chromium.org/2054773002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.cc:516: extension_sync_data.installed_by_custodian())) { Is it still necessary to pass the installed_by_custodian flag here? If you set the pref above, it should get applied during installation anyway, right? https://codereview.chromium.org/2054773002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/2054773002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_util.cc:235: CHECK(service); ? https://codereview.chromium.org/2054773002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/unpacked_installer.cc (right): https://codereview.chromium.org/2054773002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/unpacked_installer.cc:263: installed_by_custodian = prefs->IsInstalledByCustodian(id); If this is unset, it should default to false anyway, right? Then we could get rid of the Has... method. https://codereview.chromium.org/2054773002/diff/20001/extensions/browser/exte... File extensions/browser/extension_prefs.h (right): https://codereview.chromium.org/2054773002/diff/20001/extensions/browser/exte... extensions/browser/extension_prefs.h:415: bool IsInstalledByCustodian(const std::string& extension_id) const; WasInstalledByCustodian please, to stay consistent with the install flag. I'm not very happy with being both a pref and an install flag, but I guess there's no way around it.
Description was changed from ========== Fixing the bug of custodian-installed extensions that are preinstalled. BUG= ========== to ========== If a custodian-installed extension was already installed in the browser before the child signs in, the "WasInstalledByCustodian" flag is reset to false ignoring the value stored in the Sync server. BUG=619526 ==========
Description was changed from ========== If a custodian-installed extension was already installed in the browser before the child signs in, the "WasInstalledByCustodian" flag is reset to false ignoring the value stored in the Sync server. BUG=619526 ========== to ========== Fixing the bug of delegated install of extensions that are pre-installed to Chrome. If a custodian-installed extension was already installed in the browser before the child signs in, the "WasInstalledByCustodian" flag is reset to false ignoring the value stored in the Sync server. BUG=619526 ==========
https://codereview.chromium.org/2054773002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_service_sync_unittest.cc (right): https://codereview.chromium.org/2054773002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service_sync_unittest.cc:1699: sync_pb::EntitySpecifics specifics; On 2016/06/13 09:37:13, Marc Treib wrote: > Add a comment about what this does please. Done. https://codereview.chromium.org/2054773002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service_sync_unittest.cc:1712: // Now make the profile supervised. On 2016/06/13 09:37:14, Marc Treib wrote: > What about the other way around - first it becomes supervised, then the sync > data arrives? That should be the more common case, so can we add another test > for that? Done. https://codereview.chromium.org/2054773002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service_sync_unittest.cc:1717: EXPECT_TRUE(registry()->enabled_extensions().Contains(id)); On 2016/06/13 09:37:14, Marc Treib wrote: > Also check that is has the installed_by_custodian flag? Done. https://codereview.chromium.org/2054773002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_sync_service.cc (right): https://codereview.chromium.org/2054773002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.cc:203: On 2016/06/13 09:37:14, Marc Treib wrote: > Please avoid unnecessary changes like this. Sorry. Overlooked it after removing a LOG statement. Done! https://codereview.chromium.org/2054773002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.cc:320: #if defined(ENABLE_SUPERVISED_USERS) On 2016/06/13 09:37:14, Marc Treib wrote: > I don't think this needs an #ifdef. All the other code that deals with that flag > isn't #ifdef'd either. Done. https://codereview.chromium.org/2054773002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.cc:516: extension_sync_data.installed_by_custodian())) { On 2016/06/13 09:37:14, Marc Treib wrote: > Is it still necessary to pass the installed_by_custodian flag here? If you set > the pref above, it should get applied during installation anyway, right? It not necessary anymore, you are right. Done! https://codereview.chromium.org/2054773002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/2054773002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_util.cc:235: CHECK(service); On 2016/06/13 09:37:14, Marc Treib wrote: > ? Done. https://codereview.chromium.org/2054773002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/unpacked_installer.cc (right): https://codereview.chromium.org/2054773002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/unpacked_installer.cc:263: installed_by_custodian = prefs->IsInstalledByCustodian(id); On 2016/06/13 09:37:14, Marc Treib wrote: > If this is unset, it should default to false anyway, right? Then we could get > rid of the Has... method. Done. https://codereview.chromium.org/2054773002/diff/20001/extensions/browser/exte... File extensions/browser/extension_prefs.h (right): https://codereview.chromium.org/2054773002/diff/20001/extensions/browser/exte... extensions/browser/extension_prefs.h:415: bool IsInstalledByCustodian(const std::string& extension_id) const; On 2016/06/13 09:37:14, Marc Treib wrote: > WasInstalledByCustodian please, to stay consistent with the install flag. > > I'm not very happy with being both a pref and an install flag, but I guess > there's no way around it. Done. And I share the same feeling about being both a pref and a install flag.
https://codereview.chromium.org/2054773002/diff/20001/extensions/browser/exte... File extensions/browser/extension_prefs.h (right): https://codereview.chromium.org/2054773002/diff/20001/extensions/browser/exte... extensions/browser/extension_prefs.h:415: bool IsInstalledByCustodian(const std::string& extension_id) const; On 2016/06/13 11:30:17, mamir wrote: > On 2016/06/13 09:37:14, Marc Treib wrote: > > WasInstalledByCustodian please, to stay consistent with the install flag. > > > > I'm not very happy with being both a pref and an install flag, but I guess > > there's no way around it. > > Done. > And I share the same feeling about being both a pref and a install flag. Hm. Do we still need the creation flag? (There's actually another thing called "install flags" which is different...) It seems that we could get rid of it and have all callers check the pref instead, but I might be missing something. https://codereview.chromium.org/2054773002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_service_sync_unittest.cc (right): https://codereview.chromium.org/2054773002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_service_sync_unittest.cc:1722: EXPECT_TRUE(registry()->enabled_extensions().Contains(id)); This should probably be ASSERT_TRUE - if it's false, the next line would crash. https://codereview.chromium.org/2054773002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_service_sync_unittest.cc:1726: ->was_installed_by_custodian()); Woah, did git cl format do this? Ugh... maybe pull up to "GetByID" out of the EXPECT, so things will fit on a line? (Then you could also ASSERT_TRUE(extension) to make sure it's not null, and leave the EXPECT above be.) https://codereview.chromium.org/2054773002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/2054773002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_util.cc:235: service->ReloadExtension(extension_id); Maybe add a comment on why the reload is necessary? https://codereview.chromium.org/2054773002/diff/40001/extensions/browser/exte... File extensions/browser/extension_prefs.h (right): https://codereview.chromium.org/2054773002/diff/40001/extensions/browser/exte... extensions/browser/extension_prefs.h:413: // a supervised user. It is relevant for supervised users and used to block nitty nit: only one space after "."
Ah, also please still update the CL description/title. "Fixing the bug of" isn't helpful to anyone who doesn't already know what this is about. Also please make clear whether you are describing the "before" or "after" state.
Description was changed from ========== Fixing the bug of delegated install of extensions that are pre-installed to Chrome. If a custodian-installed extension was already installed in the browser before the child signs in, the "WasInstalledByCustodian" flag is reset to false ignoring the value stored in the Sync server. BUG=619526 ========== to ========== Propagating the "was_installed_by_custodian_ flag" from Sync data to the pre-installed extension. If a custodian-installed extension was already installed in the browser before the child signs in, the "WasInstalledByCustodian" flag is reset to false ignoring the value stored in the Sync server. The CL makes sure that this information is properly propagated from sync data to creation flags of the already installed extensions. BUG=619526 ==========
https://codereview.chromium.org/2054773002/diff/20001/extensions/browser/exte... File extensions/browser/extension_prefs.h (right): https://codereview.chromium.org/2054773002/diff/20001/extensions/browser/exte... extensions/browser/extension_prefs.h:415: bool IsInstalledByCustodian(const std::string& extension_id) const; On 2016/06/13 12:17:09, Marc Treib wrote: > On 2016/06/13 11:30:17, mamir wrote: > > On 2016/06/13 09:37:14, Marc Treib wrote: > > > WasInstalledByCustodian please, to stay consistent with the install flag. > > > > > > I'm not very happy with being both a pref and an install flag, but I guess > > > there's no way around it. > > > > Done. > > And I share the same feeling about being both a pref and a install flag. > > Hm. Do we still need the creation flag? (There's actually another thing called > "install flags" which is different...) > It seems that we could get rid of it and have all callers check the pref > instead, but I might be missing something. Well, this was my 3rd proposal to tackle this issue (shared earlier in a doc). I checked where the (installed_by_custodian) creation flag is used in the code, and it looks like we can actually get rid of it. But of course some corner case might be overlooked when switching from creation_flags to a pref. I will try that in another CL. https://codereview.chromium.org/2054773002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_service_sync_unittest.cc (right): https://codereview.chromium.org/2054773002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_service_sync_unittest.cc:1722: EXPECT_TRUE(registry()->enabled_extensions().Contains(id)); On 2016/06/13 12:17:09, Marc Treib wrote: > This should probably be ASSERT_TRUE - if it's false, the next line would crash. Done. https://codereview.chromium.org/2054773002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_service_sync_unittest.cc:1726: ->was_installed_by_custodian()); On 2016/06/13 12:17:09, Marc Treib wrote: > Woah, did git cl format do this? Ugh... maybe pull up to "GetByID" out of the > EXPECT, so things will fit on a line? (Then you could also > ASSERT_TRUE(extension) to make sure it's not null, and leave the EXPECT above > be.) Done. https://codereview.chromium.org/2054773002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/2054773002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_util.cc:235: service->ReloadExtension(extension_id); On 2016/06/13 12:17:09, Marc Treib wrote: > Maybe add a comment on why the reload is necessary? Done. https://codereview.chromium.org/2054773002/diff/40001/extensions/browser/exte... File extensions/browser/extension_prefs.h (right): https://codereview.chromium.org/2054773002/diff/40001/extensions/browser/exte... extensions/browser/extension_prefs.h:413: // a supervised user. It is relevant for supervised users and used to block On 2016/06/13 12:17:09, Marc Treib wrote: > nitty nit: only one space after "." Done.
Thanks! LGTM with one more description nit: "the "WasInstalledByCustodian" flag is reset to false ignoring the value stored in the Sync server" - this sounds like you are describing what your CL does. Please reformulate to make clear what's the "before" and "after" state, maybe something like "before this CL, the .. flag was ...". (Also "reset" isn't really accurate, since it was actually never set.) https://codereview.chromium.org/2054773002/diff/20001/extensions/browser/exte... File extensions/browser/extension_prefs.h (right): https://codereview.chromium.org/2054773002/diff/20001/extensions/browser/exte... extensions/browser/extension_prefs.h:415: bool IsInstalledByCustodian(const std::string& extension_id) const; On 2016/06/13 14:15:57, mamir wrote: > On 2016/06/13 12:17:09, Marc Treib wrote: > > On 2016/06/13 11:30:17, mamir wrote: > > > On 2016/06/13 09:37:14, Marc Treib wrote: > > > > WasInstalledByCustodian please, to stay consistent with the install flag. > > > > > > > > I'm not very happy with being both a pref and an install flag, but I guess > > > > there's no way around it. > > > > > > Done. > > > And I share the same feeling about being both a pref and a install flag. > > > > Hm. Do we still need the creation flag? (There's actually another thing called > > "install flags" which is different...) > > It seems that we could get rid of it and have all callers check the pref > > instead, but I might be missing something. > Well, this was my 3rd proposal to tackle this issue (shared earlier in a doc). > I checked where the (installed_by_custodian) creation flag is used in the code, > and it looks like we can actually get rid of it. > But of course some corner case might be overlooked when switching from > creation_flags to a pref. > I will try that in another CL. > Alright. If Devlin is okay with that, then I am too. :) https://codereview.chromium.org/2054773002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/2054773002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_util.cc:236: // Extension immultable object, a reload is required. immutable
https://codereview.chromium.org/2054773002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_service_sync_unittest.cc (right): https://codereview.chromium.org/2054773002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_service_sync_unittest.cc:1704: // It should not be enabled now (it is not loaded at all actually). nit: you have an extra space between "now" and "(" https://codereview.chromium.org/2054773002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_service_sync_unittest.cc:1722: EXPECT_TRUE(registry()->enabled_extensions().Contains(id)); This check is redundant with the check below. https://codereview.chromium.org/2054773002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_sync_service.cc (right): https://codereview.chromium.org/2054773002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.cc:321: profile_, true); nit: indentation https://codereview.chromium.org/2054773002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/2054773002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_util.cc:228: if (extension && mention how extension can be null. https://codereview.chromium.org/2054773002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/pending_extension_manager.cc (right): https://codereview.chromium.org/2054773002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/pending_extension_manager.cc:125: creation_flags, I haven't been real thorough, but at first glance it looks like we can remove creation_flags entirely from this method and PendingExtensionInfo. https://codereview.chromium.org/2054773002/diff/60001/extensions/browser/exte... File extensions/browser/extension_prefs.h (right): https://codereview.chromium.org/2054773002/diff/60001/extensions/browser/exte... extensions/browser/extension_prefs.h:416: void SetWasInstalledByCustodian(const std::string& extension_id, We should remove these and just use the generic UpdateExtensionPref() method from extension_util.
also, please wrap CL title and description to 72 chars to be git-friendly. :)
Description was changed from ========== Propagating the "was_installed_by_custodian_ flag" from Sync data to the pre-installed extension. If a custodian-installed extension was already installed in the browser before the child signs in, the "WasInstalledByCustodian" flag is reset to false ignoring the value stored in the Sync server. The CL makes sure that this information is properly propagated from sync data to creation flags of the already installed extensions. BUG=619526 ========== to ========== Propagate installed_by_custodian flag from sync to preinstalled extension Before this CL: If a custodian-installed extension was already installed in the browser before the child signs in, the "WasInstalledByCustodian" flag is stored in Sync in ignored and the default value (false) is used giving the child the ability to uninstall it. After this CL: During Syncing, the flag is read from the sync data and stored as pref and the extensions is reloaded. When reloading, the pref is checked and the proper creation flags are used accordingly. BUG=619526 ==========
Description was changed from ========== Propagate installed_by_custodian flag from sync to preinstalled extension Before this CL: If a custodian-installed extension was already installed in the browser before the child signs in, the "WasInstalledByCustodian" flag is stored in Sync in ignored and the default value (false) is used giving the child the ability to uninstall it. After this CL: During Syncing, the flag is read from the sync data and stored as pref and the extensions is reloaded. When reloading, the pref is checked and the proper creation flags are used accordingly. BUG=619526 ========== to ========== Propagate installed_by_custodian flag from sync to preinstalled extension Before this CL: If a custodian-installed extension was already installed in the browser before the child signs in, the "WasInstalledByCustodian" flag is stored in Sync was ignored and the default value (false) is used giving the child the ability to uninstall the extension for example. After this CL: During Syncing, the flag is read from the sync data and stored as a pref and the extensions is reloaded. When reloading, the pref is read and the proper creation flags are assigned accordingly. BUG=619526 ==========
https://codereview.chromium.org/2054773002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_service_sync_unittest.cc (right): https://codereview.chromium.org/2054773002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_service_sync_unittest.cc:1704: // It should not be enabled now (it is not loaded at all actually). On 2016/06/13 14:36:58, Devlin wrote: > nit: you have an extra space between "now" and "(" Done. https://codereview.chromium.org/2054773002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_service_sync_unittest.cc:1722: EXPECT_TRUE(registry()->enabled_extensions().Contains(id)); On 2016/06/13 14:36:58, Devlin wrote: > This check is redundant with the check below. Done. https://codereview.chromium.org/2054773002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_sync_service.cc (right): https://codereview.chromium.org/2054773002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.cc:321: profile_, true); On 2016/06/13 14:36:58, Devlin wrote: > nit: indentation Done. https://codereview.chromium.org/2054773002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/2054773002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_util.cc:228: if (extension && On 2016/06/13 14:36:58, Devlin wrote: > mention how extension can be null. Done. https://codereview.chromium.org/2054773002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_util.cc:236: // Extension immultable object, a reload is required. On 2016/06/13 14:36:50, Marc Treib wrote: > immutable Done. https://codereview.chromium.org/2054773002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/pending_extension_manager.cc (right): https://codereview.chromium.org/2054773002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/pending_extension_manager.cc:125: creation_flags, On 2016/06/13 14:36:58, Devlin wrote: > I haven't been real thorough, but at first glance it looks like we can remove > creation_flags entirely from this method and PendingExtensionInfo. That's correct. I removed it. Done. https://codereview.chromium.org/2054773002/diff/60001/extensions/browser/exte... File extensions/browser/extension_prefs.h (right): https://codereview.chromium.org/2054773002/diff/60001/extensions/browser/exte... extensions/browser/extension_prefs.h:416: void SetWasInstalledByCustodian(const std::string& extension_id, On 2016/06/13 14:36:58, Devlin wrote: > We should remove these and just use the generic UpdateExtensionPref() method > from extension_util. But other prefs such as IsIcognitoEnabled, AllowFileAccess ...etc. has their own Setters. Why this should be done differently?
https://codereview.chromium.org/2054773002/diff/60001/extensions/browser/exte... File extensions/browser/extension_prefs.h (right): https://codereview.chromium.org/2054773002/diff/60001/extensions/browser/exte... extensions/browser/extension_prefs.h:416: void SetWasInstalledByCustodian(const std::string& extension_id, On 2016/06/13 15:25:31, mamir wrote: > On 2016/06/13 14:36:58, Devlin wrote: > > We should remove these and just use the generic UpdateExtensionPref() method > > from extension_util. > > But other prefs such as IsIcognitoEnabled, AllowFileAccess ...etc. has their own > Setters. > Why this should be done differently? We should do everything through Set/Get extension pref, but sadly we don't (we haven't gone through and converted everything, etc). If you look at the number of callers of UpdateExtensionPref, you can see that it's more common (I'm pretty sure). And it makes it a lot clearer, especially when you have to do some surrounding work (as we do in extension_util), because then we don't need the annoying comments "You almost certainly want to use this other method over here with the exact same name" (as we say on lines 398, 406, etc).
https://codereview.chromium.org/2054773002/diff/60001/extensions/browser/exte... File extensions/browser/extension_prefs.h (right): https://codereview.chromium.org/2054773002/diff/60001/extensions/browser/exte... extensions/browser/extension_prefs.h:416: void SetWasInstalledByCustodian(const std::string& extension_id, On 2016/06/13 15:41:43, Devlin wrote: > On 2016/06/13 15:25:31, mamir wrote: > > On 2016/06/13 14:36:58, Devlin wrote: > > > We should remove these and just use the generic UpdateExtensionPref() method > > > from extension_util. > > > > But other prefs such as IsIcognitoEnabled, AllowFileAccess ...etc. has their > own > > Setters. > > Why this should be done differently? > > We should do everything through Set/Get extension pref, but sadly we don't (we > haven't gone through and converted everything, etc). If you look at the number > of callers of UpdateExtensionPref, you can see that it's more common (I'm pretty > sure). And it makes it a lot clearer, especially when you have to do some > surrounding work (as we do in extension_util), because then we don't need the > annoying comments "You almost certainly want to use this other method over here > with the exact same name" (as we say on lines 398, 406, etc). Done.
lgtm https://codereview.chromium.org/2054773002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/2054773002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_util.cc:239: maybe also: if (!extension) return; https://codereview.chromium.org/2054773002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/installed_loader.cc (right): https://codereview.chromium.org/2054773002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/installed_loader.cc:614: info->extension_id, extension_service_->profile()); is this git cl formatted?
https://codereview.chromium.org/2054773002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/2054773002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_util.cc:239: On 2016/06/15 00:30:36, Devlin wrote: > maybe also: > if (!extension) > return; Done. https://codereview.chromium.org/2054773002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/installed_loader.cc (right): https://codereview.chromium.org/2054773002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/installed_loader.cc:614: info->extension_id, extension_service_->profile()); On 2016/06/15 00:30:36, Devlin wrote: > is this git cl formatted? Yes.
https://codereview.chromium.org/2054773002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/2054773002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_util.cc:239: On 2016/06/15 09:54:41, mamir wrote: > On 2016/06/15 00:30:36, Devlin wrote: > > maybe also: > > if (!extension) > > return; > > Done. Not really. If we return here, the extension won't be reloaded. I will roll this back.
https://codereview.chromium.org/2054773002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/2054773002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/extension_util.cc:235: // If it is already enabled, nothing to be done. This is assuming that |installed_by_custodian| is true, yes? If it can never be set to false, then remove the parameter. https://codereview.chromium.org/2054773002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/extension_util.cc:239: // If the extension is not installed, it may need to be reloaded. Why? Is there any situation where registry->GetInstalledExtension would return null, but reloading will do anything? https://codereview.chromium.org/2054773002/diff/180001/chrome/browser/supervi... File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/2054773002/diff/180001/chrome/browser/supervi... chrome/browser/supervised_user/supervised_user_service.cc:78: #include "extensions/browser/extension_prefs.h" wrong #ifdef https://codereview.chromium.org/2054773002/diff/180001/chrome/browser/supervi... chrome/browser/supervised_user/supervised_user_service.cc:84: using extensions::ExtensionPrefs; add #ifdef https://codereview.chromium.org/2054773002/diff/180001/chrome/browser/supervi... File chrome/browser/supervised_user/supervised_user_service_unittest.cc (right): https://codereview.chromium.org/2054773002/diff/180001/chrome/browser/supervi... chrome/browser/supervised_user/supervised_user_service_unittest.cc:417: extensions::util::SetWasInstalledByCustodian(extension->id(), braces
https://codereview.chromium.org/2054773002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/2054773002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/extension_util.cc:235: // If it is already enabled, nothing to be done. On 2016/06/16 12:35:39, Marc Treib wrote: > This is assuming that |installed_by_custodian| is true, yes? If it can never be > set to false, then remove the parameter. Yes. It can never be set to false, but I left the parameter for consistency with other methods. I removed it. https://codereview.chromium.org/2054773002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/extension_util.cc:239: // If the extension is not installed, it may need to be reloaded. On 2016/06/16 12:35:39, Marc Treib wrote: > Why? > Is there any situation where registry->GetInstalledExtension would return null, > but reloading will do anything? Pre-installed extensions that get unloaded after Chrome knows it's a supervised user. https://codereview.chromium.org/2054773002/diff/180001/chrome/browser/supervi... File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/2054773002/diff/180001/chrome/browser/supervi... chrome/browser/supervised_user/supervised_user_service.cc:78: #include "extensions/browser/extension_prefs.h" On 2016/06/16 12:35:39, Marc Treib wrote: > wrong #ifdef Sorry. Done! https://codereview.chromium.org/2054773002/diff/180001/chrome/browser/supervi... chrome/browser/supervised_user/supervised_user_service.cc:84: using extensions::ExtensionPrefs; On 2016/06/16 12:35:39, Marc Treib wrote: > add #ifdef Done. https://codereview.chromium.org/2054773002/diff/180001/chrome/browser/supervi... File chrome/browser/supervised_user/supervised_user_service_unittest.cc (right): https://codereview.chromium.org/2054773002/diff/180001/chrome/browser/supervi... chrome/browser/supervised_user/supervised_user_service_unittest.cc:417: extensions::util::SetWasInstalledByCustodian(extension->id(), On 2016/06/16 12:35:39, Marc Treib wrote: > braces Done.
Mostly LG now, thanks! A few more small comments, and one actual question. https://codereview.chromium.org/2054773002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/2054773002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/extension_util.cc:239: // If the extension is not installed, it may need to be reloaded. On 2016/06/17 08:33:18, mamir wrote: > On 2016/06/16 12:35:39, Marc Treib wrote: > > Why? > > Is there any situation where registry->GetInstalledExtension would return > null, > > but reloading will do anything? > > Pre-installed extensions that get unloaded after Chrome knows it's a supervised > user. And in that case, GetInstalledExtension won't return it at all? Okay, maybe mention that in the comment? (I think GetInstalledExtension is really misnamed, making this super confusing to read...) https://codereview.chromium.org/2054773002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/extension_service_sync_unittest.cc (right): https://codereview.chromium.org/2054773002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/extension_service_sync_unittest.cc:1734: ASSERT_TRUE(installed_extension); Since you don't need |installed_extension| afterwards, this can be an EXPECT_TRUE, and you can just inline the registry lookup directly here. https://codereview.chromium.org/2054773002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/extension_service_sync_unittest.cc:1772: ASSERT_TRUE(installed_extension); Same here. https://codereview.chromium.org/2054773002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/extension_sync_service.cc (right): https://codereview.chromium.org/2054773002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/extension_sync_service.cc:37: #include "chrome/browser/extensions/extension_util.h" This needs to go out of the #ifdef https://codereview.chromium.org/2054773002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/extension_sync_service.cc:322: if (extension_sync_data.installed_by_custodian()) { Hm. Now that I see this, I kinda think we should trust the data coming from Sync, and clear the pref if necessary. Otherwise, it's impossible to ever remove the flag from Sync, because the Chrome client will always add it back. Consider: - Extension is custodian-installed. - Custodian uninstalls it. - Another machine gets the uninstall. - On that other machine, SU initiates an install of the same extension. - Back on this machine, we get a Sync update saying that it's now not installed by custodian anymore, but Chrome ignores that, and adds the flag back to Sync. So, would it break anything to clear the pref here if Sync says so? https://codereview.chromium.org/2054773002/diff/200001/extensions/common/exte... File extensions/common/extension.h (right): https://codereview.chromium.org/2054773002/diff/200001/extensions/common/exte... extensions/common/extension.h:156: // custodian of a supervised user. I'd remove this comment now - if someone cares, they should look at the pref instead. (This one is bound to get out of date)
https://codereview.chromium.org/2054773002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/2054773002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/extension_util.cc:239: // If the extension is not installed, it may need to be reloaded. On 2016/06/17 08:56:54, Marc Treib wrote: > On 2016/06/17 08:33:18, mamir wrote: > > On 2016/06/16 12:35:39, Marc Treib wrote: > > > Why? > > > Is there any situation where registry->GetInstalledExtension would return > > null, > > > but reloading will do anything? > > > > Pre-installed extensions that get unloaded after Chrome knows it's a > supervised > > user. > > And in that case, GetInstalledExtension won't return it at all? Okay, maybe > mention that in the comment? > (I think GetInstalledExtension is really misnamed, making this super confusing > to read...) Done. https://codereview.chromium.org/2054773002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/extension_service_sync_unittest.cc (right): https://codereview.chromium.org/2054773002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/extension_service_sync_unittest.cc:1734: ASSERT_TRUE(installed_extension); On 2016/06/17 08:56:54, Marc Treib wrote: > Since you don't need |installed_extension| afterwards, this can be an > EXPECT_TRUE, and you can just inline the registry lookup directly here. Done. https://codereview.chromium.org/2054773002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/extension_service_sync_unittest.cc:1772: ASSERT_TRUE(installed_extension); On 2016/06/17 08:56:54, Marc Treib wrote: > Same here. Done. https://codereview.chromium.org/2054773002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/extension_sync_service.cc (right): https://codereview.chromium.org/2054773002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/extension_sync_service.cc:37: #include "chrome/browser/extensions/extension_util.h" On 2016/06/17 08:56:54, Marc Treib wrote: > This needs to go out of the #ifdef Done. https://codereview.chromium.org/2054773002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/extension_sync_service.cc:322: if (extension_sync_data.installed_by_custodian()) { On 2016/06/17 08:56:54, Marc Treib wrote: > Hm. Now that I see this, I kinda think we should trust the data coming from > Sync, and clear the pref if necessary. Otherwise, it's impossible to ever remove > the flag from Sync, because the Chrome client will always add it back. > > Consider: > - Extension is custodian-installed. > - Custodian uninstalls it. > - Another machine gets the uninstall. > - On that other machine, SU initiates an install of the same extension. > - Back on this machine, we get a Sync update saying that it's now not installed > by custodian anymore, but Chrome ignores that, and adds the flag back to Sync. > > So, would it break anything to clear the pref here if Sync says so? I agree. No thing should break because this flag is always set to true on the server not the client. Done! https://codereview.chromium.org/2054773002/diff/200001/extensions/common/exte... File extensions/common/extension.h (right): https://codereview.chromium.org/2054773002/diff/200001/extensions/common/exte... extensions/common/extension.h:156: // custodian of a supervised user. On 2016/06/17 08:56:54, Marc Treib wrote: > I'd remove this comment now - if someone cares, they should look at the pref > instead. (This one is bound to get out of date) Done.
Description was changed from ========== Propagate installed_by_custodian flag from sync to preinstalled extension Before this CL: If a custodian-installed extension was already installed in the browser before the child signs in, the "WasInstalledByCustodian" flag is stored in Sync was ignored and the default value (false) is used giving the child the ability to uninstall the extension for example. After this CL: During Syncing, the flag is read from the sync data and stored as a pref and the extensions is reloaded. When reloading, the pref is read and the proper creation flags are assigned accordingly. BUG=619526 ========== to ========== Replace the WAS_INSTALLED_BY_CUSTODIAN creation flag with a pref Before this CL: If a custodian-installed extension was already installed in the browser before the child signs in, the "WasInstalledByCustodian" flag is stored in Sync was ignored and the default value (false) is used giving the child the ability to uninstall the extension for example. After this CL: The creation flag has been replaced with a pref which is updated properly upon receiving Sync data. BUG=619526 ==========
https://codereview.chromium.org/2054773002/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/extension_service_sync_unittest.cc (right): https://codereview.chromium.org/2054773002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/extension_service_sync_unittest.cc:1732: EXPECT_TRUE(registry()->enabled_extensions().GetByID(id)); nit: Contains instead of GetByID https://codereview.chromium.org/2054773002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/extension_service_sync_unittest.cc:1768: EXPECT_TRUE(registry()->enabled_extensions().GetByID(id)); nit: Contains instead of GetByID https://codereview.chromium.org/2054773002/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/2054773002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/extension_util.cc:232: new base::FundamentalValue(installed_by_custodian)); If installed_by_custodian is false, then we should probably just clear the pref instead of setting it to false explicitly - as it is, we'll end up storing the pref for all users, even if they're not SUs at all. So something like: installed_by_custodian ? new base::FundamentalValue(true) : nullptr https://codereview.chromium.org/2054773002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/extension_util.cc:235: // If the installed_by_custodian flag is reset, do nothing. Hm. Is this correct? Might we need to unload the extension? Maybe something like "if (enabled != installed_by_custodian) Reload()" ?
https://codereview.chromium.org/2054773002/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/extension_service_sync_unittest.cc (right): https://codereview.chromium.org/2054773002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/extension_service_sync_unittest.cc:1732: EXPECT_TRUE(registry()->enabled_extensions().GetByID(id)); On 2016/06/17 16:34:22, Marc Treib wrote: > nit: Contains instead of GetByID Done. https://codereview.chromium.org/2054773002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/extension_service_sync_unittest.cc:1768: EXPECT_TRUE(registry()->enabled_extensions().GetByID(id)); On 2016/06/17 16:34:22, Marc Treib wrote: > nit: Contains instead of GetByID Done. https://codereview.chromium.org/2054773002/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/2054773002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/extension_util.cc:232: new base::FundamentalValue(installed_by_custodian)); On 2016/06/17 16:34:22, Marc Treib wrote: > If installed_by_custodian is false, then we should probably just clear the pref > instead of setting it to false explicitly - as it is, we'll end up storing the > pref for all users, even if they're not SUs at all. So something like: > installed_by_custodian ? new base::FundamentalValue(true) : nullptr Done. https://codereview.chromium.org/2054773002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/extension_util.cc:235: // If the installed_by_custodian flag is reset, do nothing. On 2016/06/17 16:34:22, Marc Treib wrote: > Hm. Is this correct? Might we need to unload the extension? > Maybe something like "if (enabled != installed_by_custodian) Reload()" ? The unload would happen in the callback of "MayLoad" in Supervised User service. So adding it here would be unnecessary, or what do you think?
https://codereview.chromium.org/2054773002/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/2054773002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/extension_util.cc:235: // If the installed_by_custodian flag is reset, do nothing. On 2016/06/19 11:43:09, mamir wrote: > On 2016/06/17 16:34:22, Marc Treib wrote: > > Hm. Is this correct? Might we need to unload the extension? > > Maybe something like "if (enabled != installed_by_custodian) Reload()" ? > > The unload would happen in the callback of "MayLoad" in Supervised User service. > So adding it here would be unnecessary, or what do you think? But UserMayLoad won't get called, right?
https://codereview.chromium.org/2054773002/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/2054773002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/extension_util.cc:235: // If the installed_by_custodian flag is reset, do nothing. On 2016/06/20 12:42:28, Marc Treib wrote: > On 2016/06/19 11:43:09, mamir wrote: > > On 2016/06/17 16:34:22, Marc Treib wrote: > > > Hm. Is this correct? Might we need to unload the extension? > > > Maybe something like "if (enabled != installed_by_custodian) Reload()" ? > > > > The unload would happen in the callback of "MayLoad" in Supervised User > service. > > So adding it here would be unnecessary, or what do you think? > > But UserMayLoad won't get called, right? in which case? ahaaa, you refer to when the flag is coming from Sync for example as false?
https://codereview.chromium.org/2054773002/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/2054773002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/extension_util.cc:235: // If the installed_by_custodian flag is reset, do nothing. On 2016/06/20 12:46:46, mamir wrote: > On 2016/06/20 12:42:28, Marc Treib wrote: > > On 2016/06/19 11:43:09, mamir wrote: > > > On 2016/06/17 16:34:22, Marc Treib wrote: > > > > Hm. Is this correct? Might we need to unload the extension? > > > > Maybe something like "if (enabled != installed_by_custodian) Reload()" ? > > > > > > The unload would happen in the callback of "MayLoad" in Supervised User > > service. > > > So adding it here would be unnecessary, or what do you think? > > > > But UserMayLoad won't get called, right? > > in which case? > ahaaa, you refer to when the flag is coming from Sync for example as false? Yes. If the flag is set from "true" to "false" here, then we might need to unload the extension. The next time UserMayLoad is called for some other reason, that will happen. But it won't get called automatically here.
https://codereview.chromium.org/2054773002/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/2054773002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/extension_util.cc:235: // If the installed_by_custodian flag is reset, do nothing. On 2016/06/20 15:01:30, Marc Treib wrote: > On 2016/06/20 12:46:46, mamir wrote: > > On 2016/06/20 12:42:28, Marc Treib wrote: > > > On 2016/06/19 11:43:09, mamir wrote: > > > > On 2016/06/17 16:34:22, Marc Treib wrote: > > > > > Hm. Is this correct? Might we need to unload the extension? > > > > > Maybe something like "if (enabled != installed_by_custodian) Reload()" ? > > > > > > > > The unload would happen in the callback of "MayLoad" in Supervised User > > > service. > > > > So adding it here would be unnecessary, or what do you think? > > > > > > But UserMayLoad won't get called, right? > > > > in which case? > > ahaaa, you refer to when the flag is coming from Sync for example as false? > > Yes. If the flag is set from "true" to "false" here, then we might need to > unload the extension. The next time UserMayLoad is called for some other reason, > that will happen. But it won't get called automatically here. Done.
https://codereview.chromium.org/2054773002/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/2054773002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/extension_util.cc:58: // It is relevant only for supervised users nit: period after comment https://codereview.chromium.org/2054773002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/extension_util.cc:240: service->ReloadExtension(extension_id); You'll get here every time any sync data for an extension arrives. The reload should only happen if the value of the flag *changes*. https://codereview.chromium.org/2054773002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/extension_util.cc:244: // If it is already enabled, do nothing; nit: period after comment https://codereview.chromium.org/2054773002/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/extension_util.h (right): https://codereview.chromium.org/2054773002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/extension_util.h:74: // for extensions installed by their custodian (e.g. they cannot uninstall them) nit: period after comment https://codereview.chromium.org/2054773002/diff/260001/chrome/browser/supervi... File chrome/browser/supervised_user/supervised_user_service_unittest.cc (right): https://codereview.chromium.org/2054773002/diff/260001/chrome/browser/supervi... chrome/browser/supervised_user/supervised_user_service_unittest.cc:416: extensions::util::SetWasInstalledByCustodian(extension->id(), nit: misaligned https://codereview.chromium.org/2054773002/diff/260001/extensions/common/exte... File extensions/common/extension.h (right): https://codereview.chromium.org/2054773002/diff/260001/extensions/common/exte... extensions/common/extension.h:155: // DEPRECATED:: WAS_INSTALLED_BY_CUSTODIAN is now stored as a pref instead. nit: double colon
https://codereview.chromium.org/2054773002/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/2054773002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/extension_util.cc:58: // It is relevant only for supervised users On 2016/06/20 16:42:38, Marc Treib wrote: > nit: period after comment Done. https://codereview.chromium.org/2054773002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/extension_util.cc:240: service->ReloadExtension(extension_id); On 2016/06/20 16:42:38, Marc Treib wrote: > You'll get here every time any sync data for an extension arrives. The reload > should only happen if the value of the flag *changes*. Done. https://codereview.chromium.org/2054773002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/extension_util.cc:244: // If it is already enabled, do nothing; On 2016/06/20 16:42:38, Marc Treib wrote: > nit: period after comment Done. https://codereview.chromium.org/2054773002/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/extension_util.h (right): https://codereview.chromium.org/2054773002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/extension_util.h:74: // for extensions installed by their custodian (e.g. they cannot uninstall them) On 2016/06/20 16:42:38, Marc Treib wrote: > nit: period after comment Done. https://codereview.chromium.org/2054773002/diff/260001/chrome/browser/supervi... File chrome/browser/supervised_user/supervised_user_service_unittest.cc (right): https://codereview.chromium.org/2054773002/diff/260001/chrome/browser/supervi... chrome/browser/supervised_user/supervised_user_service_unittest.cc:416: extensions::util::SetWasInstalledByCustodian(extension->id(), On 2016/06/20 16:42:38, Marc Treib wrote: > nit: misaligned Done. https://codereview.chromium.org/2054773002/diff/260001/extensions/common/exte... File extensions/common/extension.h (right): https://codereview.chromium.org/2054773002/diff/260001/extensions/common/exte... extensions/common/extension.h:155: // DEPRECATED:: WAS_INSTALLED_BY_CUSTODIAN is now stored as a pref instead. On 2016/06/20 16:42:39, Marc Treib wrote: > nit: double colon Done.
LGTM again, thanks!
https://codereview.chromium.org/2054773002/diff/280001/chrome/browser/extensi... File chrome/browser/extensions/extension_sync_service.cc (right): https://codereview.chromium.org/2054773002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/extension_sync_service.cc:321: extensions::util::SetWasInstalledByCustodian( It might be worth a comment: // Note: this may cause an existing version of the extension to be reloaded. https://codereview.chromium.org/2054773002/diff/280001/chrome/browser/extensi... File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/2054773002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/extension_util.cc:243: service->ReloadExtension(extension_id); Where is service defined? https://codereview.chromium.org/2054773002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/extension_util.cc:251: // If the extension is not installed, it may need to be reloaded. This comment doesn't make sense. If it's not installed, it *can't* be reloaded. :) https://codereview.chromium.org/2054773002/diff/280001/chrome/browser/supervi... File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/2054773002/diff/280001/chrome/browser/supervi... chrome/browser/supervised_user/supervised_user_service.cc:924: SupervisedUserService::ExtensionState SupervisedUserService::GetExtensionState( It's not clear to me why we moved this in this CL? https://codereview.chromium.org/2054773002/diff/280001/chrome/browser/supervi... File chrome/browser/supervised_user/supervised_user_service.h (right): https://codereview.chromium.org/2054773002/diff/280001/chrome/browser/supervi... chrome/browser/supervised_user/supervised_user_service.h:307: ExtensionState GetExtensionState( This warrants a comment
https://codereview.chromium.org/2054773002/diff/280001/chrome/browser/extensi... File chrome/browser/extensions/extension_sync_service.cc (right): https://codereview.chromium.org/2054773002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/extension_sync_service.cc:321: extensions::util::SetWasInstalledByCustodian( On 2016/06/20 17:09:24, Devlin wrote: > It might be worth a comment: > // Note: this may cause an existing version of the extension to be reloaded. Done. https://codereview.chromium.org/2054773002/diff/280001/chrome/browser/extensi... File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/2054773002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/extension_util.cc:243: service->ReloadExtension(extension_id); On 2016/06/20 17:09:24, Devlin wrote: > Where is service defined? Sorry! Done! https://codereview.chromium.org/2054773002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/extension_util.cc:251: // If the extension is not installed, it may need to be reloaded. On 2016/06/20 17:09:25, Devlin wrote: > This comment doesn't make sense. If it's not installed, it *can't* be reloaded. > :) Done. https://codereview.chromium.org/2054773002/diff/280001/chrome/browser/supervi... File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/2054773002/diff/280001/chrome/browser/supervi... chrome/browser/supervised_user/supervised_user_service.cc:924: SupervisedUserService::ExtensionState SupervisedUserService::GetExtensionState( On 2016/06/20 17:09:25, Devlin wrote: > It's not clear to me why we moved this in this CL? The reason is it needed access to profile to call extension::util::WasInstalledByCustodian(); While before this CL, the information existed in the extension object itself. So, instead of sending another parameter, it made more sense to move it. https://codereview.chromium.org/2054773002/diff/280001/chrome/browser/supervi... File chrome/browser/supervised_user/supervised_user_service.h (right): https://codereview.chromium.org/2054773002/diff/280001/chrome/browser/supervi... chrome/browser/supervised_user/supervised_user_service.h:307: ExtensionState GetExtensionState( On 2016/06/20 17:09:25, Devlin wrote: > This warrants a comment Done.
mamir@chromium.org changed reviewers: + caitkp@chromium.org, pkotwicz@chromium.org
caitkp@chromium.org: Please review changes in chrome/browser/safe_browsing/incident_reporting/extension_data_collection.cc pkotwicz@chromium.org: Please review changes in chrome/browser/themes/theme_syncable_service.cc
chrome/browser/themes/theme_syncable_service.cc rubberstamp LGTM
chrome/browser/themes/theme_syncable_service.cc rubberstamp LGTM
chrome/browser/themes/theme_syncable_service.cc rubberstamp LGTM
lgtm https://codereview.chromium.org/2054773002/diff/280001/chrome/browser/supervi... File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/2054773002/diff/280001/chrome/browser/supervi... chrome/browser/supervised_user/supervised_user_service.cc:924: SupervisedUserService::ExtensionState SupervisedUserService::GetExtensionState( On 2016/06/21 11:38:22, mamir wrote: > On 2016/06/20 17:09:25, Devlin wrote: > > It's not clear to me why we moved this in this CL? > > The reason is it needed access to profile to call > extension::util::WasInstalledByCustodian(); > While before this CL, the information existed in the extension object itself. > So, instead of sending another parameter, it made more sense to move it. Ah, I missed the change to use profile_. That makes sense.
lgtm
The CQ bit was checked by mamir@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, pkotwicz@chromium.org, rdevlin.cronin@chromium.org, caitkp@chromium.org Link to the patchset: https://codereview.chromium.org/2054773002/#ps320001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2054773002/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
mamir@chromium.org changed reviewers: + pavely@chromium.org
pavely@chromium.org: Please review changes in chrome/browser/sync/test/integration/two_client_apps_sync_test.cc
The CQ bit was checked by mamir@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2054773002/340001
The CQ bit was unchecked by mamir@chromium.org
The CQ bit was checked by mamir@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2054773002/340001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
chrome/browser/sync/test/integration/two_client_apps_sync_test.cc lgtm
The CQ bit was checked by mamir@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, pkotwicz@chromium.org, rdevlin.cronin@chromium.org, caitkp@chromium.org Link to the patchset: https://codereview.chromium.org/2054773002/#ps340001 (title: "Fixing the build")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2054773002/340001
Message was sent while issue was closed.
Description was changed from ========== Replace the WAS_INSTALLED_BY_CUSTODIAN creation flag with a pref Before this CL: If a custodian-installed extension was already installed in the browser before the child signs in, the "WasInstalledByCustodian" flag is stored in Sync was ignored and the default value (false) is used giving the child the ability to uninstall the extension for example. After this CL: The creation flag has been replaced with a pref which is updated properly upon receiving Sync data. BUG=619526 ========== to ========== Replace the WAS_INSTALLED_BY_CUSTODIAN creation flag with a pref Before this CL: If a custodian-installed extension was already installed in the browser before the child signs in, the "WasInstalledByCustodian" flag is stored in Sync was ignored and the default value (false) is used giving the child the ability to uninstall the extension for example. After this CL: The creation flag has been replaced with a pref which is updated properly upon receiving Sync data. BUG=619526 ==========
Message was sent while issue was closed.
Committed patchset #18 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== Replace the WAS_INSTALLED_BY_CUSTODIAN creation flag with a pref Before this CL: If a custodian-installed extension was already installed in the browser before the child signs in, the "WasInstalledByCustodian" flag is stored in Sync was ignored and the default value (false) is used giving the child the ability to uninstall the extension for example. After this CL: The creation flag has been replaced with a pref which is updated properly upon receiving Sync data. BUG=619526 ========== to ========== Replace the WAS_INSTALLED_BY_CUSTODIAN creation flag with a pref Before this CL: If a custodian-installed extension was already installed in the browser before the child signs in, the "WasInstalledByCustodian" flag is stored in Sync was ignored and the default value (false) is used giving the child the ability to uninstall the extension for example. After this CL: The creation flag has been replaced with a pref which is updated properly upon receiving Sync data. BUG=619526 Committed: https://crrev.com/192d78833ff9297c754f59f20cb4e590d693a7c2 Cr-Commit-Position: refs/heads/master@{#401318} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/192d78833ff9297c754f59f20cb4e590d693a7c2 Cr-Commit-Position: refs/heads/master@{#401318} |
