Notify users about certain changes in installed extensions.

Notify users about certain changes in installed extensions.

This is an interim checkin. The added code is unreachable.
Remaining:

  - Implement concept of orphaned extensions. Currently hardcoded false.
  - Once GlobalError's view supports DOMView, enhance to allow selective
    disabling of extensions.
  - Add unit test for ExtensionGlobalError (though there isn't much to test).
  - Get real UI layout and text. Currently placeholder text.
  - Bug: extensions page opens in new window, not new tab.
  - Bug: once we've determined that an alert needs to be displayed, the
    alert doesn't actually display until the next window open. We ought
    to add a different trigger. This might be a systemwide decision, so I'd
    like sail's input.

BUG=94494
TEST=enhanced ExtensionPref unit tests.


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

[miket_OOO, 2011-10-07 18:38:03]

asargent: general review.

sail: please confirm proper usage of GlobalError.

jstritar: FYI; just making sure you and I are staying in sync on user
notifications about new extension installation events.

[asargent_no_longer_on_chrome, 2011-10-07 21:25:34]

LGTM with some nits and optional suggestions

http://codereview.chromium.org/8202011/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_global_error.cc (right):

http://codereview.chromium.org/8202011/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_global_error.cc:51: }
Simple getters like the 3 above can be inlined in the .h if you want (doing that
is common but not required, so your call either way).

http://codereview.chromium.org/8202011/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_global_error.cc:101: DCHECK(extensions);
Might as well make this a CHECK instead of DCHECK, since you'll crash if few
lines below anyway in the extensions->begin() call.

http://codereview.chromium.org/8202011/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_global_error.h (right):

http://codereview.chromium.org/8202011/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_global_error.h:21: // |extension_service|
must live longer than us.
FYI, ExtensionService has a WeakPtrFactory member variable (actually it could be
switched to just inherit from SupportsWeakPtr), so if there's a chance that a
ExtensionGlobalError object might actually outlive the ExtensionService (during
shutdown I suppose), you could have this take a base::WeakPtr<ExtensionService>
instead of a bare pointer.

http://codereview.chromium.org/8202011/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/8202011/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_prefs.h:179: bool
IsNotificationSystemInitialized();
nit: "Notification" is becoming an overloaded term in the codebase, since we
already have the notifications sent via content/common/notification_service.h,
and the newly landing "App Notification" stuff that I'm working on. Maybe naming
this something along the lines of "Alert" (as you do in the extension_service.cc
file) would be better.

Also, when I see a method called IsFoo I usually think of it as performing some
work to get the answer, but not making a permanent state change. Consider
renaming to something like 
"InitializeAlertingIfNeeded" or "SetAlertSystemFirstRun".

http://codereview.chromium.org/8202011/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_prefs_unittest.cc (right):

http://codereview.chromium.org/8202011/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_prefs_unittest.cc:520:
TEST_F(ExtensionPrefsAcknowledgment, Acknowledgment) {}
Not necessary for this CL, but it would be nice to eventually see a test for
your "fuse" method behavior here. You could take advantage of the fact that
Verify gets called twice (you might need to add something to keep track of first
versus second call). Perhaps add TODO or something.

http://codereview.chromium.org/8202011/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/8202011/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service.cc:2149:
IdentifyAlertableExtensions();
Here's a thought for you to consider - I think it clearly makes sense to alert
relatively soon after startup for newly installed external extensions, because
it's soon after startup that we scan for and install them. But for blacklisted
and orphaned ones, we'll find out about that after one of the periodic update
checks run by the ExtensionUpdater. At least for blacklisted ones, we disable
the extension immediately, so it might be nice to show the alert about that
right at the time it happens, instead of the next time the user starts up the
browser (which for many people might be days/weeks after this).

http://codereview.chromium.org/8202011/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/8202011/diff/1/chrome/common/pref_names.cc#new...
chrome/common/pref_names.cc:1191: const char
kExtensionNotificationsInitializedPref[] =
nit: include a brief comment here about what this pref means.

[miket_OOO, 2011-10-07 22:38:22]

Patch uploaded, changes made. WeakPtr makes me feel good!

http://codereview.chromium.org/8202011/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_global_error.h (right):

http://codereview.chromium.org/8202011/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_global_error.h:21: // |extension_service|
must live longer than us.
On 2011/10/07 21:25:34, Antony Sargent wrote:
> FYI, ExtensionService has a WeakPtrFactory member variable (actually it could
be
> switched to just inherit from SupportsWeakPtr), so if there's a chance that a
> ExtensionGlobalError object might actually outlive the ExtensionService
(during
> shutdown I suppose), you could have this take a
base::WeakPtr<ExtensionService>
> instead of a bare pointer. 
> 

Cool. Done!

http://codereview.chromium.org/8202011/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/8202011/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_prefs.h:179: bool
IsNotificationSystemInitialized();
Done.

http://codereview.chromium.org/8202011/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/8202011/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service.cc:2149:
IdentifyAlertableExtensions();
Agreed. I have a bad bug that's related, as described in the CL description:
that we don't display the alert at all until the next new window. In conjunction
with resolving when to display the alert, I'll figure out when to generate the
alert events. But not for this CL.

[sail, 2011-10-10 17:23:31]

lgtm

http://codereview.chromium.org/8202011/diff/15/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/8202011/diff/15/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_service.cc:2196:
GlobalErrorServiceFactory::GetForProfile(profile_)->AddGlobalError(
If you'd like to show the error bubble right away then you can just call
global_error->ShowBubbleView() from here.

[commit-bot: I haz the power, 2011-10-10 19:35:04]

CQ is trying the patch. Follow status at
https://chromium-status.appspot.com/cq/miket@chromium.org/8202011/8002

[commit-bot: I haz the power, 2011-10-10 21:09:18]

Change committed as 104782