Changed the extension.connect() API not to broadcast to all tabs. Added a

Changed the extension.connect() API not to broadcast to all tabs.  Added a
more specific tabs.connect(tabId) API to connect to a specific tab.

Also changed the ExtensionMessageService from a singleton to a Profile-owned object.

BUG=12461
TEST=no

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

[Aaron Boodman, 2009-07-17 22:48:45]

http://codereview.chromium.org/155707/diff/1001/67
File chrome/browser/extensions/extension_bookmarks_module.cc (right):

http://codereview.chromium.org/155707/diff/1001/67#newcode157
Line 157: if (profile->GetExtensionMessageService()) {
Just curious: is the main case this instance doesn't exist testing?

http://codereview.chromium.org/155707/diff/1001/71
File chrome/browser/extensions/extension_message_service.h (right):

http://codereview.chromium.org/155707/diff/1001/71#newcode33
Line 33: // open a channel to a content script in the given tab.  The EMS
notifies the
Nit: "to a content script in the given tab" should be "to the content scripts
for a given tab" (plural), right?

http://codereview.chromium.org/155707/diff/1001/71#newcode35
Line 35: // in that process/tab.
Maybe worth clarifying that in the case of tabs, we only send messages to
contexts in the same extension.

http://codereview.chromium.org/155707/diff/1001/76
File chrome/browser/profile.h (right):

http://codereview.chromium.org/155707/diff/1001/76#newcode137
Line 137: // profile.  The instance is created on demand.
Comment is incorrect, the instance is created at profile construction.

http://codereview.chromium.org/155707/diff/1001/82
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/155707/diff/1001/82#newcode555
Line 555: // Call a javascript function in every registered context in this
process.
Comment looks out of date now?

http://codereview.chromium.org/155707/diff/1001/83
File chrome/renderer/extensions/bindings_utils.h (right):

http://codereview.chromium.org/155707/diff/1001/83#newcode70
Line 70: RenderView* renderview;
Nit: render_view.

http://codereview.chromium.org/155707/diff/1001/84
File chrome/renderer/extensions/event_bindings.cc (right):

http://codereview.chromium.org/155707/diff/1001/84#newcode159
Line 159: (*context_iter)->parent_context.Dispose();
This doesn't seem related to the rest of this change. Is it just a bug you
happened across?

http://codereview.chromium.org/155707/diff/1001/84#newcode213
Line 213: if (frame_context != context) {
This seems brittle because there can be other contexts that aren't content
scripts. For example I believe that DOM inspector creates new contexts. Can we
change WebFrame::EvaluateInIsolatedWorld() to return the v8 context directly
(when used with v8 bindings).

[Aaron Boodman, 2009-07-17 22:54:18]

http://codereview.chromium.org/155707/diff/1001/70
File chrome/browser/extensions/extension_message_service.cc (right):

http://codereview.chromium.org/155707/diff/1001/70#newcode225
Line 225: }
Looks like we default to sending to all tabs in the process? This should be an
error.

[Matt Perry, 2009-07-17 23:06:02]

http://codereview.chromium.org/155707/diff/1001/67
File chrome/browser/extensions/extension_bookmarks_module.cc (right):

http://codereview.chromium.org/155707/diff/1001/67#newcode157
Line 157: if (profile->GetExtensionMessageService()) {
On 2009/07/17 22:48:46, Aaron Boodman wrote:
> Just curious: is the main case this instance doesn't exist testing?

No, incognito mode.  If this is an incognito profile, it has no EMS.

http://codereview.chromium.org/155707/diff/1001/71
File chrome/browser/extensions/extension_message_service.h (right):

http://codereview.chromium.org/155707/diff/1001/71#newcode33
Line 33: // open a channel to a content script in the given tab.  The EMS
notifies the
On 2009/07/17 22:48:46, Aaron Boodman wrote:
> Nit: "to a content script in the given tab" should be "to the content scripts
> for a given tab" (plural), right?

Done.

http://codereview.chromium.org/155707/diff/1001/71#newcode35
Line 35: // in that process/tab.
On 2009/07/17 22:48:46, Aaron Boodman wrote:
> Maybe worth clarifying that in the case of tabs, we only send messages to
> contexts in the same extension.

Done.

http://codereview.chromium.org/155707/diff/1001/76
File chrome/browser/profile.h (right):

http://codereview.chromium.org/155707/diff/1001/76#newcode137
Line 137: // profile.  The instance is created on demand.
On 2009/07/17 22:48:46, Aaron Boodman wrote:
> Comment is incorrect, the instance is created at profile construction.

Done.

http://codereview.chromium.org/155707/diff/1001/82
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/155707/diff/1001/82#newcode555
Line 555: // Call a javascript function in every registered context in this
process.
On 2009/07/17 22:48:46, Aaron Boodman wrote:
> Comment looks out of date now?

Done.

http://codereview.chromium.org/155707/diff/1001/83
File chrome/renderer/extensions/bindings_utils.h (right):

http://codereview.chromium.org/155707/diff/1001/83#newcode70
Line 70: RenderView* renderview;
On 2009/07/17 22:48:46, Aaron Boodman wrote:
> Nit: render_view.

Done.

http://codereview.chromium.org/155707/diff/1001/84
File chrome/renderer/extensions/event_bindings.cc (right):

http://codereview.chromium.org/155707/diff/1001/84#newcode159
Line 159: (*context_iter)->parent_context.Dispose();
On 2009/07/17 22:48:46, Aaron Boodman wrote:
> This doesn't seem related to the rest of this change. Is it just a bug you
> happened across?

Yes.

http://codereview.chromium.org/155707/diff/1001/84#newcode213
Line 213: if (frame_context != context) {
On 2009/07/17 22:48:46, Aaron Boodman wrote:
> This seems brittle because there can be other contexts that aren't content
> scripts. For example I believe that DOM inspector creates new contexts. Can we
> change WebFrame::EvaluateInIsolatedWorld() to return the v8 context directly
> (when used with v8 bindings).

We only get notified about Frame contexts and content script contexts.  I tried
to make this less ugly with an extra param.

[Matt Perry, 2009-07-17 23:06:52]

http://codereview.chromium.org/155707/diff/1001/70
File chrome/browser/extensions/extension_message_service.cc (right):

http://codereview.chromium.org/155707/diff/1001/70#newcode225
Line 225: }
On 2009/07/17 22:54:18, Aaron Boodman wrote:
> Looks like we default to sending to all tabs in the process? This should be an
> error.

receiver.sender will be NULL otherwise, and we handle that error in the Impl
function.

[Aaron Boodman, 2009-07-20 18:02:15]

lgtm