[Media Router] Unload the legacy Cast extension at install

[Media Router] Unload the legacy Cast extension at install

This change makes ComponentToolbarActionsFactory unload the Cast extension when
the toolbar model becomes ready or the extension gets installed. Previously, we
were unloading the extension when the toolbar became ready (only if the
extension was enabled), but not if it was installed afterwards, which could
happen because of profile sync.

BUG=691575

[takumif, 2017-04-06 03:41:55]

Description was changed from

==========
Uninstall the legacy Cast extension

BUG=
==========

to

==========
[Media Router] Uninstall the legacy Cast extension

This change makes ComponentToolbarActionsFactory uninstall the Cast extension if
it's installed when the toolbar model becomes ready, or when it gets installed.
Previously, the extension was getting unloaded.

BUG=691575
==========

[takumif, 2017-04-06 03:45:26]

Description was changed from

==========
[Media Router] Uninstall the legacy Cast extension

This change makes ComponentToolbarActionsFactory uninstall the Cast extension if
it's installed when the toolbar model becomes ready, or when it gets installed.
Previously, the extension was getting unloaded.

BUG=691575
==========

to

==========
[Media Router] Uninstall the legacy Cast extension

This change makes ComponentToolbarActionsFactory uninstall the Cast extension if
it's installed when the toolbar model becomes ready, or when it gets installed.
Previously, the extension was getting unloaded.

BUG=691575
==========

[takumif, 2017-04-06 03:58:19]

Description was changed from

==========
[Media Router] Uninstall the legacy Cast extension

This change makes ComponentToolbarActionsFactory uninstall the Cast extension if
it's installed when the toolbar model becomes ready, or when it gets installed.
Previously, the extension was getting unloaded.

BUG=691575
==========

to

==========
[Media Router] Uninstall the legacy Cast extension

This change makes ComponentToolbarActionsFactory uninstall the Cast extension if
it's installed when the toolbar model becomes ready, or when it gets installed.
Previously, we were unloading the extension when the toolbar became ready, but
not when it was installed afterwards, which can happen because of profile sync.

BUG=691575
==========

[takumif, 2017-04-06 03:58:30]

Description was changed from

==========
[Media Router] Uninstall the legacy Cast extension

This change makes ComponentToolbarActionsFactory uninstall the Cast extension if
it's installed when the toolbar model becomes ready, or when it gets installed.
Previously, we were unloading the extension when the toolbar became ready, but
not when it was installed afterwards, which can happen because of profile sync.

BUG=691575
==========

to

==========
[Media Router] Uninstall the legacy Cast extension

This change makes ComponentToolbarActionsFactory uninstall the Cast extension if
it's installed when the toolbar model becomes ready, or when it gets installed.
Previously, we were unloading the extension when the toolbar became ready, but
not when it was installed afterwards, which can happen because of profile sync.

BUG=691575
==========

[takumif, 2017-04-06 04:00:10]

takumif@chromium.org changed reviewers:
+ imcheng@chromium.org, rdevlin.cronin@chromium.org

[takumif, 2017-04-06 04:00:11]

Please take a look.
I couldn't tell if it's safe to call ExtensionService::UninstallExtension() from
ExtensionRegistryObserver::OnExtensionInstalled(), but I found a precedence [1].
Please let me know if there's a better way. Thanks!

[1]
https://cs.chromium.org/chromium/src/chrome/browser/search/hotword_service.cc...

[Devlin, 2017-04-06 21:34:40]

On 2017/04/06 04:00:11, takumif wrote:
> Please take a look.
> I couldn't tell if it's safe to call ExtensionService::UninstallExtension()
from
> ExtensionRegistryObserver::OnExtensionInstalled(), but I found a precedence
[1].
> Please let me know if there's a better way. Thanks!
> 
> [1]
>
https://cs.chromium.org/chromium/src/chrome/browser/search/hotword_service.cc...

Hmm...

Two thoughts.
- This does seem like an odd place for the uninstallation.  If we go this route,
I'd prefer to put this in ExtensionService, sometime after initialization.
- Usually, we don't uninstall non-component extensions directly in chrome, but
rather through the extension itself (using the management.uninstallSelf method).
 Is there a strong reason to uninstall this non-component extension through
chrome itself?

[takumif, 2017-04-07 17:57:17]

Description was changed from

==========
[Media Router] Uninstall the legacy Cast extension

This change makes ComponentToolbarActionsFactory uninstall the Cast extension if
it's installed when the toolbar model becomes ready, or when it gets installed.
Previously, we were unloading the extension when the toolbar became ready, but
not when it was installed afterwards, which can happen because of profile sync.

BUG=691575
==========

to

==========
[Media Router] Unload the legacy Cast extension at install

This change makes ComponentToolbarActionsFactory unload the Cast extension when
it gets installed. Previously, we were unloading the extension when the toolbar
became ready, but not if it was installed afterwards, which could happen because
of profile sync.

BUG=691575
==========

[takumif, 2017-04-07 17:57:37]

Description was changed from

==========
[Media Router] Unload the legacy Cast extension at install

This change makes ComponentToolbarActionsFactory unload the Cast extension when
it gets installed. Previously, we were unloading the extension when the toolbar
became ready, but not if it was installed afterwards, which could happen because
of profile sync.

BUG=691575
==========

to

==========
[Media Router] Unload the legacy Cast extension at install

This change makes ComponentToolbarActionsFactory unload the Cast extension when
it gets installed. Previously, we were unloading the extension when the toolbar
became ready, but not if it was installed afterwards, which could happen because
of profile sync.

BUG=691575
==========

[takumif, 2017-04-07 18:05:54]

Description was changed from

==========
[Media Router] Unload the legacy Cast extension at install

This change makes ComponentToolbarActionsFactory unload the Cast extension when
it gets installed. Previously, we were unloading the extension when the toolbar
became ready, but not if it was installed afterwards, which could happen because
of profile sync.

BUG=691575
==========

to

==========
[Media Router] Unload the legacy Cast extension at install

This change makes ComponentToolbarActionsFactory unload the Cast extension when
the toolbar model becomes ready or the extension gets installed. Previously, we
were unloading the extension when the toolbar became ready (only if the
extension was enabled), but not if it was installed afterwards, which could
happen because of profile sync.

BUG=691575
==========

[takumif, 2017-04-07 18:06:16]

Patchset #3 (id:40001) has been deleted

[takumif, 2017-04-07 18:22:25]

On 2017/04/06 21:34:40, Devlin wrote:
> On 2017/04/06 04:00:11, takumif wrote:
> > Please take a look.
> > I couldn't tell if it's safe to call ExtensionService::UninstallExtension()
> from
> > ExtensionRegistryObserver::OnExtensionInstalled(), but I found a precedence
> [1].
> > Please let me know if there's a better way. Thanks!
> > 
> > [1]
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/search/hotword_service.cc...
> 
> Hmm...
> 
> Two thoughts.
> - This does seem like an odd place for the uninstallation.  If we go this
route,
> I'd prefer to put this in ExtensionService, sometime after initialization.
> - Usually, we don't uninstall non-component extensions directly in chrome, but
> rather through the extension itself (using the management.uninstallSelf
method).
>  Is there a strong reason to uninstall this non-component extension through
> chrome itself?

We think we can make the extension uninstall itself, so we'll try doing that.
We've already removed the extension from the codebase so we'll need to put it
back (at least the necessary parts) first.
In the meantime we'd like to fix the bug by unloading the extension at startup
and installation of the extension. We're basically making the
ComponentToolbarActionsFactory do what the ComponentMigrationHelper [1] used to
do. Please take a look, thanks.


[1]
https://codereview.chromium.org/2678083005/diff/140001/chrome/browser/extensi...

[Devlin, 2017-04-10 22:48:00]

On 2017/04/07 18:22:25, takumif wrote:
> On 2017/04/06 21:34:40, Devlin wrote:
> > On 2017/04/06 04:00:11, takumif wrote:
> > > Please take a look.
> > > I couldn't tell if it's safe to call
ExtensionService::UninstallExtension()
> > from
> > > ExtensionRegistryObserver::OnExtensionInstalled(), but I found a
precedence
> > [1].
> > > Please let me know if there's a better way. Thanks!
> > > 
> > > [1]
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/search/hotword_service.cc...
> > 
> > Hmm...
> > 
> > Two thoughts.
> > - This does seem like an odd place for the uninstallation.  If we go this
> route,
> > I'd prefer to put this in ExtensionService, sometime after initialization.
> > - Usually, we don't uninstall non-component extensions directly in chrome,
but
> > rather through the extension itself (using the management.uninstallSelf
> method).
> >  Is there a strong reason to uninstall this non-component extension through
> > chrome itself?
> 
> We think we can make the extension uninstall itself, so we'll try doing that.
> We've already removed the extension from the codebase so we'll need to put it
> back (at least the necessary parts) first.
> In the meantime we'd like to fix the bug by unloading the extension at startup
> and installation of the extension. We're basically making the
> ComponentToolbarActionsFactory do what the ComponentMigrationHelper [1] used
to
> do. Please take a look, thanks.
> 
> 
> [1]
>
https://codereview.chromium.org/2678083005/diff/140001/chrome/browser/extensi...

Hmm... this is a bit complicated.  If we always unload the extension here, then
the extension will never be able to uninstall itself (because it'll never be
running).

Pushing a change to the extension should be pretty quick (actually quicker than
waiting for a fix to percolate through the chrome release cycle); is there a
reason that approach isn't preferred?

Otherwise, we'll probably have to go with the approach of uninstalling here,
though that's not necessarily my preference (and in which case my initial
comment that this is better in a more central location like ExtensionService
will still stand).