Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14490002:
Start refactoring extension reloading to avoid redundant reloads. (Closed)
|
Created:
7 years, 8 months ago by Jeffrey Yasskin Modified:
7 years, 8 months ago CC:
chromium-reviews, Aaron Boodman, tfarina, chromium-apps-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionStart refactoring extension reloading to avoid redundant reloads.
This isn't intended to change any behavior, but it may fix a ui glitch
in the BrowserActionsContainer between an upgrade and a reload.
BUG=235681
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=196634
Patch Set 1 : #
Total comments: 15
Patch Set 2 : Clean up and fix DISABLE_UNKNOWN_FROM_SYNC logic #
Messages
Total messages: 10 (0 generated)
https://codereview.chromium.org/14490002/diff/4001/chrome/browser/extensions/... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/14490002/diff/4001/chrome/browser/extensions/... chrome/browser/extensions/extension_service.cc:2056: EnableExtension(extension->id()); This Enable followed by Unload can cause an extra subprocess to be created and destroyed. I'm planning to sanitize it in a subsequent change. https://codereview.chromium.org/14490002/diff/4001/chrome/browser/extensions/... chrome/browser/extensions/extension_service.cc:2104: SetBeingUpgraded(extension, false); This should be late enough to cover the uses inside BrowserActionsContainer. BrowserActionAdded is called transitively through NotifyExtensionLoaded. https://codereview.chromium.org/14490002/diff/4001/chrome/browser/extensions/... chrome/browser/extensions/extension_service.cc:2196: // Add all the recognized permissions if the granted permissions list This isn't actually the effect of the next line, as far as I can tell. I'm inclined just to remove the comment. https://codereview.chromium.org/14490002/diff/4001/chrome/browser/extensions/... chrome/browser/extensions/extension_service.cc:2227: extension_prefs_->ClearDisableReasons(extension->id()); If there's no privilege increase, this will drop all the disable reasons from the extension, even though the code looks like it wants to set DISABLE_USER_ACTION. Thoughts on the right fix? https://codereview.chromium.org/14490002/diff/4001/chrome/browser/extensions/... chrome/browser/extensions/extension_service.cc:2232: disable_reasons &= ~Extension::DISABLE_UNKNOWN_FROM_SYNC; This has no effect unless the ClearDisableReasons line was executed. The "if" is also pointless since clearing a bit has no effect on a DISABLE_NONE reason. https://codereview.chromium.org/14490002/diff/4001/chrome/browser/extensions/... File chrome/browser/extensions/extension_service.h (left): https://codereview.chromium.org/14490002/diff/4001/chrome/browser/extensions/... chrome/browser/extensions/extension_service.h:455: void InitializePermissions(const extensions::Extension* extension); Made private and renamed.
LGTM https://codereview.chromium.org/14490002/diff/4001/chrome/browser/extensions/... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/14490002/diff/4001/chrome/browser/extensions/... chrome/browser/extensions/extension_service.cc:2056: EnableExtension(extension->id()); On 2013/04/25 01:53:37, Jeffrey Yasskin wrote: > This Enable followed by Unload can cause an extra subprocess to be created and > destroyed. I'm planning to sanitize it in a subsequent change. Do you mean the Unload on line 2065? Can you just move this after that line? https://codereview.chromium.org/14490002/diff/4001/chrome/browser/extensions/... chrome/browser/extensions/extension_service.cc:2227: extension_prefs_->ClearDisableReasons(extension->id()); On 2013/04/25 01:53:37, Jeffrey Yasskin wrote: > If there's no privilege increase, this will drop all the disable reasons from > the extension, even though the code looks like it wants to set > DISABLE_USER_ACTION. Thoughts on the right fix? Seems harmless here, though, because disable_reasons == Extension::DISABLE_UNKNOWN_FROM_SYNC. We want to clear that anyway.
https://codereview.chromium.org/14490002/diff/4001/chrome/browser/extensions/... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/14490002/diff/4001/chrome/browser/extensions/... chrome/browser/extensions/extension_service.cc:2056: EnableExtension(extension->id()); On 2013/04/25 19:03:46, Matt Perry wrote: > On 2013/04/25 01:53:37, Jeffrey Yasskin wrote: > > This Enable followed by Unload can cause an extra subprocess to be created and > > destroyed. I'm planning to sanitize it in a subsequent change. > > Do you mean the Unload on line 2065? Can you just move this after that line? Not immediately, that breaks tests. :) I'm figuring out how to move it in a separate change. https://codereview.chromium.org/14490002/diff/4001/chrome/browser/extensions/... chrome/browser/extensions/extension_service.cc:2227: extension_prefs_->ClearDisableReasons(extension->id()); On 2013/04/25 19:03:46, Matt Perry wrote: > On 2013/04/25 01:53:37, Jeffrey Yasskin wrote: > > If there's no privilege increase, this will drop all the disable reasons from > > the extension, even though the code looks like it wants to set > > DISABLE_USER_ACTION. Thoughts on the right fix? > > Seems harmless here, though, because disable_reasons == > Extension::DISABLE_UNKNOWN_FROM_SYNC. We want to clear that anyway. We want to clear that and add DISABLE_USER_ACTION. Without a privilege increase, we lose the DISABLE_USER_ACTION.
https://codereview.chromium.org/14490002/diff/4001/chrome/browser/extensions/... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/14490002/diff/4001/chrome/browser/extensions/... chrome/browser/extensions/extension_service.cc:2232: disable_reasons &= ~Extension::DISABLE_UNKNOWN_FROM_SYNC; On 2013/04/25 01:53:37, Jeffrey Yasskin wrote: > This has no effect unless the ClearDisableReasons line was executed. The "if" > is also pointless since clearing a bit has no effect on a DISABLE_NONE reason. This logic is quite complicated to compensate for not syncing disable reasons... If we're UNKNOWN_FROM_SYNC and there is no privilege increase, it should be USER_ACTION (2228). Otherwise it should be PERMISSIONS_INCREASE (2237). In any case we want to clear the UNKNOWN_FROM_SYNC bit. But you're right that these 2 lines seem to do nothing. There also seems to be a bug here where we don't actually AddDisableReason for the new disable reasons. My vote for the fix is to pull 2243 and 2245 out of the if (is_privilege_increase) and put them into an if (disable_reasons != Extension::DISABLE_NONE).
https://codereview.chromium.org/14490002/diff/4001/chrome/browser/extensions/... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/14490002/diff/4001/chrome/browser/extensions/... chrome/browser/extensions/extension_service.cc:2227: extension_prefs_->ClearDisableReasons(extension->id()); On 2013/04/25 19:12:28, Jeffrey Yasskin wrote: > On 2013/04/25 19:03:46, Matt Perry wrote: > > On 2013/04/25 01:53:37, Jeffrey Yasskin wrote: > > > If there's no privilege increase, this will drop all the disable reasons > from > > > the extension, even though the code looks like it wants to set > > > DISABLE_USER_ACTION. Thoughts on the right fix? > > > > Seems harmless here, though, because disable_reasons == > > Extension::DISABLE_UNKNOWN_FROM_SYNC. We want to clear that anyway. > > We want to clear that and add DISABLE_USER_ACTION. Without a privilege increase, > we lose the DISABLE_USER_ACTION. We only add DISABLE_USER_ACTION if it's *not* a privilege increase. That's because if it was a privilege increase, we can't assume the user disabled it explicitly. It may have been disabled due to an update.
https://codereview.chromium.org/14490002/diff/4001/chrome/browser/extensions/... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/14490002/diff/4001/chrome/browser/extensions/... chrome/browser/extensions/extension_service.cc:2227: extension_prefs_->ClearDisableReasons(extension->id()); On 2013/04/25 19:22:43, Matt Perry wrote: > On 2013/04/25 19:12:28, Jeffrey Yasskin wrote: > > On 2013/04/25 19:03:46, Matt Perry wrote: > > > On 2013/04/25 01:53:37, Jeffrey Yasskin wrote: > > > > If there's no privilege increase, this will drop all the disable reasons > > from > > > > the extension, even though the code looks like it wants to set > > > > DISABLE_USER_ACTION. Thoughts on the right fix? > > > > > > Seems harmless here, though, because disable_reasons == > > > Extension::DISABLE_UNKNOWN_FROM_SYNC. We want to clear that anyway. > > > > We want to clear that and add DISABLE_USER_ACTION. Without a privilege > increase, > > we lose the DISABLE_USER_ACTION. > > We only add DISABLE_USER_ACTION if it's *not* a privilege increase. That's > because if it was a privilege increase, we can't assume the user disabled it > explicitly. It may have been disabled due to an update. I think we (I, specifically) forgot to actually add DISABLE_USER_ACTION.
https://codereview.chromium.org/14490002/diff/4001/chrome/browser/extensions/... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/14490002/diff/4001/chrome/browser/extensions/... chrome/browser/extensions/extension_service.cc:2227: extension_prefs_->ClearDisableReasons(extension->id()); On 2013/04/25 19:24:39, Yoyo Zhou wrote: > On 2013/04/25 19:22:43, Matt Perry wrote: > > On 2013/04/25 19:12:28, Jeffrey Yasskin wrote: > > > On 2013/04/25 19:03:46, Matt Perry wrote: > > > > On 2013/04/25 01:53:37, Jeffrey Yasskin wrote: > > > > > If there's no privilege increase, this will drop all the disable reasons > > > from > > > > > the extension, even though the code looks like it wants to set > > > > > DISABLE_USER_ACTION. Thoughts on the right fix? > > > > > > > > Seems harmless here, though, because disable_reasons == > > > > Extension::DISABLE_UNKNOWN_FROM_SYNC. We want to clear that anyway. > > > > > > We want to clear that and add DISABLE_USER_ACTION. Without a privilege > > increase, > > > we lose the DISABLE_USER_ACTION. > > > > We only add DISABLE_USER_ACTION if it's *not* a privilege increase. That's > > because if it was a privilege increase, we can't assume the user disabled it > > explicitly. It may have been disabled due to an update. > > I think we (I, specifically) forgot to actually add DISABLE_USER_ACTION. Oh right, because we only AddDisableReason if it's a privilege increase... oops.
https://codereview.chromium.org/14490002/diff/4001/chrome/browser/extensions/... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/14490002/diff/4001/chrome/browser/extensions/... chrome/browser/extensions/extension_service.cc:2232: disable_reasons &= ~Extension::DISABLE_UNKNOWN_FROM_SYNC; On 2013/04/25 19:21:58, Yoyo Zhou wrote: > On 2013/04/25 01:53:37, Jeffrey Yasskin wrote: > > This has no effect unless the ClearDisableReasons line was executed. The "if" > > is also pointless since clearing a bit has no effect on a DISABLE_NONE reason. > > This logic is quite complicated to compensate for not syncing disable reasons... > > If we're UNKNOWN_FROM_SYNC and there is no privilege increase, it should be > USER_ACTION (2228). Otherwise it should be PERMISSIONS_INCREASE (2237). In any > case we want to clear the UNKNOWN_FROM_SYNC bit. > > But you're right that these 2 lines seem to do nothing. > > There also seems to be a bug here where we don't actually AddDisableReason for > the new disable reasons. > > My vote for the fix is to pull 2243 and 2245 out of the if > (is_privilege_increase) and put them into an if (disable_reasons != > Extension::DISABLE_NONE). Moving SetExtensionState broke a ManagementPolicy test, apparently because management policy resets the enabled state without removing any disable reason, and runs before the permission check. We should try to figure that out in a separate change, because I'm worried it means that management policy is overridden by permission increases.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jyasskin@chromium.org/14490002/16001
Message was sent while issue was closed.
Change committed as 196634 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
