[Extensions Bindings] Add Events support

[Extensions Bindings] Add Events support

Add an APIEventHandler that provides the ability to both create
v8::Objects for extension API events (and provides the implementation
for the event functions like hasListener()) and can dispatch events to
specific contexts.

Add specific event tests and update others to ensure it is wired in
correctly.

BUG=653596
Committed: https://crrev.com/9a0f04b02bd0d7c5d9156cd7155e9994caf19d71
Cr-Commit-Position: refs/heads/master@{#430340}

[Devlin, 2016-11-01 00:33:17]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 00:33:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2016-11-01 00:34:31]

rdevlin.cronin@chromium.org changed reviewers:
+ jbroman@chromium.org, lazyboy@chromium.org

[Devlin, 2016-11-01 00:34:32]

Hey folks - event time!

This one's a little longer than I anticipated, but I'm not sure there's a good
way to break it down.  Sorry in advance.

[commit-bot: I haz the power, 2016-11-01 01:45:15]

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

[commit-bot: I haz the power, 2016-11-01 01:45:16]

Dry run: This issue passed the CQ dry run.

[jbroman, 2016-11-01 18:59:24]

Focused on APIEventHandler for now; some long-ish thoughts here.

https://codereview.chromium.org/2469593002/diff/20001/extensions/renderer/api...
File extensions/renderer/api_event_handler.cc (right):

https://codereview.chromium.org/2469593002/diff/20001/extensions/renderer/api...
extensions/renderer/api_event_handler.cc:42: v8::Local<v8::Object>
APIEventHandler::CreateEventInstance(
High-level comment here: while we needed to get closer to the V8 API to deal
with some of the quirks of the extensions IDL (and the fact that we read the
JSON dynamically at runtime), the event system does not seem similarly
constrained. Have you looked at whether things turn out simpler if you just use
gin::Wrappable for this (leaving fewer of the subtle cases to write yourself)?
Something like (modulo choice of names and header/source-file split):

// should only be used from one context (a separate instance is expected
per-context)
class EventEmitter final : public gin::Wrappable<EventEmitter> {
 public:
  static gin::WrapperInfo kWrapperInfo = {gin::kEmbedderNativeGin};

  static gin::Handle<EventEmitter> Create(v8::Isolate* isolate,
v8::Local<v8::Context> context, const std::string& event_name) {
    return gin::CreateHandle(isolate, new EventEmitter(context, event_name));
  }

  gin::ObjectTemplateBuilder GetObjectTemplateBuilder(v8::Isolate* isolate)
final {
    return Wrappable<EventEmitter>::GetObjectTemplateBuilder(isolate)
        .SetMethod("addListener", &EventEmitter::AddListener)
        .SetMethod("removeListener", &EventEmitter::RemoveListener)
        .SetMethod("hasListener", &EventEmitter::HasListener)
        .SetMethod("hasListeners", &EventEmitter::HasListeners);
  }

  EventEmitter(v8::Local<v8::Context> context, const std::string& event_name) {
    // talk to APIEventPerContextData, initialize members, etc.
  }

  void AddListener(v8::Local<v8::Function> listener) {
    if (!HasListener(listener))
      listeners_.push_back(listener);
  }
  void RemoveListener(v8::Local<v8::Function> listener) { /* ... */ }
  bool HasListener(v8::Local<v8::Function> listener) { /* ... */ }
  bool HasListeners() { return !listeners_.empty(); }

  void FireEvent(...) { ... }

 private:
  std::vector<v8::Global<v8::Function>> listeners_;
};

And then you can just extract the v8 local from the gin::Handle when you want to
attach it to things.

While gin doesn't necessarily make _optimal_ choices for this use case, it comes
close and has been vetted by jochen@ for correctness. It will also give you
things like caching of the object template (and constituent function templates),
validating the underlying wrapped object, and so on "for free".

If you do switch to gin::Wrappable, it will nullify most (all?) of my other
comments here.

https://codereview.chromium.org/2469593002/diff/20001/extensions/renderer/api...
extensions/renderer/api_event_handler.cc:68:
CHECK(maybe_object.ToLocal(&object));
I believe this won't fail, because we know that since there is no provided
constructor function, it should use the built-in object constructor function,
which should neither throw nor configure properties in such a way that adding
addListener etc. should be able to fail. It used to be the case that problems
would happen if Object.prototype.constructor were overloaded, but I think that's
been changed since?

nit: You can probably just put this on one line (if you extract
context->GetIsolate() into a local variable, which seems helpful anyhow
considering how many times it occurs here):

CHECK(event_template_.Get(isolate)->NewInstance(context).ToLocal(&object));

https://codereview.chromium.org/2469593002/diff/20001/extensions/renderer/api...
extensions/renderer/api_event_handler.cc:107: // instantiate a single function
object?
It means that if you call v8::FunctionTemplate::GetFunction with the same
context, you get back the same v8::Function (with reference equality) each time.

For instance, chrome.idle.onStateChanged.addListener ===
chrome.runtime.onMessage.addListener.

The function objects are equal, but you can distinguish the call by examining
the holder of the function callback info.

You should only have one instance of this FunctionTemplate (and similar for
ObjectTemplate) for the entire isolate. Given that, event_template_ might as
well be held with a v8::Eternal, like gin::PerIsolateData does.

https://codereview.chromium.org/2469593002/diff/20001/extensions/renderer/api...
extensions/renderer/api_event_handler.cc:111: v8::Local<v8::Signature>(), 0,
v8::ConstructorBehavior::kThrow);
If you use a signature, you can have V8 validate that the holder is always an
instance of this ObjectTemplate, which limits the kind of weird tricks that
script can play. You'd do this by starting with a FunctionTemplate (that would
be the "constructor" of these objects, even though it wouldn't be exposed to
script), and then adding these to its PrototypeTemplate (or InstanceTemplate if
you'd really rather they be functions defined on the instance).

https://codereview.chromium.org/2469593002/diff/20001/extensions/renderer/api...
extensions/renderer/api_event_handler.cc:172: CHECK(value->IsExternal());
You can't rely on this working, at the moment. You're using the |this| value
provided by script, but it won't necessarily actually be an event-emitter
object.

For instance, script could do this:
chrome.runtime.onMessage.addEventListener.call(someOtherObject);

In which case these checks will fail. You should either use a signature (and
correspondingly, a FunctionTemplate for the hidden constructor of this type of
object) to have V8 check this before dispatching the call (this is what Blink
does) and guarantee that info.Holder() -- but not info.This() -- is actually a
wrapper object created by this class, or you should be prepared for these to
fail, and handle that case.

(Probably a fun category of cases to check with a unit test, incidentally.)

https://codereview.chromium.org/2469593002/diff/20001/extensions/renderer/api...
extensions/renderer/api_event_handler.cc:183: GetListenersForObject(caller,
args->isolate()->GetCurrentContext());
Didn't catch this before, but getting the context from the holder is what Blink
does, rather than using GetCurrentContext.

v8::Local<v8::Object> holder;
if (!args->GetHolder(&holder))
  return;
v8::Local<v8::Context> context = holder->CreationContext();
EventListeners* listeners = GetListenersForObject(holder, context);

[jbroman, 2016-11-01 19:14:34]

https://codereview.chromium.org/2469593002/diff/20001/extensions/renderer/api...
File extensions/renderer/api_event_handler.cc (right):

https://codereview.chromium.org/2469593002/diff/20001/extensions/renderer/api...
extensions/renderer/api_event_handler.cc:42: v8::Local<v8::Object>
APIEventHandler::CreateEventInstance(
On 2016/11/01 at 18:59:24, jbroman wrote:
> High-level comment here: while we needed to get closer to the V8 API to deal
with some of the quirks of the extensions IDL (and the fact that we read the
JSON dynamically at runtime), the event system does not seem similarly
constrained. Have you looked at whether things turn out simpler if you just use
gin::Wrappable for this (leaving fewer of the subtle cases to write yourself)?
Something like (modulo choice of names and header/source-file split):
> 
> // should only be used from one context (a separate instance is expected
per-context)
> class EventEmitter final : public gin::Wrappable<EventEmitter> {
>  public:
>   static gin::WrapperInfo kWrapperInfo = {gin::kEmbedderNativeGin};
> 
>   static gin::Handle<EventEmitter> Create(v8::Isolate* isolate,
v8::Local<v8::Context> context, const std::string& event_name) {
>     return gin::CreateHandle(isolate, new EventEmitter(context, event_name));
>   }
> 
>   gin::ObjectTemplateBuilder GetObjectTemplateBuilder(v8::Isolate* isolate)
final {
>     return Wrappable<EventEmitter>::GetObjectTemplateBuilder(isolate)
>         .SetMethod("addListener", &EventEmitter::AddListener)
>         .SetMethod("removeListener", &EventEmitter::RemoveListener)
>         .SetMethod("hasListener", &EventEmitter::HasListener)
>         .SetMethod("hasListeners", &EventEmitter::HasListeners);
>   }
> 
>   EventEmitter(v8::Local<v8::Context> context, const std::string& event_name)
{
>     // talk to APIEventPerContextData, initialize members, etc.
>   }
> 
>   void AddListener(v8::Local<v8::Function> listener) {
>     if (!HasListener(listener))
>       listeners_.push_back(listener);
>   }
>   void RemoveListener(v8::Local<v8::Function> listener) { /* ... */ }
>   bool HasListener(v8::Local<v8::Function> listener) { /* ... */ }
>   bool HasListeners() { return !listeners_.empty(); }
> 
>   void FireEvent(...) { ... }
> 
>  private:
>   std::vector<v8::Global<v8::Function>> listeners_;
> };
> 
> And then you can just extract the v8 local from the gin::Handle when you want
to attach it to things.
> 
> While gin doesn't necessarily make _optimal_ choices for this use case, it
comes close and has been vetted by jochen@ for correctness. It will also give
you things like caching of the object template (and constituent function
templates), validating the underlying wrapped object, and so on "for free".
> 
> If you do switch to gin::Wrappable, it will nullify most (all?) of my other
comments here.

One note here: a little more caution than I've implied here is necessary to
avoid cycles between listeners and the emitter wrapper causing a leak. Probably
resolvable just by clearing out the listener list when the context is dying (or
they otherwise could not possibly be needed anymore), though there are more
sophisticated approaches (like making the globals weak and keeping them alive
through a private property).

[Devlin, 2016-11-02 00:39:07]

Patchset #3 (id:40001) has been deleted

[Devlin, 2016-11-02 00:48:29]

https://codereview.chromium.org/2469593002/diff/20001/extensions/renderer/api...
File extensions/renderer/api_event_handler.cc (right):

https://codereview.chromium.org/2469593002/diff/20001/extensions/renderer/api...
extensions/renderer/api_event_handler.cc:42: v8::Local<v8::Object>
APIEventHandler::CreateEventInstance(
On 2016/11/01 19:14:33, jbroman wrote:
> On 2016/11/01 at 18:59:24, jbroman wrote:
> > High-level comment here: while we needed to get closer to the V8 API to deal
> with some of the quirks of the extensions IDL (and the fact that we read the
> JSON dynamically at runtime), the event system does not seem similarly
> constrained. Have you looked at whether things turn out simpler if you just
use
> gin::Wrappable for this (leaving fewer of the subtle cases to write yourself)?
> Something like (modulo choice of names and header/source-file split):
> > 
> > // should only be used from one context (a separate instance is expected
> per-context)
> > class EventEmitter final : public gin::Wrappable<EventEmitter> {
> >  public:
> >   static gin::WrapperInfo kWrapperInfo = {gin::kEmbedderNativeGin};
> > 
> >   static gin::Handle<EventEmitter> Create(v8::Isolate* isolate,
> v8::Local<v8::Context> context, const std::string& event_name) {
> >     return gin::CreateHandle(isolate, new EventEmitter(context,
event_name));
> >   }
> > 
> >   gin::ObjectTemplateBuilder GetObjectTemplateBuilder(v8::Isolate* isolate)
> final {
> >     return Wrappable<EventEmitter>::GetObjectTemplateBuilder(isolate)
> >         .SetMethod("addListener", &EventEmitter::AddListener)
> >         .SetMethod("removeListener", &EventEmitter::RemoveListener)
> >         .SetMethod("hasListener", &EventEmitter::HasListener)
> >         .SetMethod("hasListeners", &EventEmitter::HasListeners);
> >   }
> > 
> >   EventEmitter(v8::Local<v8::Context> context, const std::string&
event_name)
> {
> >     // talk to APIEventPerContextData, initialize members, etc.
> >   }
> > 
> >   void AddListener(v8::Local<v8::Function> listener) {
> >     if (!HasListener(listener))
> >       listeners_.push_back(listener);
> >   }
> >   void RemoveListener(v8::Local<v8::Function> listener) { /* ... */ }
> >   bool HasListener(v8::Local<v8::Function> listener) { /* ... */ }
> >   bool HasListeners() { return !listeners_.empty(); }
> > 
> >   void FireEvent(...) { ... }
> > 
> >  private:
> >   std::vector<v8::Global<v8::Function>> listeners_;
> > };
> > 
> > And then you can just extract the v8 local from the gin::Handle when you
want
> to attach it to things.
> > 
> > While gin doesn't necessarily make _optimal_ choices for this use case, it
> comes close and has been vetted by jochen@ for correctness. It will also give
> you things like caching of the object template (and constituent function
> templates), validating the underlying wrapped object, and so on "for free".
> > 
> > If you do switch to gin::Wrappable, it will nullify most (all?) of my other
> comments here.
> 
> One note here: a little more caution than I've implied here is necessary to
> avoid cycles between listeners and the emitter wrapper causing a leak.
Probably
> resolvable just by clearing out the listener list when the context is dying
(or
> they otherwise could not possibly be needed anymore), though there are more
> sophisticated approaches (like making the globals weak and keeping them alive
> through a private property).

So, there's a few gotchas here and there with how we would have to cache and
lookup the event emitters for each event-context pair, etc, that make me a
little dubious of the full EventEmitter approach.  That said, I've managed to
take (what I think is) the main motivation of it (using gin::Wrappable,
ObjectTemplateBuilder, etc) and combine it with some of the nice bits of this
one (easy to look up event listeners for a context, dispatch events, single
object to route to).  I think it's pretty clean.  But I might have just made
Frankenstein's monster to someone's eyes if they're more knowledgable about
v8/gin. :)  Lemme know if this is on the right track!

https://codereview.chromium.org/2469593002/diff/20001/extensions/renderer/api...
extensions/renderer/api_event_handler.cc:68:
CHECK(maybe_object.ToLocal(&object));
On 2016/11/01 18:59:24, jbroman wrote:
> I believe this won't fail, because we know that since there is no provided
> constructor function, it should use the built-in object constructor function,
> which should neither throw nor configure properties in such a way that adding
> addListener etc. should be able to fail. It used to be the case that problems
> would happen if Object.prototype.constructor were overloaded, but I think
that's
> been changed since?
> 
> nit: You can probably just put this on one line (if you extract
> context->GetIsolate() into a local variable, which seems helpful anyhow
> considering how many times it occurs here):
> 
> CHECK(event_template_.Get(isolate)->NewInstance(context).ToLocal(&object));

Moot if we go with the latest patch set.

https://codereview.chromium.org/2469593002/diff/20001/extensions/renderer/api...
extensions/renderer/api_event_handler.cc:107: // instantiate a single function
object?
On 2016/11/01 18:59:24, jbroman wrote:
> It means that if you call v8::FunctionTemplate::GetFunction with the same
> context, you get back the same v8::Function (with reference equality) each
time.
> 
> For instance, chrome.idle.onStateChanged.addListener ===
> chrome.runtime.onMessage.addListener.
> 
> The function objects are equal, but you can distinguish the call by examining
> the holder of the function callback info.
> 
> You should only have one instance of this FunctionTemplate (and similar for
> ObjectTemplate) for the entire isolate. Given that, event_template_ might as
> well be held with a v8::Eternal, like gin::PerIsolateData does.

Ditto

https://codereview.chromium.org/2469593002/diff/20001/extensions/renderer/api...
extensions/renderer/api_event_handler.cc:111: v8::Local<v8::Signature>(), 0,
v8::ConstructorBehavior::kThrow);
On 2016/11/01 18:59:24, jbroman wrote:
> If you use a signature, you can have V8 validate that the holder is always an
> instance of this ObjectTemplate, which limits the kind of weird tricks that
> script can play. You'd do this by starting with a FunctionTemplate (that would
> be the "constructor" of these objects, even though it wouldn't be exposed to
> script), and then adding these to its PrototypeTemplate (or InstanceTemplate
if
> you'd really rather they be functions defined on the instance).

Ditto

https://codereview.chromium.org/2469593002/diff/20001/extensions/renderer/api...
extensions/renderer/api_event_handler.cc:172: CHECK(value->IsExternal());
On 2016/11/01 18:59:24, jbroman wrote:
> You can't rely on this working, at the moment. You're using the |this| value
> provided by script, but it won't necessarily actually be an event-emitter
> object.
> 
> For instance, script could do this:
> chrome.runtime.onMessage.addEventListener.call(someOtherObject);
> 
> In which case these checks will fail. You should either use a signature (and
> correspondingly, a FunctionTemplate for the hidden constructor of this type of
> object) to have V8 check this before dispatching the call (this is what Blink
> does) and guarantee that info.Holder() -- but not info.This() -- is actually a
> wrapper object created by this class, or you should be prepared for these to
> fail, and handle that case.
> 
> (Probably a fun category of cases to check with a unit test, incidentally.)

Also moot.

https://codereview.chromium.org/2469593002/diff/20001/extensions/renderer/api...
extensions/renderer/api_event_handler.cc:183: GetListenersForObject(caller,
args->isolate()->GetCurrentContext());
On 2016/11/01 18:59:24, jbroman wrote:
> Didn't catch this before, but getting the context from the holder is what
Blink
> does, rather than using GetCurrentContext.
> 
> v8::Local<v8::Object> holder;
> if (!args->GetHolder(&holder))
>   return;
> v8::Local<v8::Context> context = holder->CreationContext();
> EventListeners* listeners = GetListenersForObject(holder, context);

And moot :)

https://codereview.chromium.org/2469593002/diff/60001/extensions/renderer/api...
File extensions/renderer/api_event_handler.cc (right):

https://codereview.chromium.org/2469593002/diff/60001/extensions/renderer/api...
extensions/renderer/api_event_handler.cc:51:
data->event_data.insert(std::make_pair(event_name, std::move(emitter)));
Most places instead seem to do:

gin::CreateHandle(isolate, new EventEmitter())

However, it's unclear to me how the EventEmitter would be deleted in that case
(if it would).  If it wouldn't, then this is fine (and in my mind preferable,
since it ensures we don't leak the listeners).  If it would, then we have a
double-free that's not detected from the tests, and I'm confused. :)

[jbroman, 2016-11-02 19:50:15]

Looks good to me in general, but we'll need to agree on a way to handle the
memory management of the listeners.

https://codereview.chromium.org/2469593002/diff/60001/extensions/renderer/api...
File extensions/renderer/api_event_handler.cc (right):

https://codereview.chromium.org/2469593002/diff/60001/extensions/renderer/api...
extensions/renderer/api_event_handler.cc:51:
data->event_data.insert(std::make_pair(event_name, std::move(emitter)));
On 2016/11/02 at 00:48:29, Devlin wrote:
> Most places instead seem to do:
> 
> gin::CreateHandle(isolate, new EventEmitter())
> 
> However, it's unclear to me how the EventEmitter would be deleted in that case
(if it would).  If it wouldn't, then this is fine (and in my mind preferable,
since it ensures we don't leak the listeners).  If it would, then we have a
double-free that's not detected from the tests, and I'm confused. :)

It's perhaps not obvious from the comment in gin/wrappable.h, but gin::Wrappable
objects are owned by their V8 wrapper objects. In particular, they register
themselves so that when their V8 wrapper is collected, they delete themselves.
As a result, keeping a unique_ptr around is actually a double-free. I expect
your unit tests simply aren't running a GC.

https://codereview.chromium.org/2469593002/diff/60001/extensions/renderer/api...
extensions/renderer/api_event_handler.cc:72: std::vector<v8::Local<v8::Value>>
v8_args;
Just confirming that this is the behaviour you want (seems fine to me): if any
of these is an object (including an array), one callback could modify it, and
those modifications would be visible to subsequent callbacks in the same
context.

https://codereview.chromium.org/2469593002/diff/60001/extensions/renderer/api...
extensions/renderer/api_event_handler.cc:79: for (auto& listener : *listeners) {
nit: const auto&

https://codereview.chromium.org/2469593002/diff/60001/extensions/renderer/api...
File extensions/renderer/api_event_handler.h (right):

https://codereview.chromium.org/2469593002/diff/60001/extensions/renderer/api...
extensions/renderer/api_event_handler.h:37: using HandlerCallback =
unused?

https://codereview.chromium.org/2469593002/diff/60001/extensions/renderer/api...
extensions/renderer/api_event_handler.h:54: const EventEmitter::Listeners&
GetEventListenersForTesting(
nit: It looks like the unit tests only look at the size of this. Could we reduce
the exposure of this API to just returning the number of event listeners?

https://codereview.chromium.org/2469593002/diff/60001/extensions/renderer/api...
extensions/renderer/api_event_handler.h:59: struct APIEventPerContextData :
public base::SupportsUserData::Data {
nit: Since this class doesn't seem to be used outside the implementation source
file, could it be in an anonymous namespace in that file? (This would also
mitigate the need to declare the constructor/destructor out-of-line.)

https://codereview.chromium.org/2469593002/diff/60001/extensions/renderer/api...
extensions/renderer/api_event_handler.h:64: std::map<std::string,
std::unique_ptr<EventEmitter>> event_data;
As mentioned elsewhere, you shouldn't hold a unique_ptr to a gin::Wrappable.

Could be an std::map<std::string, v8::Global<v8::Object>> (and then you can use
gin::Converter<EventEmitter*>::FromV8 to get the native object.

We'll still have to address the issue of cycles between V8 and the C++ heap
(that would otherwise cause leaks -- this kind of issue has been a thorn in
Blink's side for a good long while), so the persistent handles have to be
cleared to make all of the objects collectable. The problem here is cycles of
this form, which aren't traceable because of the two edges through native code:

EventEmitter (V8 object) --[owns]--> EventEmitter (native) --[persistent]-->
listener (V8 function) --[may capture (depending on script)]--> EventEmitter (V8
object)

As I see it, there are two major categories of approach here:

1. Explicitly clear the edges on context disposal. This is a bit of a hack, but
should work. Since the emitters live for the lifetime of the context and we're
done with them thereafter, we can have ~APIEventPerContextData clear each event
emitter's listener list (thus breaking the [persistent] edge in the diagram
above) on context disposal. If so, AddListener should check that the
PerContextData hasn't already been disposed (you can get at it via
arguments->GetHolder() and then CreationContext), because I think it's
technically possible for script to still be running for a bit after the context
is disposed. (Possible downside: this might be observable to script under
exceptional circumstances.)

2. Make the cycle visible to V8. This could be done by storing the strong
references using a V8 hidden property on the wrapper, rather than a persistent
handle on the native side. This is complicated slightly by the fact that we want
to store several listeners, not just one. Roughly, we'd have to store/remove
listeners using a v8::Set (or other object) accessed via GetPrivate/SetPrivate.
We could either use that exclusively, or we could have a parallel C++-side data
structure (vector or whatever), as long as it held only weak handles.

(Blink's been migrating toward a third thing, which traces between Oilpan and
V8, but that doesn't apply here.)

Aside: I only realized now that the extensions docs call these objects "events"
rather than 'targets' or 'emitters', which confused me for a bit because in web
APIs events are the things that listeners receive, and event targets are the
things they register to. Sorry for my use of this terminology being all over the
map. :)

https://codereview.chromium.org/2469593002/diff/60001/extensions/renderer/api...
extensions/renderer/api_event_handler.h:70:
base::WeakPtrFactory<APIEventHandler> weak_factory_;
Does anything use |weak_factory_|?

[Devlin, 2016-11-02 22:02:22]

https://codereview.chromium.org/2469593002/diff/60001/extensions/renderer/api...
File extensions/renderer/api_event_handler.cc (right):

https://codereview.chromium.org/2469593002/diff/60001/extensions/renderer/api...
extensions/renderer/api_event_handler.cc:51:
data->event_data.insert(std::make_pair(event_name, std::move(emitter)));
On 2016/11/02 19:50:14, jbroman wrote:
> On 2016/11/02 at 00:48:29, Devlin wrote:
> > Most places instead seem to do:
> > 
> > gin::CreateHandle(isolate, new EventEmitter())
> > 
> > However, it's unclear to me how the EventEmitter would be deleted in that
case
> (if it would).  If it wouldn't, then this is fine (and in my mind preferable,
> since it ensures we don't leak the listeners).  If it would, then we have a
> double-free that's not detected from the tests, and I'm confused. :)
> 
> It's perhaps not obvious from the comment in gin/wrappable.h, but
gin::Wrappable
> objects are owned by their V8 wrapper objects. In particular, they register
> themselves so that when their V8 wrapper is collected, they delete themselves.
> As a result, keeping a unique_ptr around is actually a double-free. I expect
> your unit tests simply aren't running a GC.

is there a way to run a gc in a unittest so we can add it?  (Also addressed
double-free; see other comment.)

https://codereview.chromium.org/2469593002/diff/60001/extensions/renderer/api...
extensions/renderer/api_event_handler.cc:72: std::vector<v8::Local<v8::Value>>
v8_args;
On 2016/11/02 19:50:14, jbroman wrote:
> Just confirming that this is the behaviour you want (seems fine to me): if any
> of these is an object (including an array), one callback could modify it, and
> those modifications would be visible to subsequent callbacks in the same
> context.

Good call!  I've checked and this is what we're currently doing, and given it's
within the same context (and thus the same extension), I think this is probably
fine.  But I've added a comment to make it clear.

https://codereview.chromium.org/2469593002/diff/60001/extensions/renderer/api...
extensions/renderer/api_event_handler.cc:79: for (auto& listener : *listeners) {
On 2016/11/02 19:50:14, jbroman wrote:
> nit: const auto&

Done.

https://codereview.chromium.org/2469593002/diff/60001/extensions/renderer/api...
File extensions/renderer/api_event_handler.h (right):

https://codereview.chromium.org/2469593002/diff/60001/extensions/renderer/api...
extensions/renderer/api_event_handler.h:37: using HandlerCallback =
On 2016/11/02 19:50:14, jbroman wrote:
> unused?

Removed.

https://codereview.chromium.org/2469593002/diff/60001/extensions/renderer/api...
extensions/renderer/api_event_handler.h:54: const EventEmitter::Listeners&
GetEventListenersForTesting(
On 2016/11/02 19:50:14, jbroman wrote:
> nit: It looks like the unit tests only look at the size of this. Could we
reduce
> the exposure of this API to just returning the number of event listeners?

Sure, done.

https://codereview.chromium.org/2469593002/diff/60001/extensions/renderer/api...
extensions/renderer/api_event_handler.h:59: struct APIEventPerContextData :
public base::SupportsUserData::Data {
On 2016/11/02 19:50:14, jbroman wrote:
> nit: Since this class doesn't seem to be used outside the implementation
source
> file, could it be in an anonymous namespace in that file? (This would also
> mitigate the need to declare the constructor/destructor out-of-line.)

Done.

https://codereview.chromium.org/2469593002/diff/60001/extensions/renderer/api...
extensions/renderer/api_event_handler.h:64: std::map<std::string,
std::unique_ptr<EventEmitter>> event_data;
On 2016/11/02 19:50:14, jbroman wrote:
> As mentioned elsewhere, you shouldn't hold a unique_ptr to a gin::Wrappable.
> 
> Could be an std::map<std::string, v8::Global<v8::Object>> (and then you can
use
> gin::Converter<EventEmitter*>::FromV8 to get the native object.
> 
> We'll still have to address the issue of cycles between V8 and the C++ heap
> (that would otherwise cause leaks -- this kind of issue has been a thorn in
> Blink's side for a good long while), so the persistent handles have to be
> cleared to make all of the objects collectable. The problem here is cycles of
> this form, which aren't traceable because of the two edges through native
code:
> 
> EventEmitter (V8 object) --[owns]--> EventEmitter (native) --[persistent]-->
> listener (V8 function) --[may capture (depending on script)]--> EventEmitter
(V8
> object)
> 
> As I see it, there are two major categories of approach here:
> 
> 1. Explicitly clear the edges on context disposal. This is a bit of a hack,
but
> should work. Since the emitters live for the lifetime of the context and we're
> done with them thereafter, we can have ~APIEventPerContextData clear each
event
> emitter's listener list (thus breaking the [persistent] edge in the diagram
> above) on context disposal. If so, AddListener should check that the
> PerContextData hasn't already been disposed (you can get at it via
> arguments->GetHolder() and then CreationContext), because I think it's
> technically possible for script to still be running for a bit after the
context
> is disposed. (Possible downside: this might be observable to script under
> exceptional circumstances.)
> 
> 2. Make the cycle visible to V8. This could be done by storing the strong
> references using a V8 hidden property on the wrapper, rather than a persistent
> handle on the native side. This is complicated slightly by the fact that we
want
> to store several listeners, not just one. Roughly, we'd have to store/remove
> listeners using a v8::Set (or other object) accessed via
GetPrivate/SetPrivate.
> We could either use that exclusively, or we could have a parallel C++-side
data
> structure (vector or whatever), as long as it held only weak handles.
> 
> (Blink's been migrating toward a third thing, which traces between Oilpan and
> V8, but that doesn't apply here.)
> 
> Aside: I only realized now that the extensions docs call these objects
"events"
> rather than 'targets' or 'emitters', which confused me for a bit because in
web
> APIs events are the things that listeners receive, and event targets are the
> things they register to. Sorry for my use of this terminology being all over
the
> map. :)

Thanks for the detailed explanation!  That makes a lot more sense now.  I've
opted for approach 1) (just clearing the map when the context is destroyed); it
seems the most reasonable and least likely mess up. :)  If we end up seeing
exceptional circumstances, we can revisit.

https://codereview.chromium.org/2469593002/diff/60001/extensions/renderer/api...
extensions/renderer/api_event_handler.h:70:
base::WeakPtrFactory<APIEventHandler> weak_factory_;
On 2016/11/02 19:50:14, jbroman wrote:
> Does anything use |weak_factory_|?

Not anymore; removed.

[jbroman, 2016-11-03 19:28:04]

https://codereview.chromium.org/2469593002/diff/60001/extensions/renderer/api...
File extensions/renderer/api_event_handler.cc (right):

https://codereview.chromium.org/2469593002/diff/60001/extensions/renderer/api...
extensions/renderer/api_event_handler.cc:51:
data->event_data.insert(std::make_pair(event_name, std::move(emitter)));
On 2016/11/02 at 22:02:21, Devlin (slow) wrote:
> On 2016/11/02 19:50:14, jbroman wrote:
> > On 2016/11/02 at 00:48:29, Devlin wrote:
> > > Most places instead seem to do:
> > > 
> > > gin::CreateHandle(isolate, new EventEmitter())
> > > 
> > > However, it's unclear to me how the EventEmitter would be deleted in that
case
> > (if it would).  If it wouldn't, then this is fine (and in my mind
preferable,
> > since it ensures we don't leak the listeners).  If it would, then we have a
> > double-free that's not detected from the tests, and I'm confused. :)
> > 
> > It's perhaps not obvious from the comment in gin/wrappable.h, but
gin::Wrappable
> > objects are owned by their V8 wrapper objects. In particular, they register
> > themselves so that when their V8 wrapper is collected, they delete
themselves.
> > As a result, keeping a unique_ptr around is actually a double-free. I expect
> > your unit tests simply aren't running a GC.
> 
> is there a way to run a gc in a unittest so we can add it?  (Also addressed
double-free; see other comment.)

Yup,
isolate->RequestGarbageCollectionForTesting(v8::Isolate::kFullGarbageCollection).
Blink calls it 5 times (to make sure paths that require multiple GCs to collect
are dealt with).

see, e.g.,
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...

https://codereview.chromium.org/2469593002/diff/60001/extensions/renderer/api...
extensions/renderer/api_event_handler.cc:72: std::vector<v8::Local<v8::Value>>
v8_args;
On 2016/11/02 at 22:02:21, Devlin (slow) wrote:
> On 2016/11/02 19:50:14, jbroman wrote:
> > Just confirming that this is the behaviour you want (seems fine to me): if
any
> > of these is an object (including an array), one callback could modify it,
and
> > those modifications would be visible to subsequent callbacks in the same
> > context.
> 
> Good call!  I've checked and this is what we're currently doing, and given
it's within the same context (and thus the same extension), I think this is
probably fine.  But I've added a comment to make it clear.

Thanks!

https://codereview.chromium.org/2469593002/diff/80001/extensions/renderer/api...
File extensions/renderer/api_event_handler.cc (right):

https://codereview.chromium.org/2469593002/diff/80001/extensions/renderer/api...
extensions/renderer/api_event_handler.cc:34: event_data.clear();
Apologies for not being clear here. event_data itself will be cleared
automatically in the map's destructor. But the listener vectors in each event
emitter won't be cleared implicitly (because that object isn't destroyed here).
It's the handles from EventEmitter to v8::Function that needs to be cleared.

for (const auto& pair : event_data) {
  EventEmitter* emitter = nullptr;
  if (gin::Converter<EventEmitter*>::FromV8(isolate, pair.second, &emitter))
    emitter->listeners()->clear();
}

This will clear any cycles that may exist between |emitter| and one of its
listeners, which may be necessary to make the emitter collectible.

https://codereview.chromium.org/2469593002/diff/80001/extensions/renderer/api...
File extensions/renderer/api_event_handler.h (right):

https://codereview.chromium.org/2469593002/diff/80001/extensions/renderer/api...
extensions/renderer/api_event_handler.h:13: #include
"extensions/renderer/event_emitter.h"
super-nit: unused include; move to .cc?

https://codereview.chromium.org/2469593002/diff/80001/extensions/renderer/api...
extensions/renderer/api_event_handler.h:21: class Arguments;
nit: unused forward declaration (apologies for missing this earlier)

https://codereview.chromium.org/2469593002/diff/80001/extensions/renderer/eve...
File extensions/renderer/event_emitter.cc (right):

https://codereview.chromium.org/2469593002/diff/80001/extensions/renderer/eve...
extensions/renderer/event_emitter.cc:34: listeners_.push_back(
nit: probably safer to also check that the gin::PerContextData hasn't already
been destroyed (else a listener added here may be able to escape the cleanup
logic above)

https://codereview.chromium.org/2469593002/diff/80001/extensions/renderer/eve...
File extensions/renderer/event_emitter.h (right):

https://codereview.chromium.org/2469593002/diff/80001/extensions/renderer/eve...
extensions/renderer/event_emitter.h:10: #include "gin/handle.h"
super-nit: since you ended up calling CreateHandle in the caller instead of in
this class, this include seems unused here

[Devlin, 2016-11-04 17:59:30]

https://codereview.chromium.org/2469593002/diff/60001/extensions/renderer/api...
File extensions/renderer/api_event_handler.cc (right):

https://codereview.chromium.org/2469593002/diff/60001/extensions/renderer/api...
extensions/renderer/api_event_handler.cc:51:
data->event_data.insert(std::make_pair(event_name, std::move(emitter)));
On 2016/11/03 19:28:04, jbroman wrote:
> On 2016/11/02 at 22:02:21, Devlin (slow) wrote:
> > On 2016/11/02 19:50:14, jbroman wrote:
> > > On 2016/11/02 at 00:48:29, Devlin wrote:
> > > > Most places instead seem to do:
> > > > 
> > > > gin::CreateHandle(isolate, new EventEmitter())
> > > > 
> > > > However, it's unclear to me how the EventEmitter would be deleted in
that
> case
> > > (if it would).  If it wouldn't, then this is fine (and in my mind
> preferable,
> > > since it ensures we don't leak the listeners).  If it would, then we have
a
> > > double-free that's not detected from the tests, and I'm confused. :)
> > > 
> > > It's perhaps not obvious from the comment in gin/wrappable.h, but
> gin::Wrappable
> > > objects are owned by their V8 wrapper objects. In particular, they
register
> > > themselves so that when their V8 wrapper is collected, they delete
> themselves.
> > > As a result, keeping a unique_ptr around is actually a double-free. I
expect
> > > your unit tests simply aren't running a GC.
> > 
> > is there a way to run a gc in a unittest so we can add it?  (Also addressed
> double-free; see other comment.)
> 
> Yup,
>
isolate->RequestGarbageCollectionForTesting(v8::Isolate::kFullGarbageCollection).
> Blink calls it 5 times (to make sure paths that require multiple GCs to
collect
> are dealt with).
> 
> see, e.g.,
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...

Nifty, thanks!

https://codereview.chromium.org/2469593002/diff/80001/extensions/renderer/api...
File extensions/renderer/api_event_handler.cc (right):

https://codereview.chromium.org/2469593002/diff/80001/extensions/renderer/api...
extensions/renderer/api_event_handler.cc:34: event_data.clear();
On 2016/11/03 19:28:04, jbroman wrote:
> Apologies for not being clear here. event_data itself will be cleared
> automatically in the map's destructor. But the listener vectors in each event
> emitter won't be cleared implicitly (because that object isn't destroyed
here).
> It's the handles from EventEmitter to v8::Function that needs to be cleared.
> 
> for (const auto& pair : event_data) {
>   EventEmitter* emitter = nullptr;
>   if (gin::Converter<EventEmitter*>::FromV8(isolate, pair.second, &emitter))
>     emitter->listeners()->clear();
> }
> 
> This will clear any cycles that may exist between |emitter| and one of its
> listeners, which may be necessary to make the emitter collectible.

Whoops!  I understood what you meant, and somewhere along the line way over
simplified it.  Fixed now.

In your snippet, you guard the conversion failure in an if - when could this
reasonably fail?  Would a (D)CHECK be better?  (And if so, would it be better at
other places we are converting, like 101 and 124?)

https://codereview.chromium.org/2469593002/diff/80001/extensions/renderer/api...
File extensions/renderer/api_event_handler.h (right):

https://codereview.chromium.org/2469593002/diff/80001/extensions/renderer/api...
extensions/renderer/api_event_handler.h:13: #include
"extensions/renderer/event_emitter.h"
On 2016/11/03 19:28:04, jbroman wrote:
> super-nit: unused include; move to .cc?

Done.

https://codereview.chromium.org/2469593002/diff/80001/extensions/renderer/api...
extensions/renderer/api_event_handler.h:21: class Arguments;
On 2016/11/03 19:28:04, jbroman wrote:
> nit: unused forward declaration (apologies for missing this earlier)

Done.

https://codereview.chromium.org/2469593002/diff/80001/extensions/renderer/eve...
File extensions/renderer/event_emitter.cc (right):

https://codereview.chromium.org/2469593002/diff/80001/extensions/renderer/eve...
extensions/renderer/event_emitter.cc:34: listeners_.push_back(
On 2016/11/03 19:28:04, jbroman wrote:
> nit: probably safer to also check that the gin::PerContextData hasn't already
> been destroyed (else a listener added here may be able to escape the cleanup
> logic above)

Done.  It's a tiny shame since EventEmitter otherwise doesn't know it exists on
the PerContextData, but I think that's probably fine.

Right now, I'm getting the context from arguments->holder->CreationContext() -
is that the right way to get the current context?  (Rather than e.g.
arguments->isolate->GetCurrentContext())

https://codereview.chromium.org/2469593002/diff/80001/extensions/renderer/eve...
File extensions/renderer/event_emitter.h (right):

https://codereview.chromium.org/2469593002/diff/80001/extensions/renderer/eve...
extensions/renderer/event_emitter.h:10: #include "gin/handle.h"
On 2016/11/03 19:28:04, jbroman wrote:
> super-nit: since you ended up calling CreateHandle in the caller instead of in
> this class, this include seems unused here

Done.

https://codereview.chromium.org/2469593002/diff/100001/extensions/renderer/ap...
File extensions/renderer/api_event_handler_unittest.cc (right):

https://codereview.chromium.org/2469593002/diff/100001/extensions/renderer/ap...
extensions/renderer/api_event_handler_unittest.cc:44: gin::V8Test::TearDown();
After this point, the context and PerContextData should be released - is there a
v8 way to check if anything is leaked?  Or would ASAN pick it up?

[jbroman, 2016-11-04 18:33:04]

lgtm

https://codereview.chromium.org/2469593002/diff/80001/extensions/renderer/api...
File extensions/renderer/api_event_handler.cc (right):

https://codereview.chromium.org/2469593002/diff/80001/extensions/renderer/api...
extensions/renderer/api_event_handler.cc:34: event_data.clear();
On 2016/11/04 at 17:59:30, Devlin (slow) wrote:
> On 2016/11/03 19:28:04, jbroman wrote:
> > Apologies for not being clear here. event_data itself will be cleared
> > automatically in the map's destructor. But the listener vectors in each
event
> > emitter won't be cleared implicitly (because that object isn't destroyed
here).
> > It's the handles from EventEmitter to v8::Function that needs to be cleared.
> > 
> > for (const auto& pair : event_data) {
> >   EventEmitter* emitter = nullptr;
> >   if (gin::Converter<EventEmitter*>::FromV8(isolate, pair.second, &emitter))
> >     emitter->listeners()->clear();
> > }
> > 
> > This will clear any cycles that may exist between |emitter| and one of its
> > listeners, which may be necessary to make the emitter collectible.
> 
> Whoops!  I understood what you meant, and somewhere along the line way over
simplified it.  Fixed now.
> 
> In your snippet, you guard the conversion failure in an if - when could this
reasonably fail?  Would a (D)CHECK be better?  (And if so, would it be better at
other places we are converting, like 101 and 124?)

No, it shouldn't fail. CHECK or DCHECK sgtm.

https://codereview.chromium.org/2469593002/diff/80001/extensions/renderer/eve...
File extensions/renderer/event_emitter.cc (right):

https://codereview.chromium.org/2469593002/diff/80001/extensions/renderer/eve...
extensions/renderer/event_emitter.cc:34: listeners_.push_back(
On 2016/11/04 at 17:59:30, Devlin (slow) wrote:
> On 2016/11/03 19:28:04, jbroman wrote:
> > nit: probably safer to also check that the gin::PerContextData hasn't
already
> > been destroyed (else a listener added here may be able to escape the cleanup
> > logic above)
> 
> Done.  It's a tiny shame since EventEmitter otherwise doesn't know it exists
on the PerContextData, but I think that's probably fine.

Agreed, but that's the cost of relying on the PerContextData to ensure lifetime
correctness here.

> Right now, I'm getting the context from arguments->holder->CreationContext() -
is that the right way to get the current context?  (Rather than e.g.
arguments->isolate->GetCurrentContext())

arguments->holder->CreationContext() is perfect.

https://codereview.chromium.org/2469593002/diff/100001/extensions/renderer/ap...
File extensions/renderer/api_event_handler_unittest.cc (right):

https://codereview.chromium.org/2469593002/diff/100001/extensions/renderer/ap...
extensions/renderer/api_event_handler_unittest.cc:44: gin::V8Test::TearDown();
On 2016/11/04 at 17:59:30, Devlin (slow) wrote:
> After this point, the context and PerContextData should be released - is there
a v8 way to check if anything is leaked?  Or would ASAN pick it up?

That's a good question. I'm not aware of a general way to check this (and I
don't think LSAN would catch it, but I'm not 100% sure about that). I think we
usually find this sort of thing in a long-running test (or real world use) when
memory usage increases without bound.

I _think_ this would work, but someone else might know better:
- take a global handle to the context and set weak
- destroy the context holder
- have the isolate do a GC of the heap
- check that the handle to the context is null (i.e. the context was not
retained by any objects created in that context)

Could ask v8-users, platform-architecture-dev or similar if anyone else has
suggestions. I'm OK with landing without an answer to this, though.

[Devlin, 2016-11-04 20:21:20]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-04 20:22:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-04 20:24:53]

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

[commit-bot: I haz the power, 2016-11-04 20:24:54]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Devlin, 2016-11-05 01:16:55]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-05 01:17:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2016-11-05 01:24:19]

https://codereview.chromium.org/2469593002/diff/100001/extensions/renderer/ap...
File extensions/renderer/api_event_handler_unittest.cc (right):

https://codereview.chromium.org/2469593002/diff/100001/extensions/renderer/ap...
extensions/renderer/api_event_handler_unittest.cc:44: gin::V8Test::TearDown();
On 2016/11/04 18:33:04, jbroman wrote:
> On 2016/11/04 at 17:59:30, Devlin (slow) wrote:
> > After this point, the context and PerContextData should be released - is
there
> a v8 way to check if anything is leaked?  Or would ASAN pick it up?
> 
> That's a good question. I'm not aware of a general way to check this (and I
> don't think LSAN would catch it, but I'm not 100% sure about that). I think we
> usually find this sort of thing in a long-running test (or real world use)
when
> memory usage increases without bound.
> 
> I _think_ this would work, but someone else might know better:
> - take a global handle to the context and set weak
> - destroy the context holder
> - have the isolate do a GC of the heap
> - check that the handle to the context is null (i.e. the context was not
> retained by any objects created in that context)
> 
> Could ask v8-users, platform-architecture-dev or similar if anyone else has
> suggestions. I'm OK with landing without an answer to this, though.

Great idea!  That worked - passes as-is and fails if we remove the listener
clean up.

[commit-bot: I haz the power, 2016-11-05 03:12:16]

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

[commit-bot: I haz the power, 2016-11-05 03:12:17]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jbroman, 2016-11-05 20:55:56]

still lgtm

https://codereview.chromium.org/2469593002/diff/140001/extensions/renderer/ap...
File extensions/renderer/api_event_handler_unittest.cc (right):

https://codereview.chromium.org/2469593002/diff/140001/extensions/renderer/ap...
extensions/renderer/api_event_handler_unittest.cc:36: // '5' is a magic number
stolen from Blink; arbitrarily large enough to
nit: move this comment down next to the loop it describes

[Devlin, 2016-11-07 17:49:51]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-07 17:50:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-07 18:47:51]

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

[commit-bot: I haz the power, 2016-11-07 18:47:52]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-11-07 19:17:31]

Note: As discussed offline, copy-pasted gc code to different tests; cleaning up
by making a dedicated class in a followup.

https://codereview.chromium.org/2469593002/diff/140001/extensions/renderer/ap...
File extensions/renderer/api_event_handler_unittest.cc (right):

https://codereview.chromium.org/2469593002/diff/140001/extensions/renderer/ap...
extensions/renderer/api_event_handler_unittest.cc:36: // '5' is a magic number
stolen from Blink; arbitrarily large enough to
On 2016/11/05 20:55:55, jbroman wrote:
> nit: move this comment down next to the loop it describes

Whoops!  Done.

[Devlin, 2016-11-07 19:17:46]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2016-11-07 19:17:46]

The patchset sent to the CQ was uploaded after l-g-t-m from jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2469593002/#ps160001
(title: "asantest")

[commit-bot: I haz the power, 2016-11-07 19:18:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-07 19:23:19]

Committed patchset #8 (id:160001)

[Devlin, 2016-11-07 19:29:29]

https://codereview.chromium.org/2469593002/diff/140001/extensions/renderer/ap...
File extensions/renderer/api_event_handler_unittest.cc (right):

https://codereview.chromium.org/2469593002/diff/140001/extensions/renderer/ap...
extensions/renderer/api_event_handler_unittest.cc:36: // '5' is a magic number
stolen from Blink; arbitrarily large enough to
On 2016/11/07 19:17:30, Devlin (slow) wrote:
> On 2016/11/05 20:55:55, jbroman wrote:
> > nit: move this comment down next to the loop it describes
> 
> Whoops!  Done.

In a case of the Mondays, I did this, and then left the file open and didn't
save it.  So the copy-pasted versions are fine, but this one remains in the
wrong spot.  Will fix pronto.

[commit-bot: I haz the power, 2016-11-07 19:35:10]

Description was changed from

==========
[Extensions Bindings] Add Events support

Add an APIEventHandler that provides the ability to both create
v8::Objects for extension API events (and provides the implementation
for the event functions like hasListener()) and can dispatch events to
specific contexts.

Add specific event tests and update others to ensure it is wired in
correctly.

BUG=653596
==========

to

==========
[Extensions Bindings] Add Events support

Add an APIEventHandler that provides the ability to both create
v8::Objects for extension API events (and provides the implementation
for the event functions like hasListener()) and can dispatch events to
specific contexts.

Add specific event tests and update others to ensure it is wired in
correctly.

BUG=653596

Committed: https://crrev.com/9a0f04b02bd0d7c5d9156cd7155e9994caf19d71
Cr-Commit-Position: refs/heads/master@{#430340}
==========

[commit-bot: I haz the power, 2016-11-07 19:35:11]

Patchset 8 (id:??) landed as
https://crrev.com/9a0f04b02bd0d7c5d9156cd7155e9994caf19d71
Cr-Commit-Position: refs/heads/master@{#430340}