Destroy configurations created by VPN extension on disable

Destroy configurations created by VPN extension on disable

The CL destroys VPN configurations created by an extension when
the extension is disabled.

BUG=468477
Committed: https://crrev.com/b92b69ac6bbae38ad5af4a969d6f189349f399ce
Cr-Commit-Position: refs/heads/master@{#321984}

[kaliamoorthi, 2015-03-23 17:44:06]

kaliamoorthi@chromium.org changed reviewers:
+ pneubeck@chromium.org

[kaliamoorthi, 2015-03-23 17:44:06]

PTAL

[pneubeck (no reviews), 2015-03-24 08:55:26]

https://codereview.chromium.org/1028213002/diff/1/extensions/browser/api/vpn_...
File extensions/browser/api/vpn_provider/vpn_service.cc (right):

https://codereview.chromium.org/1028213002/diff/1/extensions/browser/api/vpn_...
extensions/browser/api/vpn_provider/vpn_service.cc:479: if
(extension_registry_->disabled_extensions().Contains(extension->id())) {
The documentation at 
ExtensionRegistryObserver::OnExtensionUnloaded
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

is wrong. The guarantee of that function is that the extension will not be
enabled anymore.
That should be a sufficient condition to DestroyConfigurations...

I'd just call Destroy... unconditionally.

https://codereview.chromium.org/1028213002/diff/1/extensions/browser/api/vpn_...
extensions/browser/api/vpn_provider/vpn_service.cc:481: }
nit: remove {}

[pneubeck (no reviews), 2015-03-24 09:13:48]

On 2015/03/24 08:55:26, pneubeck wrote:
>
https://codereview.chromium.org/1028213002/diff/1/extensions/browser/api/vpn_...
> File extensions/browser/api/vpn_provider/vpn_service.cc (right):
> 
>
https://codereview.chromium.org/1028213002/diff/1/extensions/browser/api/vpn_...
> extensions/browser/api/vpn_provider/vpn_service.cc:479: if
> (extension_registry_->disabled_extensions().Contains(extension->id())) {
> The documentation at 
> ExtensionRegistryObserver::OnExtensionUnloaded
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
> 
> is wrong. The guarantee of that function is that the extension will not be
> enabled anymore.
> That should be a sufficient condition to DestroyConfigurations...
> 
> I'd just call Destroy... unconditionally.
> 
>
https://codereview.chromium.org/1028213002/diff/1/extensions/browser/api/vpn_...
> extensions/browser/api/vpn_provider/vpn_service.cc:481: }
> nit: remove {}

Ok, as discussed you should handle all persistent disables, i.e. Disable and
Blacklist.

[kaliamoorthi, 2015-03-24 10:14:46]

PTAL

https://codereview.chromium.org/1028213002/diff/1/extensions/browser/api/vpn_...
File extensions/browser/api/vpn_provider/vpn_service.cc (right):

https://codereview.chromium.org/1028213002/diff/1/extensions/browser/api/vpn_...
extensions/browser/api/vpn_provider/vpn_service.cc:479: if
(extension_registry_->disabled_extensions().Contains(extension->id())) {
On 2015/03/24 08:55:25, pneubeck wrote:
> The documentation at 
> ExtensionRegistryObserver::OnExtensionUnloaded
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
> 
> is wrong. The guarantee of that function is that the extension will not be
> enabled anymore.
> That should be a sufficient condition to DestroyConfigurations...
> 
> I'd just call Destroy... unconditionally.

We discussed this.

https://codereview.chromium.org/1028213002/diff/1/extensions/browser/api/vpn_...
extensions/browser/api/vpn_provider/vpn_service.cc:481: }
On 2015/03/24 08:55:25, pneubeck wrote:
> nit: remove {}

This is not relevant anymore.

[pneubeck (no reviews), 2015-03-24 12:53:45]

lgtm

[kaliamoorthi, 2015-03-24 12:58:40]

The CQ bit was checked by kaliamoorthi@chromium.org

[kaliamoorthi, 2015-03-24 12:58:54]

On 2015/03/24 12:53:45, pneubeck wrote:
> lgtm

Thanks Philipp.

[commit-bot: I haz the power, 2015-03-24 12:59:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1028213002/20001

[commit-bot: I haz the power, 2015-03-24 13:03:14]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-03-24 13:04:03]

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