Send an onInstalled event for shared module update

Send an onInstalled event for shared module update

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

[elijahtaylor1, 2014-06-05 22:36:45]

Antony, can you take an early look at this and provide guidance?

I also didn't see much/any test coverage for onInstalled events.  Did I miss
where these are implemented?

https://codereview.chromium.org/317993003/diff/1/extensions/browser/api/runti...
File extensions/browser/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/317993003/diff/1/extensions/browser/api/runti...
extensions/browser/api/runtime/runtime_api.cc:17: #include
"chrome/browser/extensions/shared_module_service.h"
these includes are triggering presubmit errors, even though there is another
"chrome/browser" include above.

I need access to the SharedModuleService to figure out what extensions to send
events, any ideas?

The only thing I can think of is to modify InstalledExtensionInfo to include the
dependency information and plumb that through from extensionservice on install
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...)
to the runtime_api that gets that notification
(https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...)

[asargent_no_longer_on_chrome, 2014-06-06 21:07:01]

+cc mek for the question about where we might have tests for the onInstalled
events.

https://codereview.chromium.org/317993003/diff/1/extensions/browser/api/runti...
File extensions/browser/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/317993003/diff/1/extensions/browser/api/runti...
extensions/browser/api/runtime/runtime_api.cc:17: #include
"chrome/browser/extensions/shared_module_service.h"
On 2014/06/05 22:36:45, elijahtaylor1 wrote:
> these includes are triggering presubmit errors, even though there is another
> "chrome/browser" include above.
> 
> I need access to the SharedModuleService to figure out what extensions to send
> events, any ideas?
> 
> The only thing I can think of is to modify InstalledExtensionInfo to include
the
> dependency information and plumb that through from extensionservice on install
>
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...)
> to the runtime_api that gets that notification
>
(https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...)

The goal is that nothing in src/extensions/ can depend on anything in
src/chrome. It looks like src/extensions/DEPS whitelists
chrome_notification_types.h with a TODO to remove that. 

The right way to approach this is probably to see if we can move
chrome/browser/extensions/shared_module_service.h out of chrome/browser into
extensions/browser. That might mean making it it's own ProfileKeyedService, or
hanging it off the ExtensionSystem. 

If that seems like too big a job to tackle, one possibility would be to just add
an abstract method like "GetDependents" to ExtensionSystem that has a concrete
implementation over in ExtensionSystemImpl in chrome/. 

Another thing to consider: should ExtensionRegistry know about the concept of
shared modules?

https://codereview.chromium.org/317993003/diff/1/extensions/common/api/runtim...
File extensions/common/api/runtime.json (right):

https://codereview.chromium.org/317993003/diff/1/extensions/common/api/runtim...
extensions/common/api/runtime.json:366: "description": "Indicates the ID of the
extension that updated. This is present only if 'reason' is
'shared_module_update'."
suggestion: "Indicates the ID of the extension that updated." -> "Indicates the
ID of the imported shared module extension which updated."

[elijahtaylor1, 2014-06-06 21:20:42]

https://codereview.chromium.org/317993003/diff/1/extensions/browser/api/runti...
File extensions/browser/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/317993003/diff/1/extensions/browser/api/runti...
extensions/browser/api/runtime/runtime_api.cc:17: #include
"chrome/browser/extensions/shared_module_service.h"
On 2014/06/06 21:07:01, Antony Sargent wrote:
> On 2014/06/05 22:36:45, elijahtaylor1 wrote:
> > these includes are triggering presubmit errors, even though there is another
> > "chrome/browser" include above.
> > 
> > I need access to the SharedModuleService to figure out what extensions to
send
> > events, any ideas?
> > 
> > The only thing I can think of is to modify InstalledExtensionInfo to include
> the
> > dependency information and plumb that through from extensionservice on
install
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...)
> > to the runtime_api that gets that notification
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...)
> 
> The goal is that nothing in src/extensions/ can depend on anything in
> src/chrome. It looks like src/extensions/DEPS whitelists
> chrome_notification_types.h with a TODO to remove that. 
> 
> The right way to approach this is probably to see if we can move
> chrome/browser/extensions/shared_module_service.h out of chrome/browser into
> extensions/browser. That might mean making it it's own ProfileKeyedService, or
> hanging it off the ExtensionSystem. 
> 
> If that seems like too big a job to tackle, one possibility would be to just
add
> an abstract method like "GetDependents" to ExtensionSystem that has a concrete
> implementation over in ExtensionSystemImpl in chrome/. 

I think I would prefer to do that short term (with maybe a cleanup task to move
shared_module_service out of chrome/).  If it is any more than that, then I will
have to backburner this for a later date, because I was targetting M37 for this
and don't have much time to spend on this.  It's really a nice-to-have for M37.


Does this sound ok?


> 
> Another thing to consider: should ExtensionRegistry know about the concept of
> shared modules?

I don't have familiarity with ExtensionRegistry except briefly while writing
tests.  Can you explain or point to a doc that explains how this fits in?

[elijahtaylor1, 2014-06-06 22:41:56]

PTAL, I implemented the quick and dirty suggestion

https://codereview.chromium.org/317993003/diff/1/extensions/common/api/runtim...
File extensions/common/api/runtime.json (right):

https://codereview.chromium.org/317993003/diff/1/extensions/common/api/runtim...
extensions/common/api/runtime.json:366: "description": "Indicates the ID of the
extension that updated. This is present only if 'reason' is
'shared_module_update'."
On 2014/06/06 21:07:01, Antony Sargent wrote:
> suggestion: "Indicates the ID of the extension that updated." -> "Indicates
the
> ID of the imported shared module extension which updated."

Done.

[asargent_no_longer_on_chrome, 2014-06-09 18:49:29]

lgtm

https://codereview.chromium.org/317993003/diff/80001/extensions/browser/exten...
File extensions/browser/extension_system.h (right):

https://codereview.chromium.org/317993003/diff/80001/extensions/browser/exten...
extensions/browser/extension_system.h:136: virtual scoped_ptr<const
ExtensionSet> GetDependentExtensions(
nit: it looks like ExtensionSet already only holds "const Extension*" 
refpointers, so I don't think you need to have this return a const ExtensionSet.
(Since an ExtensionSet is just a collection of references to immutable things,
it's weird to pass ownership of this set but prohibit the receiver from
modifying the set)

[elijahtaylor1, 2014-06-09 20:48:35]

https://codereview.chromium.org/317993003/diff/80001/extensions/browser/exten...
File extensions/browser/extension_system.h (right):

https://codereview.chromium.org/317993003/diff/80001/extensions/browser/exten...
extensions/browser/extension_system.h:136: virtual scoped_ptr<const
ExtensionSet> GetDependentExtensions(
On 2014/06/09 18:49:29, Antony Sargent wrote:
> nit: it looks like ExtensionSet already only holds "const Extension*" 
> refpointers, so I don't think you need to have this return a const
ExtensionSet.
> (Since an ExtensionSet is just a collection of references to immutable things,
> it's weird to pass ownership of this set but prohibit the receiver from
> modifying the set)
> 
> 

Done.

[asargent_no_longer_on_chrome, 2014-06-11 17:01:57]

test lgtm

https://codereview.chromium.org/317993003/diff/140001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/shared_module/import_pass/check.js
(right):

https://codereview.chromium.org/317993003/diff/140001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/shared_module/import_pass/check.js:13:
chrome.test.sendMessage('shared_module_updated');
can you also check here that the version of the new module is what was expected?

[elijahtaylor1, 2014-06-11 18:01:00]

https://codereview.chromium.org/317993003/diff/140001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/shared_module/import_pass/check.js
(right):

https://codereview.chromium.org/317993003/diff/140001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/shared_module/import_pass/check.js:13:
chrome.test.sendMessage('shared_module_updated');
On 2014/06/11 17:01:56, Antony Sargent wrote:
> can you also check here that the version of the new module is what was
expected?

Done, also added an ID check

[elijahtaylor1, 2014-06-11 18:01:06]

The CQ bit was checked by elijahtaylor@chromium.org

[commit-bot: I haz the power, 2014-06-11 18:04:55]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/elijahtaylor@chromium.org/317993003/16...

[commit-bot: I haz the power, 2014-06-11 20:39:49]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-11 20:43:58]

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

[commit-bot: I haz the power, 2014-06-11 20:43:59]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[elijahtaylor1, 2014-06-12 16:50:19]

The CQ bit was checked by elijahtaylor@chromium.org

[commit-bot: I haz the power, 2014-06-12 16:51:33]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/elijahtaylor@chromium.org/317993003/16...

[commit-bot: I haz the power, 2014-06-12 18:01:13]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-12 18:13:18]

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

[commit-bot: I haz the power, 2014-06-12 18:13:19]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[elijahtaylor1, 2014-06-12 19:17:27]

The CQ bit was checked by elijahtaylor@chromium.org

[commit-bot: I haz the power, 2014-06-12 19:21:30]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/elijahtaylor@chromium.org/317993003/18...

[commit-bot: I haz the power, 2014-06-13 00:46:47]

Change committed as 276871