Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
21443002:
Add NOTIFICATION_EXTENSION_REMOVED for Extensions removed from ExtensionService. (Closed)
|
Created:
7 years, 4 months ago by Yoyo Zhou Modified:
7 years, 4 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd NOTIFICATION_EXTENSION_REMOVED for Extensions removed from ExtensionService.
Notably, error messages that prompt the user to enable a disabled extension need to go away whenever the extension is removed.
BUG=263216, 265112
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=215360
Patch Set 1 #
Total comments: 2
Patch Set 2 : take 2 #
Total comments: 2
Patch Set 3 : sigh #Patch Set 4 : #
Messages
Total messages: 18 (0 generated)
https://codereview.chromium.org/21443002/diff/1/chrome/browser/extensions/ext... File chrome/browser/extensions/extension_service.h (right): https://codereview.chromium.org/21443002/diff/1/chrome/browser/extensions/ext... chrome/browser/extensions/extension_service.h:144: virtual void OnDisabledExtensionRemoved( This seems oddly specific. I think it should be split into 2 notifications: - EXTENSION_ENABLED (or _LOADED would work, too) - EXTENSION_DELETED (or Observer methods if you prefer that. But then I would add the methods to InstallObserver https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... )
https://codereview.chromium.org/21443002/diff/1/chrome/browser/extensions/ext... File chrome/browser/extensions/extension_service.h (right): https://codereview.chromium.org/21443002/diff/1/chrome/browser/extensions/ext... chrome/browser/extensions/extension_service.h:144: virtual void OnDisabledExtensionRemoved( On 2013/08/01 00:37:33, Matt Perry wrote: > This seems oddly specific. I think it should be split into 2 notifications: > - EXTENSION_ENABLED (or _LOADED would work, too) > - EXTENSION_DELETED > (or Observer methods if you prefer that. But then I would add the methods to > InstallObserver > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > ) To be clear, there are paths which result in an Extension being destroyed without it being enabled or uninstalled (I think upgrade might be one of them). We're actually interested in knowing whenever it's not disabled anymore. For that we could bring back the EXTENSION_UNLOADED_DISABLED notification, but in general I thought we were moving away from notifications. So would you prefer this to be two methods, OnDisabledExtensionEnabled and OnDisabledExtensionDeleted? InstallObserver doesn't really look like a good fit. Too many pure virtual methods we don't care about.
On 2013/08/01 00:46:35, Yoyo Zhou wrote: > https://codereview.chromium.org/21443002/diff/1/chrome/browser/extensions/ext... > File chrome/browser/extensions/extension_service.h (right): > > https://codereview.chromium.org/21443002/diff/1/chrome/browser/extensions/ext... > chrome/browser/extensions/extension_service.h:144: virtual void > OnDisabledExtensionRemoved( > On 2013/08/01 00:37:33, Matt Perry wrote: > > This seems oddly specific. I think it should be split into 2 notifications: > > - EXTENSION_ENABLED (or _LOADED would work, too) > > - EXTENSION_DELETED > > (or Observer methods if you prefer that. But then I would add the methods to > > InstallObserver > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > ) > > To be clear, there are paths which result in an Extension being destroyed > without it being enabled or uninstalled (I think upgrade might be one of them). > We're actually interested in knowing whenever it's not disabled anymore. For > that we could bring back the EXTENSION_UNLOADED_DISABLED notification, but in > general I thought we were moving away from notifications. Eh, I suppose... personally I don't see the advantage of Observers over Notifications. > > So would you prefer this to be two methods, OnDisabledExtensionEnabled and > OnDisabledExtensionDeleted? OnDisabledExtensionEnabled already exists as NOTIFICATION_EXTENSION_ENABLED. If we add OnExtensionDeleted, would that help? It seems to me that the consumers don't actually care whether it's disabled - they want to know when the extension object is deleted.
On 2013/08/01 00:56:07, Matt Perry wrote: > On 2013/08/01 00:46:35, Yoyo Zhou wrote: > > > https://codereview.chromium.org/21443002/diff/1/chrome/browser/extensions/ext... > > File chrome/browser/extensions/extension_service.h (right): > > > > > https://codereview.chromium.org/21443002/diff/1/chrome/browser/extensions/ext... > > chrome/browser/extensions/extension_service.h:144: virtual void > > OnDisabledExtensionRemoved( > > On 2013/08/01 00:37:33, Matt Perry wrote: > > > This seems oddly specific. I think it should be split into 2 notifications: > > > - EXTENSION_ENABLED (or _LOADED would work, too) > > > - EXTENSION_DELETED > > > (or Observer methods if you prefer that. But then I would add the methods to > > > InstallObserver > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > > ) > > > > To be clear, there are paths which result in an Extension being destroyed > > without it being enabled or uninstalled (I think upgrade might be one of > them). > > We're actually interested in knowing whenever it's not disabled anymore. For > > that we could bring back the EXTENSION_UNLOADED_DISABLED notification, but in > > general I thought we were moving away from notifications. > > Eh, I suppose... personally I don't see the advantage of Observers over > Notifications. Actually I'm inclined to go back to the notification, since it's easier than making sure all the errors get cleaned up in unit tests. What do you think? > > > > So would you prefer this to be two methods, OnDisabledExtensionEnabled and > > OnDisabledExtensionDeleted? > > OnDisabledExtensionEnabled already exists as NOTIFICATION_EXTENSION_ENABLED. > > If we add OnExtensionDeleted, would that help? It seems to me that the consumers > don't actually care whether it's disabled - they want to know when the extension > object is deleted. Extensions are ref-counted, so this would not be straightforward.
On 2013/08/01 19:24:38, Yoyo Zhou wrote: > On 2013/08/01 00:56:07, Matt Perry wrote: > > On 2013/08/01 00:46:35, Yoyo Zhou wrote: > > > > > > https://codereview.chromium.org/21443002/diff/1/chrome/browser/extensions/ext... > > > File chrome/browser/extensions/extension_service.h (right): > > > > > > > > > https://codereview.chromium.org/21443002/diff/1/chrome/browser/extensions/ext... > > > chrome/browser/extensions/extension_service.h:144: virtual void > > > OnDisabledExtensionRemoved( > > > On 2013/08/01 00:37:33, Matt Perry wrote: > > > > This seems oddly specific. I think it should be split into 2 > notifications: > > > > - EXTENSION_ENABLED (or _LOADED would work, too) > > > > - EXTENSION_DELETED > > > > (or Observer methods if you prefer that. But then I would add the methods > to > > > > InstallObserver > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > > > ) > > > > > > To be clear, there are paths which result in an Extension being destroyed > > > without it being enabled or uninstalled (I think upgrade might be one of > > them). > > > We're actually interested in knowing whenever it's not disabled anymore. For > > > that we could bring back the EXTENSION_UNLOADED_DISABLED notification, but > in > > > general I thought we were moving away from notifications. > > > > Eh, I suppose... personally I don't see the advantage of Observers over > > Notifications. > > Actually I'm inclined to go back to the notification, since it's easier than > making sure all the errors get cleaned up in unit tests. What do you think? I'm not a fan of the UNLOADED_DISABLED notification because it's conceptually confusing. I'd prefer something like REMOVED to notify consumers whenever an extension is purged from the ExtensionService lists - disabled or enabled doesn't matter. > > > > > > > So would you prefer this to be two methods, OnDisabledExtensionEnabled and > > > OnDisabledExtensionDeleted? > > > > OnDisabledExtensionEnabled already exists as NOTIFICATION_EXTENSION_ENABLED. > > > > If we add OnExtensionDeleted, would that help? It seems to me that the > consumers > > don't actually care whether it's disabled - they want to know when the > extension > > object is deleted. > > Extensions are ref-counted, so this would not be straightforward. Oh right, good point. "Removed", then.
On 2013/08/01 19:29:19, Matt Perry wrote: > On 2013/08/01 19:24:38, Yoyo Zhou wrote: > > On 2013/08/01 00:56:07, Matt Perry wrote: > > > On 2013/08/01 00:46:35, Yoyo Zhou wrote: > > > > > > > > > > https://codereview.chromium.org/21443002/diff/1/chrome/browser/extensions/ext... > > > > File chrome/browser/extensions/extension_service.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/21443002/diff/1/chrome/browser/extensions/ext... > > > > chrome/browser/extensions/extension_service.h:144: virtual void > > > > OnDisabledExtensionRemoved( > > > > On 2013/08/01 00:37:33, Matt Perry wrote: > > > > > This seems oddly specific. I think it should be split into 2 > > notifications: > > > > > - EXTENSION_ENABLED (or _LOADED would work, too) > > > > > - EXTENSION_DELETED > > > > > (or Observer methods if you prefer that. But then I would add the > methods > > to > > > > > InstallObserver > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > > > > ) > > > > > > > > To be clear, there are paths which result in an Extension being destroyed > > > > without it being enabled or uninstalled (I think upgrade might be one of > > > them). > > > > We're actually interested in knowing whenever it's not disabled anymore. > For > > > > that we could bring back the EXTENSION_UNLOADED_DISABLED notification, but > > in > > > > general I thought we were moving away from notifications. > > > > > > Eh, I suppose... personally I don't see the advantage of Observers over > > > Notifications. > > > > Actually I'm inclined to go back to the notification, since it's easier than > > making sure all the errors get cleaned up in unit tests. What do you think? > > I'm not a fan of the UNLOADED_DISABLED notification because it's conceptually > confusing. I'd prefer something like REMOVED to notify consumers whenever an > extension is purged from the ExtensionService lists - disabled or enabled > doesn't matter. To do this right, we'd have to fix the dual meaning of "unloaded" in ExtensionService. One (represented by NOTIFICATION_EXTENSION_UNLOADED) is the opposite of NOTIFICATION_EXTENSION_LOADED - the extension has been removed from the extensions() (enabled extensions) list. (That is, unless it's been removed from the disabled list... this is the bug in question.) The other, ExtensionService::UnloadExtension, is more like this REMOVED. (Unless it's added to the safe browsing blacklist. Then it's still kept around in blacklisted_extensions(). Management blacklisted extensions, however, appear to actually be removed.) Thus, DisableExtension calls NotifyExtensionUnloaded but not UnloadExtension. But if we aren't concerned about accuracy with regards to the blacklist, the REMOVED notification would be doable.
On 2013/08/01 20:23:40, Yoyo Zhou wrote: > On 2013/08/01 19:29:19, Matt Perry wrote: > > On 2013/08/01 19:24:38, Yoyo Zhou wrote: > > > On 2013/08/01 00:56:07, Matt Perry wrote: > > > > On 2013/08/01 00:46:35, Yoyo Zhou wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/21443002/diff/1/chrome/browser/extensions/ext... > > > > > File chrome/browser/extensions/extension_service.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/21443002/diff/1/chrome/browser/extensions/ext... > > > > > chrome/browser/extensions/extension_service.h:144: virtual void > > > > > OnDisabledExtensionRemoved( > > > > > On 2013/08/01 00:37:33, Matt Perry wrote: > > > > > > This seems oddly specific. I think it should be split into 2 > > > notifications: > > > > > > - EXTENSION_ENABLED (or _LOADED would work, too) > > > > > > - EXTENSION_DELETED > > > > > > (or Observer methods if you prefer that. But then I would add the > > methods > > > to > > > > > > InstallObserver > > > > > > > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > > > > > ) > > > > > > > > > > To be clear, there are paths which result in an Extension being > destroyed > > > > > without it being enabled or uninstalled (I think upgrade might be one of > > > > them). > > > > > We're actually interested in knowing whenever it's not disabled anymore. > > For > > > > > that we could bring back the EXTENSION_UNLOADED_DISABLED notification, > but > > > in > > > > > general I thought we were moving away from notifications. > > > > > > > > Eh, I suppose... personally I don't see the advantage of Observers over > > > > Notifications. > > > > > > Actually I'm inclined to go back to the notification, since it's easier than > > > making sure all the errors get cleaned up in unit tests. What do you think? > > > > I'm not a fan of the UNLOADED_DISABLED notification because it's conceptually > > confusing. I'd prefer something like REMOVED to notify consumers whenever an > > extension is purged from the ExtensionService lists - disabled or enabled > > doesn't matter. > > To do this right, we'd have to fix the dual meaning of "unloaded" in > ExtensionService. > > NOTIFICATION_EXTENSION_LOADED - the extension has been removed from the > extensions() (enabled extensions) list. (That is, unless it's been removed from > One (represented by NOTIFICATION_EXTENSION_UNLOADED) is the opposite of > the disabled list... this is the bug in question.) > > The other, ExtensionService::UnloadExtension, is more like this REMOVED. (Unless > it's added to the safe browsing blacklist. Then it's still kept around in > blacklisted_extensions(). Management blacklisted extensions, however, appear to > actually be removed.) > > Thus, DisableExtension calls NotifyExtensionUnloaded but not UnloadExtension. > > But if we aren't concerned about accuracy with regards to the blacklist, the > REMOVED notification would be doable. Yikes, that's more of a mess than I thought. I could have sworn DisableExtension called UnloadExtension. Maybe UnloadExtension would be better called RemoveExtension... In any case, I still prefer a REMOVED notification (or observer). Send it just before removing an extension from the enabled/disabled list (without being added to the other).
Now with new notification. Please take a look.
https://codereview.chromium.org/21443002/diff/28001/chrome/browser/extensions... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/21443002/diff/28001/chrome/browser/extensions... chrome/browser/extensions/extension_service.cc:1891: return; You need to dispatch the notification here, too. Or move the Notify call above this if block.
https://codereview.chromium.org/21443002/diff/28001/chrome/browser/extensions... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/21443002/diff/28001/chrome/browser/extensions... chrome/browser/extensions/extension_service.cc:1891: return; On 2013/08/01 22:06:35, Matt Perry wrote: > You need to dispatch the notification here, too. Or move the Notify call above > this if block. Of course.
lgtm
jam, can you review the new notification in chrome_notification_types?
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoz@chromium.org/21443002/30002
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoz@chromium.org/21443002/30002
Message was sent while issue was closed.
Change committed as 215360 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
