Hook up runtime API to implement ExtensionRegistryObserver.

Hook up runtime API to implement ExtensionRegistryObserver.

* Also open the extension "uninstall URL" only when uninstall is user initiated.

BUG=84556
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285079

[rpaquay, 2014-07-21 19:46:47]

This is the fix to show an extension's uninstall URL only when the uninstall
action is user initiated.

[Devlin, 2014-07-22 00:26:59]

https://codereview.chromium.org/406063002/diff/20001/extensions/browser/api/r...
File extensions/browser/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/406063002/diff/20001/extensions/browser/api/r...
extensions/browser/api/runtime/runtime_api.cc:164: switch (type) {
Let's make this DCHECK_EQ(chrome::NOTIFICATION_EXTENSIONS_READY, type) and then
do the body of OnExtensionsReady() in here.

https://codereview.chromium.org/406063002/diff/20001/extensions/browser/api/r...
extensions/browser/api/runtime/runtime_api.cc:177: OnExtensionLoaded(extension);
Let's put the OnExtensionLoaded method body in here, since it's only used here
(same for WillBeInstalled and Uninstalled)

https://codereview.chromium.org/406063002/diff/20001/extensions/browser/api/r...
extensions/browser/api/runtime/runtime_api.cc:417: (reason ==
UNINSTALL_REASON_MANAGEMENT_API);
I think it's readable enough to inline this in the if statement.

[rpaquay, 2014-07-22 16:34:21]

PTAL

https://codereview.chromium.org/406063002/diff/20001/extensions/browser/api/r...
File extensions/browser/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/406063002/diff/20001/extensions/browser/api/r...
extensions/browser/api/runtime/runtime_api.cc:164: switch (type) {
On 2014/07/22 00:26:58, Devlin wrote:
> Let's make this DCHECK_EQ(chrome::NOTIFICATION_EXTENSIONS_READY, type) and
then
> do the body of OnExtensionsReady() in here.

Done.

https://codereview.chromium.org/406063002/diff/20001/extensions/browser/api/r...
extensions/browser/api/runtime/runtime_api.cc:177: OnExtensionLoaded(extension);
On 2014/07/22 00:26:59, Devlin wrote:
> Let's put the OnExtensionLoaded method body in here, since it's only used here
> (same for WillBeInstalled and Uninstalled)

Done.

https://codereview.chromium.org/406063002/diff/20001/extensions/browser/api/r...
extensions/browser/api/runtime/runtime_api.cc:417: (reason ==
UNINSTALL_REASON_MANAGEMENT_API);
On 2014/07/22 00:26:59, Devlin wrote:
> I think it's readable enough to inline this in the if statement.

Done.

[Devlin, 2014-07-22 22:21:12]

https://codereview.chromium.org/406063002/diff/20001/extensions/browser/api/r...
File extensions/browser/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/406063002/diff/20001/extensions/browser/api/r...
extensions/browser/api/runtime/runtime_api.cc:164: switch (type) {
On 2014/07/22 16:34:21, rpaquay wrote:
> On 2014/07/22 00:26:58, Devlin wrote:
> > Let's make this DCHECK_EQ(chrome::NOTIFICATION_EXTENSIONS_READY, type) and
> then
> > do the body of OnExtensionsReady() in here.
> 
> Done.

Sorry, what I meant by this was to do:
DCHECK_EQ(...)
// We're done restarting Chrome after an update.
dispatch_chrome_updated_event_ = false;
....

and then delete the OnExtensionsReady() function.

https://codereview.chromium.org/406063002/diff/40001/extensions/browser/api/r...
File extensions/browser/api/runtime/runtime_api.h (right):

https://codereview.chromium.org/406063002/diff/40001/extensions/browser/api/r...
extensions/browser/api/runtime/runtime_api.h:81: content::BrowserContext*
browser_context,
let's move this above the BCKAPI impl methods so that they stay in order in the
.cc file.

[rpaquay, 2014-07-23 00:03:59]

https://codereview.chromium.org/406063002/diff/20001/extensions/browser/api/r...
File extensions/browser/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/406063002/diff/20001/extensions/browser/api/r...
extensions/browser/api/runtime/runtime_api.cc:164: switch (type) {
On 2014/07/22 22:21:12, Devlin wrote:
> On 2014/07/22 16:34:21, rpaquay wrote:
> > On 2014/07/22 00:26:58, Devlin wrote:
> > > Let's make this DCHECK_EQ(chrome::NOTIFICATION_EXTENSIONS_READY, type) and
> > then
> > > do the body of OnExtensionsReady() in here.
> > 
> > Done.
> 
> Sorry, what I meant by this was to do:
> DCHECK_EQ(...)
> // We're done restarting Chrome after an update.
> dispatch_chrome_updated_event_ = false;
> ....
> 
> and then delete the OnExtensionsReady() function.

Done.

https://codereview.chromium.org/406063002/diff/40001/extensions/browser/api/r...
File extensions/browser/api/runtime/runtime_api.h (right):

https://codereview.chromium.org/406063002/diff/40001/extensions/browser/api/r...
extensions/browser/api/runtime/runtime_api.h:81: content::BrowserContext*
browser_context,
On 2014/07/22 22:21:12, Devlin wrote:
> let's move this above the BCKAPI impl methods so that they stay in order in
the
> .cc file.

Done.

[Devlin, 2014-07-23 15:26:44]

lgtm!

https://codereview.chromium.org/406063002/diff/60001/extensions/browser/api/r...
File extensions/browser/api/runtime/runtime_api.h (right):

https://codereview.chromium.org/406063002/diff/60001/extensions/browser/api/r...
extensions/browser/api/runtime/runtime_api.h:39: 
nit: Also forward-declare ExtensionRegistry.

[rpaquay, 2014-07-23 15:41:42]

https://codereview.chromium.org/406063002/diff/60001/extensions/browser/api/r...
File extensions/browser/api/runtime/runtime_api.h (right):

https://codereview.chromium.org/406063002/diff/60001/extensions/browser/api/r...
extensions/browser/api/runtime/runtime_api.h:39: 
On 2014/07/23 15:26:44, Devlin wrote:
> nit: Also forward-declare ExtensionRegistry.

Done.

[rpaquay, 2014-07-23 15:41:46]

The CQ bit was checked by rpaquay@chromium.org

[commit-bot: I haz the power, 2014-07-23 15:43:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rpaquay@chromium.org/406063002/80001

[rpaquay, 2014-07-23 17:43:48]

The CQ bit was checked by rpaquay@chromium.org

[commit-bot: I haz the power, 2014-07-23 17:48:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rpaquay@chromium.org/406063002/100001

[commit-bot: I haz the power, 2014-07-24 00:05:49]

Change committed as 285079

[not at google - send to devlin, 2014-07-30 02:35:01]

https://codereview.chromium.org/406063002/diff/100001/extensions/browser/api/...
File extensions/browser/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/406063002/diff/100001/extensions/browser/api/...
extensions/browser/api/runtime/runtime_api.cc:393: reason ==
UNINSTALL_REASON_MANAGEMENT_API)) {
I'm reading through the various uninstall reasons and it seems like
UNINSTALL_REASON_EXTENSION_DISABLED and
UNINSTALL_REASON_STORAGE_THRESHOLD_EXCEEDED should be in here as well?

-
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
-
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

(the former name is rather misleading by the way)

basically anything which is associated with a user action (iff it's already
installed) should trigger the uninstalled handler.

[not at google - send to devlin, 2014-07-30 02:38:02]

also is it possible to test the uninstall-from-sync case and make sure it
doesn't fire the listener?

[not at google - send to devlin, 2014-07-30 02:46:27]

That browser test for the uninstall URL is darn awkward the way it sets up a
whole extension just to check that a URL was opened.

It's also fairly limited. It could do with at least a sync test, but also
testing that various uninstall URLs can't be set, like mailto:, javascript:,
etc.

and that doesn't appear to be implemented at all. No URL validation is
implemented:
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

[not at google - send to devlin, 2014-07-30 02:47:24]

On 2014/07/30 02:46:27, kalman wrote:
> That browser test for the uninstall URL is darn awkward the way it sets up a
> whole extension just to check that a URL was opened.
> 
> It's also fairly limited. It could do with at least a sync test, but also
> testing that various uninstall URLs can't be set, like mailto:, javascript:,
> etc.
> 
> and that doesn't appear to be implemented at all. No URL validation is
> implemented:
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

I looked in the wrong place, it does check is_valid(). But this will include
every possible URL.

https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...