Change extension unload notification to indicate updates....

Change extension unload notification to indicate updates.

When an extension in being unloaded, some listeners want to know if it's 
because the extension is being updated to a newer version, or disabled. This
changes the details sent to include a reason.

Also this removes the EXTENSION_UNLOADED_DISABLED notification, since only a
small number of places actually cared about the disctinction between 
EXTENSION_UNLOADED and EXTENSION_UNLOADED_DISABLED, and puts that information
into the details as well.

BUG=65510
TEST=Should be covered by existing unit & browser tests.



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

[Erik does not do reviews, 2010-12-23 17:05:17]

I stopped the review partway through because I think my comments might wind up
having you touch a lot.

http://codereview.chromium.org/5968009/diff/25002/chrome/browser/automation/a...
File chrome/browser/automation/automation_extension_tracker.cc (right):

http://codereview.chromium.org/5968009/diff/25002/chrome/browser/automation/a...
chrome/browser/automation/automation_extension_tracker.cc:28: if (type !=
NotificationType::EXTENSION_UNLOADED)
should this be NOTREACHED()?

http://codereview.chromium.org/5968009/diff/25002/chrome/browser/automation/a...
chrome/browser/automation/automation_extension_tracker.cc:32:
Details<UnloadedExtensionInfo>(details)->extension;
extension can be NULL right?  so it looks like a crash can happen here.

http://codereview.chromium.org/5968009/diff/25002/chrome/browser/automation/a...
chrome/browser/automation/automation_extension_tracker.cc:40: if
(!service->GetExtensionById(extension->id(), true))
extension can be NULL right?  so it looks like a crash can happen here.  this
should just change to look at the reason.

http://codereview.chromium.org/5968009/diff/25002/chrome/browser/background_c...
File chrome/browser/background_contents_service.cc (right):

http://codereview.chromium.org/5968009/diff/25002/chrome/browser/background_c...
chrome/browser/background_contents_service.cc:119:
Details<UnloadedExtensionInfo>(details)->extension->id()));
extension can be NULL right?  more reason why we should have extension_id as a
member here.

http://codereview.chromium.org/5968009/diff/25002/chrome/browser/background_m...
File chrome/browser/background_mode_manager.cc (right):

http://codereview.chromium.org/5968009/diff/25002/chrome/browser/background_m...
chrome/browser/background_mode_manager.cc:132:
*Details<UnloadedExtensionInfo>(details)->extension)) {
same NULL comment here since I assume IsBackgroundApp dereferences.

http://codereview.chromium.org/5968009/diff/25002/chrome/common/extensions/ex...
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/5968009/diff/25002/chrome/common/extensions/ex...
chrome/common/extensions/extension.h:738: enum Reason {
random thought: should we have a separate reason for a disable due to privilege
increase?

http://codereview.chromium.org/5968009/diff/25002/chrome/common/extensions/ex...
chrome/common/extensions/extension.h:739: DISABLED,   // The extension was
disabled.
naming nit: it would be nice if you used the same tense for these.  it's weird
that you have three different values with three different tenses.

http://codereview.chromium.org/5968009/diff/25002/chrome/common/extensions/ex...
chrome/common/extensions/extension.h:740: UPDATING,   // The extension is being
updated to a newer version.
Are we going to need to add a subsequent UPDATE_FAILED notification?  I haven't
looked at when we unload due to update to see whether there's a way that can
happen.

http://codereview.chromium.org/5968009/diff/25002/chrome/common/extensions/ex...
chrome/common/extensions/extension.h:749: // Can be NULL in the case of an
already_disabled extension.
seems like there should be an extension id field then

[asargent_no_longer_on_chrome, 2010-12-23 18:53:04]

http://codereview.chromium.org/5968009/diff/25002/chrome/browser/automation/a...
File chrome/browser/automation/automation_extension_tracker.cc (right):

http://codereview.chromium.org/5968009/diff/25002/chrome/browser/automation/a...
chrome/browser/automation/automation_extension_tracker.cc:28: if (type !=
NotificationType::EXTENSION_UNLOADED)
On 2010/12/23 17:05:17, Erik Kay wrote:
> should this be NOTREACHED()?

Done.

http://codereview.chromium.org/5968009/diff/25002/chrome/browser/automation/a...
chrome/browser/automation/automation_extension_tracker.cc:32:
Details<UnloadedExtensionInfo>(details)->extension;
On 2010/12/23 17:05:17, Erik Kay wrote:
> extension can be NULL right?  so it looks like a crash can happen here.

It turns out it can't. See my response to comments on extension.h.

http://codereview.chromium.org/5968009/diff/25002/chrome/browser/automation/a...
chrome/browser/automation/automation_extension_tracker.cc:40: if
(!service->GetExtensionById(extension->id(), true))
On 2010/12/23 17:05:17, Erik Kay wrote:
> extension can be NULL right?  so it looks like a crash can happen here.  this
> should just change to look at the reason.

It turns out it can't.

http://codereview.chromium.org/5968009/diff/25002/chrome/browser/background_c...
File chrome/browser/background_contents_service.cc (right):

http://codereview.chromium.org/5968009/diff/25002/chrome/browser/background_c...
chrome/browser/background_contents_service.cc:119:
Details<UnloadedExtensionInfo>(details)->extension->id()));
On 2010/12/23 17:05:17, Erik Kay wrote:
> extension can be NULL right?  more reason why we should have extension_id as a
> member here.

It turns out it can't.

http://codereview.chromium.org/5968009/diff/25002/chrome/browser/background_m...
File chrome/browser/background_mode_manager.cc (right):

http://codereview.chromium.org/5968009/diff/25002/chrome/browser/background_m...
chrome/browser/background_mode_manager.cc:132:
*Details<UnloadedExtensionInfo>(details)->extension)) {
On 2010/12/23 17:05:17, Erik Kay wrote:
> same NULL comment here since I assume IsBackgroundApp dereferences.

It turns out it can't be NULL.

http://codereview.chromium.org/5968009/diff/25002/chrome/common/extensions/ex...
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/5968009/diff/25002/chrome/common/extensions/ex...
chrome/common/extensions/extension.h:738: enum Reason {
On 2010/12/23 17:05:17, Erik Kay wrote:
> random thought: should we have a separate reason for a disable due to
privilege
> increase?

We already handle this in a sort of unexpected place: in 
ExtensionService::OnExtensionLoaded, we disable the extension and send a
EXTENSION_UPDATE_DISABLED notification. This seems like a potentially larger
refactor, so I've logged crbug.com/67922 to take a look at this separately.

http://codereview.chromium.org/5968009/diff/25002/chrome/common/extensions/ex...
chrome/common/extensions/extension.h:739: DISABLED,   // The extension was
disabled.
On 2010/12/23 17:05:17, Erik Kay wrote:
> naming nit: it would be nice if you used the same tense for these.  it's weird
> that you have three different values with three different tenses.

Good point. Changed to: DISABLE, UPDATE, and UNINSTALL

http://codereview.chromium.org/5968009/diff/25002/chrome/common/extensions/ex...
chrome/common/extensions/extension.h:740: UPDATING,   // The extension is being
updated to a newer version.
On 2010/12/23 17:05:17, Erik Kay wrote:
> Are we going to need to add a subsequent UPDATE_FAILED notification?  I
haven't
> looked at when we unload due to update to see whether there's a way that can
> happen.

I think anyone who's interested could listen for EXTENSION_INSTALL_FAILED.

http://codereview.chromium.org/5968009/diff/25002/chrome/common/extensions/ex...
chrome/common/extensions/extension.h:749: // Can be NULL in the case of an
already_disabled extension.
On 2010/12/23 17:05:17, Erik Kay wrote:
> seems like there should be an extension id field then

I was mistaken that the extension can be NULL (there's an early out in
ExtensionService::UnloadExtension to skip sending the notification in that
case). I've updated the comment.

[Erik does not do reviews, 2010-12-23 19:25:24]

LGTM

http://codereview.chromium.org/5968009/diff/59001/chrome/browser/automation/a...
File chrome/browser/automation/automation_extension_tracker.cc (right):

http://codereview.chromium.org/5968009/diff/59001/chrome/browser/automation/a...
chrome/browser/automation/automation_extension_tracker.cc:39: // Remove this
extension only if it is uninstalled, not just disabled.
this should just look at the reason rather than trying to figure it out by
itself