Implement extension specific events

EFD now notifies EPM of renderviews created, which in turn notifies the renderer of page actions that it knows about.

Remove generic event "page-action-executed" in favor of page action specific event (sent as extension_id/page_action_id).

In the bindings, we now setup events for each page action we know about so we can register for specific events, and not receive broadcast events from all page actions. To setup these events I added a GetCurrentPageActions() to extension_process_bindings.cc and a helper function GetCurrentExtensionId().

And, finally, I simplified the page action background page by removing the check to see if we are already subscribed to the feed (since we now support multiple feed readers, it doesn't make sense anymore to always check Google Reader). This check might make a comeback later in a different form.

BUG=13936
TEST=The RSS sample extension should work as before.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=20714

[Matt Perry, 2009-07-14 19:12:20]

http://codereview.chromium.org/155514/diff/1006/1013
File chrome/browser/extensions/extension_function_dispatcher.cc (right):

http://codereview.chromium.org/155514/diff/1006/1013#newcode195
Line 195: epm->OnExtensionViewCreated(extension_id(),
This name is misleading.  The EFD doesn't necessarily correspond to an
"ExtensionView", or even an "ExtensionHost".  This could just be an extension
URL loaded in a tab.  How about just RegisterExtensionProcess, or something like
that?

http://codereview.chromium.org/155514/diff/1006/1015
File chrome/browser/extensions/extension_process_manager.cc (right):

http://codereview.chromium.org/155514/diff/1006/1015#newcode93
Line 93: if (i->second == process_id) {
Why do you need this for loop at all?  It looks like you're searching for
extension_id, but you already have it.

Also, this will update page actions for every tab, toolstrip, and bg page
created for this extension.  Maybe you should only send this if extension_id was
not already in the process_ids_ map?

http://codereview.chromium.org/155514/diff/1006/1015#newcode111
Line 111: if (process_ids_.find(extension_id) == process_ids_.end())
Should this be a DCHECK?

http://codereview.chromium.org/155514/diff/1006/1021
File chrome/renderer/extensions/extension_process_bindings.cc (right):

http://codereview.chromium.org/155514/diff/1006/1021#newcode115
Line 115: const v8::Arguments& args) {
indent += 2

http://codereview.chromium.org/155514/diff/1006/1022
File chrome/renderer/extensions/extension_process_bindings.h (right):

http://codereview.chromium.org/155514/diff/1006/1022#newcode31
Line 31: typedef std::map< std::string, std::vector<std::string> >
PageActionIdMap;
This doesn't need to be in the header.

http://codereview.chromium.org/155514/diff/1006/1019
File chrome/renderer/renderer_resources.grd (right):

http://codereview.chromium.org/155514/diff/1006/1019#newcode3
Line 3: without changes to the corresponding grd file. mp2 -->
hey, you're not mp!

http://codereview.chromium.org/155514/diff/1006/1024
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/155514/diff/1006/1024#newcode403
Line 403: chrome.pageActions.setupEventListeners = function() {
this method probably shouldn't be part of the public API

http://codereview.chromium.org/155514/diff/1006/1024#newcode404
Line 404: var extension_id = GetCurrentExtensionId();
use javascriptStyle

http://codereview.chromium.org/155514/diff/1006/1024#newcode561
Line 561: chrome.pageActions.setupEventListeners();
We have the extensionId passed into this function.  If you pass it along, you
won't need to add the GetCurrentExtensionId() native.

[Finnur, 2009-07-14 21:09:26]

Please take another look.

http://codereview.chromium.org/155514/diff/1006/1019
File chrome/renderer/renderer_resources.grd (right):

http://codereview.chromium.org/155514/diff/1006/1019#newcode3
Line 3: without changes to the corresponding grd file. mp2 -->
Dang! My cover is blown...
On 2009/07/14 19:12:20, Matt Perry wrote:
> hey, you're not mp!

[Matt Perry, 2009-07-14 21:17:48]

LGTM, with a handful of nits.

http://codereview.chromium.org/155514/diff/60/1066
File chrome/browser/extensions/extension_function_dispatcher.cc (right):

http://codereview.chromium.org/155514/diff/60/1066#newcode196
Line 196: render_view_host->process()->pid());
indent += 2

http://codereview.chromium.org/155514/diff/60/1074
File chrome/renderer/extensions/extension_process_bindings.cc (right):

http://codereview.chromium.org/155514/diff/60/1074#newcode119
Line 119: GetPageActionMap();
nit: no need to wrap this

http://codereview.chromium.org/155514/diff/60/1074#newcode123
Line 123: std::vector<std::string> page_actions;
This creates a copy of the vector.  Use a pointer a pointer instead.

http://codereview.chromium.org/155514/diff/60/1074#newcode130
Line 130: v8::Local<v8::Array> page_action_vector = v8::Array::New(size);
nit: "vector" is lying - it's an array

http://codereview.chromium.org/155514/diff/60/1074#newcode132
Line 132: std::string page_action_id = page_actions[i];
nit: wasteful copy. use const ref

http://codereview.chromium.org/155514/diff/60/1072
File chrome/renderer/renderer_resources.grd (right):

http://codereview.chromium.org/155514/diff/60/1072#newcode3
Line 3: without changes to the corresponding grd file. Zorglub1 -->
:)

[Aaron Boodman, 2009-07-14 21:28:52]

http://codereview.chromium.org/155514/diff/60/1077
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/155514/diff/60/1077#newcode402
Line 402: function setupEventListeners(extensionId) {
Rename this to something like setupPageActionEvents?

http://codereview.chromium.org/155514/diff/60/1077#newcode405
Line 405: var eventName = extensionId + "/" + pageActions[i];
Nit: there's no block scope in JavaScript so this variable is really defined at
the same level as pageActions. Therefore the style is usually to declare it
there.