ChromeLaunchController does proper action when extension unloaded

* Remove deprecated extension notification and use ExtensionRegistry instead.
* When extension unloaded, ChromeLauncherController check the profile, and does action only for the correct profile.

R=jamescook@chromium.org, skuhne@chromium.org
BUG=354046, 411568
TEST=unit_tests

Committed: https://crrev.com/ee8d1232c7c633fdbad14c1d509e5774b7f7556b
Cr-Commit-Position: refs/heads/master@{#295006}

[limasdf, 2014-09-12 17:27:52]

Patchset #3 (id:40001) has been deleted

[limasdf, 2014-09-12 18:03:41]

limasdf@gmail.com changed reviewers:
+ jamescook@chromium.org

[limasdf, 2014-09-12 18:03:55]

Patchset #2 (id:20001) has been deleted

[limasdf, 2014-09-12 18:05:58]

James, PTAL when you have time.

[James Cook, 2014-09-12 20:35:35]

jamescook@chromium.org changed reviewers:
+ skuhne@chromium.org

[James Cook, 2014-09-12 20:35:36]

https://codereview.chromium.org/565213002/diff/60001/chrome/browser/ui/ash/la...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (left):

https://codereview.chromium.org/565213002/diff/60001/chrome/browser/ui/ash/la...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:428:
notification_registrar_.Add(
+skuhne -- I'm not sure about the old code here.

ChromeLauncherController can be used with multiple profiles. The original code
here only monitors the first profile for extension load/unload events. That
seems a little odd -- what if the user switches profiles then uninstalls an
extension/app from the second profile?

Your change observes both the original profile and future profiles. That seems
more correct to me.

https://codereview.chromium.org/565213002/diff/60001/chrome/browser/ui/ash/la...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/565213002/diff/60001/chrome/browser/ui/ash/la...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:2079:
extension_registry_ = extensions::ExtensionRegistry::Get(profile_);
Can you get rid of the extension_registry_ member and just use
ExtensionRegistry::Get(profile_)->AddObserver() and RemoveObserver()? That might
make it a little more clear.

[limasdf, 2014-09-12 20:53:00]

https://codereview.chromium.org/565213002/diff/60001/chrome/browser/ui/ash/la...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/565213002/diff/60001/chrome/browser/ui/ash/la...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:2079:
extension_registry_ = extensions::ExtensionRegistry::Get(profile_);
On 2014/09/12 20:35:35, James Cook wrote:
> Can you get rid of the extension_registry_ member and just use
> ExtensionRegistry::Get(profile_)->AddObserver() and RemoveObserver()? That
might
> make it a little more clear.

Done.

[Mr4D (OOO till 08-26), 2014-09-12 23:12:11]

Good point James!

Yes, there is a bit more work there. Sorry.

https://codereview.chromium.org/565213002/diff/60001/chrome/browser/ui/ash/la...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (left):

https://codereview.chromium.org/565213002/diff/60001/chrome/browser/ui/ash/la...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:428:
notification_registrar_.Add(
A closer look reviles that everything should be as it was - but - it has
actually a problem:

CloseWindowedAppsFromRemovedExtension(id) should really be
CloseWindowedAppsFromRemovedExtension(id, profile).

and only browsers which are matching the id AND the profile should get closed
since each user might have the app installed, but only one does uninstall.

Pinning and unpinning should also only happen if the profile in question is the
current profile.

https://codereview.chromium.org/565213002/diff/60001/chrome/browser/ui/ash/la...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/565213002/diff/60001/chrome/browser/ui/ash/la...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1192: reason ==
UnloadedExtensionInfo::REASON_UNINSTALL) {
You should forward the profile to CloseWindowedAppsFromRemovedExtension so that
only browsers with a matching profile get closed.

https://codereview.chromium.org/565213002/diff/60001/chrome/browser/ui/ash/la...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1196: if
(IsAppPinned(id)) {
DoUnpinAppWithID() should only be called if the passed profile is profile(),
since user a's icons should not get closed when user b's applications are
shutting down due to e.g. uninstall.

The app_icon_loader_ on the other hand is a different topic since it does not
distinguish between users (at least not yet).

I guess that ideal would be to have one loader per user, but there is a comment
above stating that that might be not easy. Since this is not a "frequent and
high resource intense" action I guess that we might get away for the time being
if we could check if any user has the id installed before clearing the image.

[limasdf, 2014-09-13 18:31:02]

PTAL. added code that check profile.

https://codereview.chromium.org/565213002/diff/60001/chrome/browser/ui/ash/la...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/565213002/diff/60001/chrome/browser/ui/ash/la...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1192: reason ==
UnloadedExtensionInfo::REASON_UNINSTALL) {
On 2014/09/12 23:12:11, Mr4D wrote:
> You should forward the profile to CloseWindowedAppsFromRemovedExtension so
that
> only browsers with a matching profile get closed.

Done.

https://codereview.chromium.org/565213002/diff/60001/chrome/browser/ui/ash/la...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1196: if
(IsAppPinned(id)) {
:: DoUnpinAppWithID() should only be called if the passed profile is profile(),

Done.

On 2014/09/12 23:12:11, Mr4D wrote:
> DoUnpinAppWithID() should only be called if the passed profile is profile(),
> since user a's icons should not get closed when user b's applications are
> shutting down due to e.g. uninstall.
> 
> The app_icon_loader_ on the other hand is a different topic since it does not
> distinguish between users (at least not yet).
> 
> I guess that ideal would be to have one loader per user, but there is a
comment
> above stating that that might be not easy. Since this is not a "frequent and
> high resource intense" action I guess that we might get away for the time
being
> if we could check if any user has the id installed before clearing the image.

[limasdf, 2014-09-13 18:42:57]

also updated commit description, please lemme know if you want to correct it.

[Mr4D (OOO till 08-26), 2014-09-13 19:06:51]

lgtm.

Thanks for fixing this!

[limasdf, 2014-09-14 04:16:10]

The CQ bit was checked by limasdf@gmail.com

[limasdf, 2014-09-14 04:16:25]

Thanks much for the review!

[commit-bot: I haz the power, 2014-09-14 04:16:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/565213002/100001

[commit-bot: I haz the power, 2014-09-14 05:29:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-14 05:29:53]

Try jobs failed on following builders:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[James Cook, 2014-09-15 17:01:33]

lgtm

[limasdf, 2014-09-15 17:36:13]

The CQ bit was checked by limasdf@gmail.com

[commit-bot: I haz the power, 2014-09-15 18:02:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/565213002/100001

[limasdf, 2014-09-15 18:12:15]

The CQ bit was unchecked by limasdf@gmail.com

[limasdf, 2014-09-15 18:22:05]

On 2014/09/15 18:02:05, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patchset/565213002/100001

I'll commit after getting the result of Asan test.

[limasdf, 2014-09-16 04:38:55]

The CQ bit was checked by limasdf@gmail.com

[commit-bot: I haz the power, 2014-09-16 04:39:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/565213002/100001

[commit-bot: I haz the power, 2014-09-16 04:43:29]

Committed patchset #4 (id:100001) as c60dc9707b0084df933b419be4f97e9370c64d34

[commit-bot: I haz the power, 2014-09-16 04:44:48]

Patchset 4 (id:??) landed as
https://crrev.com/ee8d1232c7c633fdbad14c1d509e5774b7f7556b
Cr-Commit-Position: refs/heads/master@{#295006}