Initial plumbing for sending events from the browser to extension renderers. ...

Initial plumbing for sending events from the browser to extension renderers. Implement onTabMoved.



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

[rafaelw, 2009-04-15 02:07:05]

First cut at extension events. Please review.

[Aaron Boodman, 2009-04-15 05:02:12]

Nice.

http://codereview.chromium.org/73065/diff/21/31
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/73065/diff/21/31#newcode589
Line 589: ExtensionBrowserEventRouter::GetInstance()->Init();
You should move this into ExtensionsService's intialization so that it only gets
run if extensions are enabled and any are running.

http://codereview.chromium.org/73065/diff/21/30
File chrome/browser/extensions/extension_browser_event_router.cc (right):

http://codereview.chromium.org/73065/diff/21/30#newcode42
Line 42: }
Might be nice to add a NOTREACHED() here just as a sanity check.

http://codereview.chromium.org/73065/diff/21/30#newcode65
Line 65: // onTabMoved send arguments like this (tabId, windowId, fromIndex,
toIndex).
Let's pass a single object instead, with the properties tabId, windowId,
fromIndex, toIndex.

http://codereview.chromium.org/73065/diff/21/26
File chrome/browser/extensions/extension_browser_event_router.h (right):

http://codereview.chromium.org/73065/diff/21/26#newcode18
Line 18: static ExtensionBrowserEventRouter* GetInstance();
Need comments for these public methods.

http://codereview.chromium.org/73065/diff/21/26#newcode23
Line 23: static const std::string kOnTabMoved;
We aren't allowed to have static objects, so this needs to be a char[].

http://codereview.chromium.org/73065/diff/21/28
File chrome/browser/extensions/extension_message_service.cc (right):

http://codereview.chromium.org/73065/diff/21/28#newcode57
Line 57: std::set<int> ExtensionMessageService::unique_process_ids() {
This naming convention -- foo_bar() is only used for really cheap getters. Since
this is creating a new object, I think it should be GetUniqueProcessIds().

http://codereview.chromium.org/73065/diff/21/28#newcode71
Line 71: // TODO(mpcomplete): We need to ensure an extension always ends up in a
single
Check with Matt, but I think this todo might be done and you can remove it as
long as you're in here.

http://codereview.chromium.org/73065/diff/21/29
File chrome/browser/extensions/extension_process_manager.cc (right):

http://codereview.chromium.org/73065/diff/21/29#newcode43
Line 43: const ListValue& data) {
This last param can go on the previous line, to be more like the rest of the
code in this file.

http://codereview.chromium.org/73065/diff/21/33
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/73065/diff/21/33#newcode555
Line 555: std::string /*event_name*/,
add a space on either side of the arg name, to be more like the other code?

http://codereview.chromium.org/73065/diff/21/36
File chrome/renderer/extensions/renderer_extension_bindings.h (right):

http://codereview.chromium.org/73065/diff/21/36#newcode27
Line 27: static void DispatchEvent(const std::string& event_name,
Needs a comment.

http://codereview.chromium.org/73065/diff/21/38
File chrome/renderer/resources/event_bindings.js (right):

http://codereview.chromium.org/73065/diff/21/38#newcode25
Line 25: chromium.Event.dispatchJSON_ = function(name, data) {
To make the intended usage clear, how about changing the second param name to
'args' and updating the comment to explain that it should be an array of
arguments.

http://codereview.chromium.org/73065/diff/21/38#newcode124
Line 124: chromium.onTabMoved = new chromium.Event('tabMoved');
Nit: To be consistent with the way our native functions are named, I think the
name of this event should be "TabMoved". Also, prefer double quotes to be
consistent with the rest of our js.

[jam, 2009-04-15 07:06:24]

http://codereview.chromium.org/73065/diff/21/34
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/73065/diff/21/34#newcode199
Line 199: OnDispatchExtensionEvent)
seeing as there are now a number of extension related messages, what about
creating an ExtensionDispatcher like ResourceDispatcher/AppCacheDispatcher so
that extension related logic is separated from RenderThread?  This would also
make testing easier..

[Aaron Boodman, 2009-04-15 07:13:40]

A few more.

http://codereview.chromium.org/73065/diff/21/38
File chrome/renderer/resources/event_bindings.js (right):

http://codereview.chromium.org/73065/diff/21/38#newcode124
Line 124: chromium.onTabMoved = new chromium.Event('tabMoved');
Also, this should be chromium.tabs.onTabMoved, and I was thinking it would be
created in extension_process_bindings, near the other tabs stuff.

http://codereview.chromium.org/73065/diff/21/38#newcode124
Line 124: chromium.onTabMoved = new chromium.Event('tabMoved');
On 2009/04/15 05:02:13, Aaron Boodman wrote:
> Nit: To be consistent with the way our native functions are named, I think the
> name of this event should be "TabMoved". Also, prefer double quotes to be
> consistent with the rest of our js.

Hm, I see now that matt is using "tab-moved". So I guess go with that.

[brettw, 2009-04-15 15:08:38]

Drive by

http://codereview.chromium.org/73065/diff/21/26
File chrome/browser/extensions/extension_browser_event_router.h (right):

http://codereview.chromium.org/73065/diff/21/26#newcode20
Line 20: ExtensionBrowserEventRouter();
This should be first. Why do you need it to be public when you have GetInstance?
Can you clarify who should use this or make it private?

http://codereview.chromium.org/73065/diff/21/24
File chrome/browser/extensions/extension_message_service.h (right):

http://codereview.chromium.org/73065/diff/21/24#newcode66
Line 66: private:
You lost the sapce before this.

[Matt Perry, 2009-04-15 18:45:25]

http://codereview.chromium.org/73065/diff/21/30
File chrome/browser/extensions/extension_browser_event_router.cc (right):

http://codereview.chromium.org/73065/diff/21/30#newcode33
Line 33: if (type == NotificationType::BROWSER_OPENED) {
this might be a good place for a switch statement.

http://codereview.chromium.org/73065/diff/21/26
File chrome/browser/extensions/extension_browser_event_router.h (right):

http://codereview.chromium.org/73065/diff/21/26#newcode23
Line 23: static const std::string kOnTabMoved;
From what I can tell, this isn't referenced anywhere except in this class.  If
that's true, it should be private.

In that vein, I prefer not to have private static class consts.  I'd put it as a
file-level static in the .cc file, so you don't have to declare them in the
header.  (Just an annoying C++ constraint.)

http://codereview.chromium.org/73065/diff/21/28
File chrome/browser/extensions/extension_message_service.cc (right):

http://codereview.chromium.org/73065/diff/21/28#newcode62
Line 62: for (it = process_ids_.begin(); it != process_ids_.end(); it++) {
This won't work in single-process mode, or in the future when we have process
sharing.  There's no guarantee that a given process ID will appear only once in
this map.

http://codereview.chromium.org/73065/diff/21/28#newcode71
Line 71: // TODO(mpcomplete): We need to ensure an extension always ends up in a
single
On 2009/04/15 05:02:13, Aaron Boodman wrote:
> Check with Matt, but I think this todo might be done and you can remove it as
> long as you're in here.

Yeah, thanks.

http://codereview.chromium.org/73065/diff/21/28#newcode187
Line 187: }
I think gcc will complain about the missing newline at the end of the file.

http://codereview.chromium.org/73065/diff/21/27
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/73065/diff/21/27#newcode217
Line 217: int ExtensionTabUtil::GetTabId(const TabContents& tab_contents) {
fyi, try to define functions in .cc files in the same order you declare them in
the header.

http://codereview.chromium.org/73065/diff/21/23
File chrome/browser/extensions/extension_tabs_module.h (right):

http://codereview.chromium.org/73065/diff/21/23#newcode12
Line 12: class ExtensionTabUtil {
Does anyone use this outside of the .cc file?  If not, no need to put it in the
header.

http://codereview.chromium.org/73065/diff/21/23#newcode14
Line 14: static int GetTabId(const TabContents& tab_contents);
This is one place where the rule of thumb I gave you doesn't apply :).  Even
though this can't be NULL in this case, we never pass TabContents by ref.  Use a
pointer for consistency.

http://codereview.chromium.org/73065/diff/21/33
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/73065/diff/21/33#newcode554
Line 554: IPC_MESSAGE_CONTROL2(ViewMsg_Extension_DispatchEvent,
nit: I'd name this ViewMsg_ExtensionHandleEvent to be more like the *Connect and
*Message events.

http://codereview.chromium.org/73065/diff/21/36
File chrome/renderer/extensions/renderer_extension_bindings.h (right):

http://codereview.chromium.org/73065/diff/21/36#newcode27
Line 27: static void DispatchEvent(const std::string& event_name,
same nit as the IPC message

http://codereview.chromium.org/73065/diff/21/34
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/73065/diff/21/34#newcode169
Line 169: void RenderThread::OnDispatchExtensionEvent(const std::string
event_name,
nit: move this down next to OnExtensionHandleMessage, and change the name to be
consistent with that.

http://codereview.chromium.org/73065/diff/21/34#newcode199
Line 199: OnDispatchExtensionEvent)
On 2009/04/15 07:06:24, John Abd-El-Malek wrote:
> seeing as there are now a number of extension related messages, what about
> creating an ExtensionDispatcher like ResourceDispatcher/AppCacheDispatcher so
> that extension related logic is separated from RenderThread?  This would also
> make testing easier..

That sounds reasonable.  Rafael, add a TODO next to these extension messages
about that.

http://codereview.chromium.org/73065/diff/21/38
File chrome/renderer/resources/event_bindings.js (right):

http://codereview.chromium.org/73065/diff/21/38#newcode30
Line 30: chromium.Event.attached_[name].dispatch.apply(
I think data should be a single JSON object, not an array of arguments. 
Thoughts?

http://codereview.chromium.org/73065/diff/21/38#newcode39
Line 39: chromium.Event.attached_[name], data);
Ditto for this.  If we do decide dispatch_ should take an array, you need to fix
dispatchOnConnect_ in renderer_extension_bindings.js which calls it with 1 arg.

[rafaelw, 2009-04-15 23:23:42]

http://codereview.chromium.org/73065/diff/21/31
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/73065/diff/21/31#newcode589
Line 589: ExtensionBrowserEventRouter::GetInstance()->Init();
On 2009/04/15 05:02:13, Aaron Boodman wrote:
> You should move this into ExtensionsService's intialization so that it only
gets
> run if extensions are enabled and any are running.

Done.

http://codereview.chromium.org/73065/diff/21/30
File chrome/browser/extensions/extension_browser_event_router.cc (right):

http://codereview.chromium.org/73065/diff/21/30#newcode33
Line 33: if (type == NotificationType::BROWSER_OPENED) {
On 2009/04/15 18:45:25, Matt Perry wrote:
> this might be a good place for a switch statement.

Done.

http://codereview.chromium.org/73065/diff/21/30#newcode42
Line 42: }
On 2009/04/15 05:02:13, Aaron Boodman wrote:
> Might be nice to add a NOTREACHED() here just as a sanity check.

Done.

http://codereview.chromium.org/73065/diff/21/30#newcode65
Line 65: // onTabMoved send arguments like this (tabId, windowId, fromIndex,
toIndex).
On 2009/04/15 05:02:13, Aaron Boodman wrote:
> Let's pass a single object instead, with the properties tabId, windowId,
> fromIndex, toIndex.

Done.

http://codereview.chromium.org/73065/diff/21/26
File chrome/browser/extensions/extension_browser_event_router.h (right):

http://codereview.chromium.org/73065/diff/21/26#newcode18
Line 18: static ExtensionBrowserEventRouter* GetInstance();
On 2009/04/15 05:02:13, Aaron Boodman wrote:
> Need comments for these public methods.

Done.

http://codereview.chromium.org/73065/diff/21/26#newcode20
Line 20: ExtensionBrowserEventRouter();
On 2009/04/15 15:08:38, brettw wrote:
> This should be first. Why do you need it to be public when you have
GetInstance?
> Can you clarify who should use this or make it private?

Done. Is now private.

http://codereview.chromium.org/73065/diff/21/26#newcode23
Line 23: static const std::string kOnTabMoved;
On 2009/04/15 05:02:13, Aaron Boodman wrote:
> We aren't allowed to have static objects, so this needs to be a char[].

Done.

http://codereview.chromium.org/73065/diff/21/26#newcode23
Line 23: static const std::string kOnTabMoved;
On 2009/04/15 18:45:25, Matt Perry wrote:
> From what I can tell, this isn't referenced anywhere except in this class.  If
> that's true, it should be private.
> 
> In that vein, I prefer not to have private static class consts.  I'd put it as
a
> file-level static in the .cc file, so you don't have to declare them in the
> header.  (Just an annoying C++ constraint.)

Done.

http://codereview.chromium.org/73065/diff/21/28
File chrome/browser/extensions/extension_message_service.cc (right):

http://codereview.chromium.org/73065/diff/21/28#newcode57
Line 57: std::set<int> ExtensionMessageService::unique_process_ids() {
On 2009/04/15 05:02:13, Aaron Boodman wrote:
> This naming convention -- foo_bar() is only used for really cheap getters.
Since
> this is creating a new object, I think it should be GetUniqueProcessIds().

Done.

http://codereview.chromium.org/73065/diff/21/28#newcode62
Line 62: for (it = process_ids_.begin(); it != process_ids_.end(); it++) {
On 2009/04/15 18:45:25, Matt Perry wrote:
> This won't work in single-process mode, or in the future when we have process
> sharing.  There's no guarantee that a given process ID will appear only once
in
> this map.

std:set<int> should only allow each id once.

http://codereview.chromium.org/73065/diff/21/28#newcode71
Line 71: // TODO(mpcomplete): We need to ensure an extension always ends up in a
single
On 2009/04/15 05:02:13, Aaron Boodman wrote:
> Check with Matt, but I think this todo might be done and you can remove it as
> long as you're in here.

Done.

http://codereview.chromium.org/73065/diff/21/28#newcode71
Line 71: // TODO(mpcomplete): We need to ensure an extension always ends up in a
single
On 2009/04/15 18:45:25, Matt Perry wrote:
> On 2009/04/15 05:02:13, Aaron Boodman wrote:
> > Check with Matt, but I think this todo might be done and you can remove it
as
> > long as you're in here.
> 
> Yeah, thanks.

Done.

http://codereview.chromium.org/73065/diff/21/28#newcode187
Line 187: }
On 2009/04/15 18:45:25, Matt Perry wrote:
> I think gcc will complain about the missing newline at the end of the file.

Done.

http://codereview.chromium.org/73065/diff/21/24
File chrome/browser/extensions/extension_message_service.h (right):

http://codereview.chromium.org/73065/diff/21/24#newcode66
Line 66: private:
On 2009/04/15 15:08:38, brettw wrote:
> You lost the sapce before this.

Done.

http://codereview.chromium.org/73065/diff/21/29
File chrome/browser/extensions/extension_process_manager.cc (right):

http://codereview.chromium.org/73065/diff/21/29#newcode43
Line 43: const ListValue& data) {
On 2009/04/15 05:02:13, Aaron Boodman wrote:
> This last param can go on the previous line, to be more like the rest of the
> code in this file.

Done.

http://codereview.chromium.org/73065/diff/21/27
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/73065/diff/21/27#newcode217
Line 217: int ExtensionTabUtil::GetTabId(const TabContents& tab_contents) {
On 2009/04/15 18:45:25, Matt Perry wrote:
> fyi, try to define functions in .cc files in the same order you declare them
in
> the header.

Done.

http://codereview.chromium.org/73065/diff/21/23
File chrome/browser/extensions/extension_tabs_module.h (right):

http://codereview.chromium.org/73065/diff/21/23#newcode12
Line 12: class ExtensionTabUtil {
On 2009/04/15 18:45:25, Matt Perry wrote:
> Does anyone use this outside of the .cc file?  If not, no need to put it in
the
> header.

Yes. It is used by the ExtensionBrowserEventRouter. Let me know if there's
preferable way to do this.

http://codereview.chromium.org/73065/diff/21/23#newcode14
Line 14: static int GetTabId(const TabContents& tab_contents);
On 2009/04/15 18:45:25, Matt Perry wrote:
> This is one place where the rule of thumb I gave you doesn't apply :).  Even
> though this can't be NULL in this case, we never pass TabContents by ref.  Use
a
> pointer for consistency.

Done.

http://codereview.chromium.org/73065/diff/21/33
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/73065/diff/21/33#newcode554
Line 554: IPC_MESSAGE_CONTROL2(ViewMsg_Extension_DispatchEvent,
On 2009/04/15 18:45:25, Matt Perry wrote:
> nit: I'd name this ViewMsg_ExtensionHandleEvent to be more like the *Connect
and
> *Message events.

Done.

http://codereview.chromium.org/73065/diff/21/33#newcode555
Line 555: std::string /*event_name*/,
On 2009/04/15 05:02:13, Aaron Boodman wrote:
> add a space on either side of the arg name, to be more like the other code?

Done.

http://codereview.chromium.org/73065/diff/21/36
File chrome/renderer/extensions/renderer_extension_bindings.h (right):

http://codereview.chromium.org/73065/diff/21/36#newcode27
Line 27: static void DispatchEvent(const std::string& event_name,
On 2009/04/15 05:02:13, Aaron Boodman wrote:
> Needs a comment.

Done.

http://codereview.chromium.org/73065/diff/21/36#newcode27
Line 27: static void DispatchEvent(const std::string& event_name,
On 2009/04/15 18:45:25, Matt Perry wrote:
> same nit as the IPC message

Done.

http://codereview.chromium.org/73065/diff/21/34
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/73065/diff/21/34#newcode169
Line 169: void RenderThread::OnDispatchExtensionEvent(const std::string
event_name,
On 2009/04/15 18:45:25, Matt Perry wrote:
> nit: move this down next to OnExtensionHandleMessage, and change the name to
be
> consistent with that.

Done.

http://codereview.chromium.org/73065/diff/21/34#newcode199
Line 199: OnDispatchExtensionEvent)
On 2009/04/15 18:45:25, Matt Perry wrote:
> On 2009/04/15 07:06:24, John Abd-El-Malek wrote:
> > seeing as there are now a number of extension related messages, what about
> > creating an ExtensionDispatcher like ResourceDispatcher/AppCacheDispatcher
so
> > that extension related logic is separated from RenderThread?  This would
also
> > make testing easier..
> 
> That sounds reasonable.  Rafael, add a TODO next to these extension messages
> about that.

Done.

http://codereview.chromium.org/73065/diff/21/34#newcode199
Line 199: OnDispatchExtensionEvent)
On 2009/04/15 07:06:24, John Abd-El-Malek wrote:
> seeing as there are now a number of extension related messages, what about
> creating an ExtensionDispatcher like ResourceDispatcher/AppCacheDispatcher so
> that extension related logic is separated from RenderThread?  This would also
> make testing easier..

Added a TODO.

http://codereview.chromium.org/73065/diff/21/38
File chrome/renderer/resources/event_bindings.js (right):

http://codereview.chromium.org/73065/diff/21/38#newcode25
Line 25: chromium.Event.dispatchJSON_ = function(name, data) {
On 2009/04/15 05:02:13, Aaron Boodman wrote:
> To make the intended usage clear, how about changing the second param name to
> 'args' and updating the comment to explain that it should be an array of
> arguments.

Done.

http://codereview.chromium.org/73065/diff/21/38#newcode30
Line 30: chromium.Event.attached_[name].dispatch.apply(
On 2009/04/15 18:45:25, Matt Perry wrote:
> I think data should be a single JSON object, not an array of arguments. 
> Thoughts?

Offline decision: Aaron & Matt. Keep as list of arguments.

http://codereview.chromium.org/73065/diff/21/38#newcode39
Line 39: chromium.Event.attached_[name], data);
On 2009/04/15 18:45:25, Matt Perry wrote:
> Ditto for this.  If we do decide dispatch_ should take an array, you need to
fix
> dispatchOnConnect_ in renderer_extension_bindings.js which calls it with 1
arg.

Done.

http://codereview.chromium.org/73065/diff/21/38#newcode124
Line 124: chromium.onTabMoved = new chromium.Event('tabMoved');
On 2009/04/15 05:02:13, Aaron Boodman wrote:
> Nit: To be consistent with the way our native functions are named, I think the
> name of this event should be "TabMoved". Also, prefer double quotes to be
> consistent with the rest of our js.

Double-quoted.

http://codereview.chromium.org/73065/diff/21/38#newcode124
Line 124: chromium.onTabMoved = new chromium.Event('tabMoved');
On 2009/04/15 07:13:41, Aaron Boodman wrote:
> Also, this should be chromium.tabs.onTabMoved, and I was thinking it would be
> created in extension_process_bindings, near the other tabs stuff.

Done.

http://codereview.chromium.org/73065/diff/21/38#newcode124
Line 124: chromium.onTabMoved = new chromium.Event('tabMoved');
On 2009/04/15 07:13:41, Aaron Boodman wrote:
> On 2009/04/15 05:02:13, Aaron Boodman wrote:
> > Nit: To be consistent with the way our native functions are named, I think
the
> > name of this event should be "TabMoved". Also, prefer double quotes to be
> > consistent with the rest of our js.
> 
> Hm, I see now that matt is using "tab-moved". So I guess go with that.

Done.

[Matt Perry, 2009-04-15 23:35:17]

Beautiful

http://codereview.chromium.org/73065/diff/4010/3038
File chrome/browser/extensions/extension_message_service.h (right):

http://codereview.chromium.org/73065/diff/4010/3038#newcode64
Line 64: std::set<int> GetUniqueProcessIds();
Actually now that I think about it, it makes sense to cache this set.  It will
probably get used by events enough to make the miniscule memory cost worth it. 
Don't worry about it for this CL tho.

[Aaron Boodman, 2009-04-15 23:51:47]

http://codereview.chromium.org/73065/diff/4010/3049
File chrome/renderer/extensions/renderer_extension_bindings.h (right):

http://codereview.chromium.org/73065/diff/4010/3049#newcode29
Line 29: const std::string& event_data);
Little nit: call the second param 'args' and clarify in the comment that it is
meant to be a json-encoded array of arguments.

[Aaron Boodman, 2009-04-15 23:52:07]

lgtm

sweet :)