Only send events to renderers that are listening to an event....

Only send events to renderers that are listening to an event.Notify browser when a renderer is listening to an event.

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

[Matt Perry, 2009-04-30 19:11:40]

http://codereview.chromium.org/100214/diff/14/16
File chrome/browser/extensions/extension_message_service.cc (right):

http://codereview.chromium.org/100214/diff/14/16#newcode67
Line 67: CHECK(MessageLoop::current() == MessageLoopForUI::current());
Cool, I didn't know we had message loop types!  However, I think a better check
would be MessageLoop::current()->type() == MessageLoop::TYPE_UI. 
MessageLoopForUI::current() just calls through to MessageLoop::current (with an
extra DCHECK).

http://codereview.chromium.org/100214/diff/14/16#newcode67
Line 67: CHECK(MessageLoop::current() == MessageLoopForUI::current());
Also, I prefer doing DCHECKs for these.  Calling a function on the wrong thread
is something we should find in development, and should never happen in a
release.

http://codereview.chromium.org/100214/diff/14/16#newcode78
Line 78: listeners_[event_name].insert(render_process_id);
should we DCHECK that this process doesn't already exist in the map?

http://codereview.chromium.org/100214/diff/14/18
File chrome/renderer/extensions/event_bindings.cc (right):

http://codereview.chromium.org/100214/diff/14/18#newcode50
Line 50: // TODO(mpcomplete): I'm just using this to register the v8 Context
right now.
this TODO can be removed

http://codereview.chromium.org/100214/diff/14/18#newcode74
Line 74: if (args.Length() != 1)
move this up to the top and also check that the arg is a string.  this function
shouldn't get called with invalid args (maybe a DCHECK).

http://codereview.chromium.org/100214/diff/14/18#newcode77
Line 77: RenderThread::current()->ExtensionAddListener(
this is kind of a style thing, but IMO, event_bindings should keep count of the
event listeners.  you can add the map to the ExtensionData singleton.  my
motivation here is to keep extension-specific code out of RenderThread as much
as possible.

[Erik does not do reviews, 2009-04-30 21:29:07]

new snapshot ready

http://codereview.chromium.org/100214/diff/14/16
File chrome/browser/extensions/extension_message_service.cc (right):

http://codereview.chromium.org/100214/diff/14/16#newcode67
Line 67: CHECK(MessageLoop::current() == MessageLoopForUI::current());
On 2009/04/30 19:11:40, Matt Perry wrote:
> Cool, I didn't know we had message loop types!  However, I think a better
check
> would be MessageLoop::current()->type() == MessageLoop::TYPE_UI. 
> MessageLoopForUI::current() just calls through to MessageLoop::current (with
an
> extra DCHECK).

done

http://codereview.chromium.org/100214/diff/14/16#newcode67
Line 67: CHECK(MessageLoop::current() == MessageLoopForUI::current());
On 2009/04/30 19:11:40, Matt Perry wrote:
> Also, I prefer doing DCHECKs for these.  Calling a function on the wrong
thread
> is something we should find in development, and should never happen in a
> release.

OK.  Since this is more of an API issue, I agree that this should be a DCHECK.

http://codereview.chromium.org/100214/diff/14/16#newcode78
Line 78: listeners_[event_name].insert(render_process_id);
On 2009/04/30 19:11:40, Matt Perry wrote:
> should we DCHECK that this process doesn't already exist in the map?

sure (and a similar check in Remove).

http://codereview.chromium.org/100214/diff/14/18
File chrome/renderer/extensions/event_bindings.cc (right):

http://codereview.chromium.org/100214/diff/14/18#newcode50
Line 50: // TODO(mpcomplete): I'm just using this to register the v8 Context
right now.
On 2009/04/30 19:11:40, Matt Perry wrote:
> this TODO can be removed

done

http://codereview.chromium.org/100214/diff/14/18#newcode74
Line 74: if (args.Length() != 1)
On 2009/04/30 19:11:40, Matt Perry wrote:
> move this up to the top and also check that the arg is a string.  this
function
> shouldn't get called with invalid args (maybe a DCHECK).

good idea

http://codereview.chromium.org/100214/diff/14/18#newcode77
Line 77: RenderThread::current()->ExtensionAddListener(
On 2009/04/30 19:11:40, Matt Perry wrote:
> this is kind of a style thing, but IMO, event_bindings should keep count of
the
> event listeners.  you can add the map to the ExtensionData singleton.  my
> motivation here is to keep extension-specific code out of RenderThread as much
> as possible.

ok

[Matt Perry, 2009-04-30 21:34:03]

LG.  2 suggestions

http://codereview.chromium.org/100214/diff/1025/32
File chrome/renderer/extensions/event_bindings.cc (right):

http://codereview.chromium.org/100214/diff/1025/32#newcode29
Line 29: return (data->listener_count[event_name] += 1);
++foo?

http://codereview.chromium.org/100214/diff/1025/32#newcode86
Line 86: RenderThread::current()->ExtensionAddListener(name);
how about just
RenderThread::current()->Send(new ViewMsg_whatever(name))

[Erik does not do reviews, 2009-05-01 00:17:45]

It turns out that I had broken the ExtensionMessagesOpenChannel unit test by
depending on RenderThread::current().  I added some hooks to pass in the
MockRenderThread and it's all fine.  (although we should really move the various
extension-specific stuff out of RenderViewTest into something else)

http://codereview.chromium.org/100214/diff/1025/32
File chrome/renderer/extensions/event_bindings.cc (right):

http://codereview.chromium.org/100214/diff/1025/32#newcode29
Line 29: return (data->listener_count[event_name] += 1);
On 2009/04/30 21:34:03, Matt Perry wrote:
> ++foo?

yeah, it just looked really odd to me with that expression.  I was half tempted
to do:
foo++;
return foo;

http://codereview.chromium.org/100214/diff/1025/32#newcode86
Line 86: RenderThread::current()->ExtensionAddListener(name);
On 2009/04/30 21:34:03, Matt Perry wrote:
> how about just
> RenderThread::current()->Send(new ViewMsg_whatever(name))

Ah.  I didn't realize that Send was a public method like that.  Cool.

[Matt Perry, 2009-05-01 00:22:05]

http://codereview.chromium.org/100214/diff/55/59
File chrome/renderer/extensions/event_bindings.cc (right):

http://codereview.chromium.org/100214/diff/55/59#newcode19
Line 19: static RenderThreadBase* kRenderThread = NULL;
This is not a constant, so it should be named like render_thread.

http://codereview.chromium.org/100214/diff/55/59#newcode124
Line 124: if (!kRenderThread)
how about a helper:
GetRenderThread() { return render_thread ? render_thread :
RenderThread::current(); }

[Erik does not do reviews, 2009-05-01 00:31:28]

http://codereview.chromium.org/100214/diff/55/59
File chrome/renderer/extensions/event_bindings.cc (right):

http://codereview.chromium.org/100214/diff/55/59#newcode19
Line 19: static RenderThreadBase* kRenderThread = NULL;
On 2009/05/01 00:22:05, Matt Perry wrote:
> This is not a constant, so it should be named like render_thread.

yep

http://codereview.chromium.org/100214/diff/55/59#newcode124
Line 124: if (!kRenderThread)
On 2009/05/01 00:22:05, Matt Perry wrote:
> how about a helper:
> GetRenderThread() { return render_thread ? render_thread :
> RenderThread::current(); }

good idea

[Matt Perry, 2009-05-01 00:34:01]

LGTM