Disable an extension when it is upgraded to a version that requires more

Disable an extension when it is upgraded to a version that requires more
permissions then before, and prompt the user to re-enable.

Incidentally, this required adding support for disabling extensions.

BUG=12140
TEST=covered by unit tests

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

[Aaron Boodman, 2009-08-13 19:30:43]

http://codereview.chromium.org/165414/diff/1/4
File chrome/browser/extensions/extension_browsertest.cc (right):

http://codereview.chromium.org/165414/diff/1/4#newcode46
Line 46: NotificationRegistrar registrar;
Fair enough :)

http://codereview.chromium.org/165414/diff/1/4#newcode50
Line 50: MessageLoop::current()->PostDelayedTask(FROM_HERE, new
MessageLoop::QuitTask,
80 cols

http://codereview.chromium.org/165414/diff/1/5
File chrome/browser/extensions/extension_browsertest.h (right):

http://codereview.chromium.org/165414/diff/1/5#newcode25
Line 25: // |expected_change| indicates how many more extensions should be
installed.
typo? "how many more"?

http://codereview.chromium.org/165414/diff/1023/1024
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/165414/diff/1023/1024#newcode2069
Line 2069: The newest version of extension "<ph
name="EXTENSION_NAME">$1<ex>Flashblock</ex></ph>" requires more permissions, so
it has been disabled.
Nit: "... newest version of **the** extension ...."

http://codereview.chromium.org/165414/diff/1023/1032
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/165414/diff/1023/1032#newcode43
Line 43: void OnExtensionDisabled(Extension* extension);
Since these methods are not called in response to notifications, I think it is
confusing for them to be prefixed with "On". Simply DisableExtension(), or
MarkExtensionDisabled() would make more sense to me.

If you did this to be consistent with OnExtensionInstalled(), maybe add a TODO
to rename them.

http://codereview.chromium.org/165414/diff/1023/1032#newcode50
Line 50: // Returns the state of the given extension according to the prefs.
Scary comment. Makes me think "who else has an opinion on the state of the
extension?". Maybe remove the "according to..." part?

http://codereview.chromium.org/165414/diff/1023/1034
File chrome/browser/extensions/extension_updater.cc (right):

http://codereview.chromium.org/165414/diff/1023/1034#newcode592
Line 592: Extension* extension = service_->GetExtensionByIdInternal(
Ugly to be calling a method called "internal" from here. Since you are getting
only enabled extensions, it seems like GetExtensionById() would work too?

http://codereview.chromium.org/165414/diff/1023/1036
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/165414/diff/1023/1036#newcode306
Line 306: ShowExtensionDisabledUI(this, profile_, extension.get());
It is ugly to be showing UI directly from ExtensionsService. We could fire a
notification instead and have the browser windows listen to it. We do a similar
thing for extension crashes.

Then I think we would not need the EXTENSIONS_DISABLED_LOAD notification.

http://codereview.chromium.org/165414/diff/1023/1037
File chrome/browser/extensions/extensions_service.h (right):

http://codereview.chromium.org/165414/diff/1023/1037#newcode47
Line 47: virtual Extension* GetExtensionByIdInternal(const std::string& id,
This won't be needed if you call GetExtensionById() from ExtensionUpdater.

http://codereview.chromium.org/165414/diff/1023/1037#newcode129
Line 129: void EnableExtension(const std::string& extension_id);
Add a TODO for DisableExtension? Will be used by chrome://extensions UI.

http://codereview.chromium.org/165414/diff/1023/1038
File chrome/browser/extensions/extensions_ui.cc (right):

http://codereview.chromium.org/165414/diff/1023/1038#newcode104
Line 104: // Don't show the themes since this page's UI isn't really useful for
Don't copy this comment.

[Matt Perry, 2009-08-13 19:56:29]

http://codereview.chromium.org/165414/diff/1/4
File chrome/browser/extensions/extension_browsertest.cc (right):

http://codereview.chromium.org/165414/diff/1/4#newcode50
Line 50: MessageLoop::current()->PostDelayedTask(FROM_HERE, new
MessageLoop::QuitTask,
On 2009/08/13 19:30:43, Aaron Boodman wrote:
> 80 cols

Done.

http://codereview.chromium.org/165414/diff/1/5
File chrome/browser/extensions/extension_browsertest.h (right):

http://codereview.chromium.org/165414/diff/1/5#newcode25
Line 25: // |expected_change| indicates how many more extensions should be
installed.
On 2009/08/13 19:30:43, Aaron Boodman wrote:
> typo? "how many more"?

Reworded.

http://codereview.chromium.org/165414/diff/1023/1024
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/165414/diff/1023/1024#newcode2069
Line 2069: The newest version of extension "<ph
name="EXTENSION_NAME">$1<ex>Flashblock</ex></ph>" requires more permissions, so
it has been disabled.
On 2009/08/13 19:30:43, Aaron Boodman wrote:
> Nit: "... newest version of **the** extension ...."

Done.

http://codereview.chromium.org/165414/diff/1023/1032
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/165414/diff/1023/1032#newcode43
Line 43: void OnExtensionDisabled(Extension* extension);
On 2009/08/13 19:30:43, Aaron Boodman wrote:
> Since these methods are not called in response to notifications, I think it is
> confusing for them to be prefixed with "On". Simply DisableExtension(), or
> MarkExtensionDisabled() would make more sense to me.
> 
> If you did this to be consistent with OnExtensionInstalled(), maybe add a TODO
> to rename them.

Rejiggered to SetExtensionState to match GetExtensionState.

http://codereview.chromium.org/165414/diff/1023/1032#newcode50
Line 50: // Returns the state of the given extension according to the prefs.
On 2009/08/13 19:30:43, Aaron Boodman wrote:
> Scary comment. Makes me think "who else has an opinion on the state of the
> extension?". Maybe remove the "according to..." part?

Done.

http://codereview.chromium.org/165414/diff/1023/1034
File chrome/browser/extensions/extension_updater.cc (right):

http://codereview.chromium.org/165414/diff/1023/1034#newcode592
Line 592: Extension* extension = service_->GetExtensionByIdInternal(
On 2009/08/13 19:30:43, Aaron Boodman wrote:
> Ugly to be calling a method called "internal" from here. Since you are getting
> only enabled extensions, it seems like GetExtensionById() would work too?

Fixed.  service_ is an interface (for mocking), so I had to add the method to
that interface.

http://codereview.chromium.org/165414/diff/1023/1037
File chrome/browser/extensions/extensions_service.h (right):

http://codereview.chromium.org/165414/diff/1023/1037#newcode129
Line 129: void EnableExtension(const std::string& extension_id);
On 2009/08/13 19:30:43, Aaron Boodman wrote:
> Add a TODO for DisableExtension? Will be used by chrome://extensions UI. 

Done.

http://codereview.chromium.org/165414/diff/1023/1038
File chrome/browser/extensions/extensions_ui.cc (right):

http://codereview.chromium.org/165414/diff/1023/1038#newcode104
Line 104: // Don't show the themes since this page's UI isn't really useful for
On 2009/08/13 19:30:43, Aaron Boodman wrote:
> Don't copy this comment.

Done.

[Aaron Boodman, 2009-08-14 03:21:47]

http://codereview.chromium.org/165414/diff/29/37
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/165414/diff/29/37#newcode248
Line 248: return Extension::DISABLED;
I think it is better to default to ENABLED for these cases.

http://codereview.chromium.org/165414/diff/29/37#newcode296
Line 296: DictionaryValue* dict = prefs_->GetMutableDictionary(kExtensionsPref);
Nit: doesn't seem like you need a mutable dictionary. Don't know if the prefs
service exposes a non-mutable version.

http://codereview.chromium.org/165414/diff/29/41
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/165414/diff/29/41#newcode223
Line 223: disabled_extensions_.erase(iter);
It seems weird to accept ::Unload() for unloaded extensions. Can we leave it to
callers to check this first?

http://codereview.chromium.org/165414/diff/29/41#newcode307
Line 307: ShowExtensionDisabledUI(this, profile_, extension.get());
You didn't do the part about pulling this out to Browser. I feel like that could
be done in this change and would get rid of the need for
EXTENSIONS_DISABLED_LOAD, which is a testing-specific thing.

http://codereview.chromium.org/165414/diff/29/42
File chrome/browser/extensions/extensions_service.h (right):

http://codereview.chromium.org/165414/diff/29/42#newcode47
Line 47: Extension* GetExtensionById(const std::string& id) {
Why can't GetExtensionById() be pure virtual here, and defined on
ExtensionsService?

[Matt Perry, 2009-08-14 20:47:57]

http://codereview.chromium.org/165414/diff/29/37
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/165414/diff/29/37#newcode248
Line 248: return Extension::DISABLED;
On 2009/08/14 03:21:47, Aaron Boodman wrote:
> I think it is better to default to ENABLED for these cases.

Done.

http://codereview.chromium.org/165414/diff/29/37#newcode296
Line 296: DictionaryValue* dict = prefs_->GetMutableDictionary(kExtensionsPref);
On 2009/08/14 03:21:47, Aaron Boodman wrote:
> Nit: doesn't seem like you need a mutable dictionary. Don't know if the prefs
> service exposes a non-mutable version.

Done.

http://codereview.chromium.org/165414/diff/29/41
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/165414/diff/29/41#newcode223
Line 223: disabled_extensions_.erase(iter);
On 2009/08/14 03:21:47, Aaron Boodman wrote:
> It seems weird to accept ::Unload() for unloaded extensions. Can we leave it
to
> callers to check this first?

There are multiple callers (uninstall and load with higher_permissions). I think
it makes more sense to keep the logic in one place.

http://codereview.chromium.org/165414/diff/29/41#newcode307
Line 307: ShowExtensionDisabledUI(this, profile_, extension.get());
On 2009/08/14 03:21:47, Aaron Boodman wrote:
> You didn't do the part about pulling this out to Browser. I feel like that
could
> be done in this change and would get rid of the need for
> EXTENSIONS_DISABLED_LOAD, which is a testing-specific thing.

Oops, I missed that somehow. Done.

http://codereview.chromium.org/165414/diff/29/42
File chrome/browser/extensions/extensions_service.h (right):

http://codereview.chromium.org/165414/diff/29/42#newcode47
Line 47: Extension* GetExtensionById(const std::string& id) {
On 2009/08/14 03:21:47, Aaron Boodman wrote:
> Why can't GetExtensionById() be pure virtual here, and defined on
> ExtensionsService?

Because not everybody calls GetExtensionById(). For it to be effectively mocked,
GetExtensionByIdInternal needs to be the virtual method.

[Aaron Boodman, 2009-08-14 20:56:51]

lgtm, one more nit.

http://codereview.chromium.org/165414/diff/2001/2014
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/165414/diff/2001/2014#newcode305
Line 305: NotificationType::EXTENSION_LOAD_DISABLED,
Nit: I think a clearer name for this would be something like
EXTENSION_UPDATE_DISABLED.