Fixed how Apps Developer Tools is enabled/disabled

Revert dangerous changes of r218112:
- InstallTracker::Oberve() could trigger OnExtensionUninstalled() twice because NOTIFICATION_EXTENSION_UNLOADED notification with reason UNLOAD_REASON_UNINSTALL could be sent along with an additional NOTIFICATION_EXTENSION_UNINSTALLED.
- In ComponentLoader::UnloadComponent(), change reason back to UNLOAD_REASON_DISABLE.

Change how Apps Developer Tools is disabled:
- Use ExtensionService::UninstallExtension() instead of ComponentLoad::Remove() to remove Apps Developer Tools. It makes the codepath consistent with regular extension uninstall.

BUG=278393, 277897
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221329

[dvh, 2013-08-27 20:57:31]

xiyuan@ for chrome/browser/ui/webui/extensions/*
asargent@ for chrome/browser/extensions/*

PTAL. Thanks!

[xiyuan, 2013-08-27 21:43:48]

What happens if user enables develop mode again?

[dvh, 2013-08-27 22:17:55]

On 2013/08/27 21:43:48, xiyuan wrote:
> What happens if user enables develop mode again?

It works properly since the extension doesn't hold any data and it's a component
app.

[dvh, 2013-08-27 22:18:12]

On 2013/08/27 22:17:55, dvh wrote:
> On 2013/08/27 21:43:48, xiyuan wrote:
> > What happens if user enables develop mode again?
> 
> It works properly since the extension doesn't hold any data and it's a
component
> app.

Also I tested manually.

[xiyuan, 2013-08-27 22:43:25]

On 2013/08/27 22:18:12, dvh wrote:
> On 2013/08/27 22:17:55, dvh wrote:
> > On 2013/08/27 21:43:48, xiyuan wrote:
> > > What happens if user enables develop mode again?
> > 
> > It works properly since the extension doesn't hold any data and it's a
> component
> > app.
> 
> Also I tested manually.

The reason I am asking is because I am not sure what does it mean to uninstall a
component extension. It seems that uninstalling code does not touch
ComponentLoader at all so it still has the registration info of the dev
extension. When user enables it again, ComponentLoader::Add is called again. And
since ComponentLoader does not check before it adds the info, the same info is
added again. Think you might need to do something. Maybe keep the
ComponentLoader::Remove call.

[dvh, 2013-08-28 23:06:25]

On 2013/08/27 22:43:25, xiyuan wrote:
> On 2013/08/27 22:18:12, dvh wrote:
> > On 2013/08/27 22:17:55, dvh wrote:
> > > On 2013/08/27 21:43:48, xiyuan wrote:
> > > > What happens if user enables develop mode again?
> > > 
> > > It works properly since the extension doesn't hold any data and it's a
> > component
> > > app.
> > 
> > Also I tested manually.
> 
> The reason I am asking is because I am not sure what does it mean to uninstall
a
> component extension. It seems that uninstalling code does not touch
> ComponentLoader at all so it still has the registration info of the dev
> extension. When user enables it again, ComponentLoader::Add is called again.
And
> since ComponentLoader does not check before it adds the info, the same info is
> added again. Think you might need to do something. Maybe keep the
> ComponentLoader::Remove call.

Added RemoveComponentExtension() to ExtensionServices and used it instead.
PTAL.

[xiyuan, 2013-08-29 00:43:22]

I wonder if we could solve issue 278393 in a different way. We keep what we have
in the extensions/ dir. And we fix the problem in install_tracker.cc and
app_launcher_handler.cc. In install_tracker.cc, instead of firing uninstall
notification on every unload with reason uninstalling, we only fire it if the
extension is a component extension (extension->location() ==
Manifest::COMPONENT) because we know that uninstall notification would never
come for a component extension. And we do similar thing in
app_launcher_handler.cc that make component extension unload w/ reason uninstall
to go through the uninstall path.

[koz (OOO until 15th September), 2013-08-29 01:04:21]

On 2013/08/29 00:43:22, xiyuan wrote:
> I wonder if we could solve issue 278393 in a different way. We keep what we
have
> in the extensions/ dir. And we fix the problem in install_tracker.cc and
> app_launcher_handler.cc. In install_tracker.cc, instead of firing uninstall
> notification on every unload with reason uninstalling, we only fire it if the
> extension is a component extension (extension->location() ==
> Manifest::COMPONENT) because we know that uninstall notification would never
> come for a component extension. And we do similar thing in
> app_launcher_handler.cc that make component extension unload w/ reason
uninstall
> to go through the uninstall path.

+mpcomplete for extension lifetime / notification expertise.

dvh@ the changes you made to InstallTracker are similar to some I made earlier
(see r211328), but that Matt corrected me on. Just adding him to make sure the
new approach checks out.

[Matt Perry, 2013-08-29 19:27:32]

This makes sense. Though I'm not sure what it means to uninstall a component
extension. I thought those were uninstallable.

[dvh, 2013-08-29 20:09:44]

On 2013/08/29 19:27:32, Matt Perry wrote:
> This makes sense. Though I'm not sure what it means to uninstall a component
> extension. I thought those were uninstallable.

I'm listening to suggestions to move forward with this CL.
Thanks!

[not at google - send to devlin, 2013-08-29 21:34:53]

On 2013/08/29 20:09:44, dvh wrote:
> On 2013/08/29 19:27:32, Matt Perry wrote:
> > This makes sense. Though I'm not sure what it means to uninstall a component
> > extension. I thought those were uninstallable.
> 
> I'm listening to suggestions to move forward with this CL.
> Thanks!

Yeah this seems really strange. Can you expose the developer tools in the UI
with the usual extension flow to disable instead?

[dvh, 2013-08-29 21:45:59]

On 2013/08/29 21:34:53, kalman wrote:
> On 2013/08/29 20:09:44, dvh wrote:
> > On 2013/08/29 19:27:32, Matt Perry wrote:
> > > This makes sense. Though I'm not sure what it means to uninstall a
component
> > > extension. I thought those were uninstallable.
> > 
> > I'm listening to suggestions to move forward with this CL.
> > Thanks!
> 
> Yeah this seems really strange. Can you expose the developer tools in the UI
> with the usual extension flow to disable instead?

I think this CL can serve the case of an component app being when the user can
choose whether it's shown in the application list.
It will make component apps behaves properly.

Bottom line is: the current code is broken anyway. This CL is fixing it.
Apps Developer Tools is likely to move to Chrome Web Store.

[asargent_no_longer_on_chrome, 2013-08-30 22:23:00]

lgtm, since this is essentially a revert that restores us back to how the code
worked beofre

I was initially surprised also that our code had the concept of uninstalling a
component extension, but I think it actually does make some sense since we have
ComponentLoader methods that do installation. If you can start up without some
extension and then later install it, you should be able to uninstall it, right?

https://codereview.chromium.org/23636002/diff/10001/chrome/browser/extensions...
File chrome/browser/extensions/install_tracker.cc (left):

https://codereview.chromium.org/23636002/diff/10001/chrome/browser/extensions...
chrome/browser/extensions/install_tracker.cc:106:
OnExtensionUninstalled(extension));
I think removing this code block is definitely correct - we should not be
generating additional uninstall events here; instead that should come from the
case in this switch statement matching NOTIFICATION_EXTENSION_UNINSTALLED. 

(ie at a higher level, when something's getting uninstalled they should first
fire a NOTIFICATION_EXTENSION_UNLOADED notification followed by a
NOTIFICATION_EXTENSION_UNINSTALLED one)

[dvh, 2013-08-31 00:05:18]

On 2013/08/30 22:23:00, Antony Sargent wrote:
> lgtm, since this is essentially a revert that restores us back to how the code
> worked beofre
> 
> I was initially surprised also that our code had the concept of uninstalling a
> component extension, but I think it actually does make some sense since we
have
> ComponentLoader methods that do installation. If you can start up without some
> extension and then later install it, you should be able to uninstall it,
right?
> 
>
https://codereview.chromium.org/23636002/diff/10001/chrome/browser/extensions...
> File chrome/browser/extensions/install_tracker.cc (left):
> 
>
https://codereview.chromium.org/23636002/diff/10001/chrome/browser/extensions...
> chrome/browser/extensions/install_tracker.cc:106:
> OnExtensionUninstalled(extension));
> I think removing this code block is definitely correct - we should not be
> generating additional uninstall events here; instead that should come from the
> case in this switch statement matching NOTIFICATION_EXTENSION_UNINSTALLED. 
> 
> (ie at a higher level, when something's getting uninstalled they should first
> fire a NOTIFICATION_EXTENSION_UNLOADED notification followed by a
> NOTIFICATION_EXTENSION_UNINSTALLED one)

kalman@:
I think we can discuss about the UI in a separate discussion.

kalman@, xiyuan@, mpcomplete@ and koz@:
Let me know if you have additional comments.
Thanks!

[xiyuan, 2013-08-31 00:42:27]

On 2013/08/31 00:05:18, dvh wrote:
> On 2013/08/30 22:23:00, Antony Sargent wrote:
> > lgtm, since this is essentially a revert that restores us back to how the
code
> > worked beofre
> > 
> > I was initially surprised also that our code had the concept of uninstalling
a
> > component extension, but I think it actually does make some sense since we
> have
> > ComponentLoader methods that do installation. If you can start up without
some
> > extension and then later install it, you should be able to uninstall it,
> right?
> > 
> >
>
https://codereview.chromium.org/23636002/diff/10001/chrome/browser/extensions...
> > File chrome/browser/extensions/install_tracker.cc (left):
> > 
> >
>
https://codereview.chromium.org/23636002/diff/10001/chrome/browser/extensions...
> > chrome/browser/extensions/install_tracker.cc:106:
> > OnExtensionUninstalled(extension));
> > I think removing this code block is definitely correct - we should not be
> > generating additional uninstall events here; instead that should come from
the
> > case in this switch statement matching NOTIFICATION_EXTENSION_UNINSTALLED. 
> > 
> > (ie at a higher level, when something's getting uninstalled they should
first
> > fire a NOTIFICATION_EXTENSION_UNLOADED notification followed by a
> > NOTIFICATION_EXTENSION_UNINSTALLED one)
> 
> kalman@:
> I think we can discuss about the UI in a separate discussion.
> 
> kalman@, xiyuan@, mpcomplete@ and koz@:
> Let me know if you have additional comments.
> Thanks!

My major concern was that the CL is not inline with the views of extension
owners. Since Antony blessed the CL, the concern is gone. So I am all good.

[Matt Perry, 2013-08-31 00:56:02]

also lgtm

[commit-bot: I haz the power, 2013-09-03 17:18:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dvh@chromium.org/23636002/10001

[commit-bot: I haz the power, 2013-09-03 17:46:23]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-03 23:19:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dvh@chromium.org/23636002/42001

[commit-bot: I haz the power, 2013-09-04 00:18:04]

Retried try job too often on linux_aura for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-09-04 05:49:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dvh@chromium.org/23636002/60001

[commit-bot: I haz the power, 2013-09-04 17:49:59]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-09-05 01:45:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dvh@chromium.org/23636002/60001

[commit-bot: I haz the power, 2013-09-05 02:09:51]

Change committed as 221329