[Extensions Bindings] Add Events support

extensions/renderer/api_event_handler.h

Index: extensions/renderer/api_event_handler.h
diff --git a/extensions/renderer/api_event_handler.h b/extensions/renderer/api_event_handler.h
new file mode 100644
index 0000000000000000000000000000000000000000..22fdddc0ad7f002eb6d81e735c8d72697a04e55e
--- /dev/null
+++ b/extensions/renderer/api_event_handler.h
@@ -0,0 +1,77 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef EXTENSIONS_RENDERER_API_EVENT_HANDLER_H_
+#define EXTENSIONS_RENDERER_API_EVENT_HANDLER_H_
+
+#include <map>
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "base/callback.h"
+#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+#include "base/supports_user_data.h"
+#include "extensions/renderer/api_binding_types.h"
+#include "extensions/renderer/event_emitter.h"
+#include "v8/include/v8.h"
+
+namespace base {
+class ListValue;
+}
+
+namespace gin {
+class Arguments;
+}
+
+namespace extensions {
+
+// The object to handle API events. This includes vending v8::Objects for the
+// event; handling adding, removing, and querying listeners; and firing events
+// to subscribed listeners. Designed to be used across JS contexts, but on a
+// single thread.
+class APIEventHandler {
+ public:
+  using HandlerCallback =

[jbroman, 2016/11/02 19:50:14]

unused?

[Devlin, 2016/11/02 22:02:21]

Removed.

+      base::Callback<void(v8::Local<v8::Object>, gin::Arguments*)>;
+
+  APIEventHandler(const binding::RunJSFunction& call_js);
+  ~APIEventHandler();
+
+  // Returns a new v8::Object for an event with the given |event_name|.
+  v8::Local<v8::Object> CreateEventInstance(const std::string& event_name,
+                                            v8::Local<v8::Context> context);
+
+  // Notifies all listeners of the event with the given |event_name| in the
+  // specified |context|, sending the included |arguments|.
+  void FireEventInContext(const std::string& event_name,
+                          v8::Local<v8::Context> context,
+                          const base::ListValue& arguments);
+
+  // Returns the EventListeners for a given |event_name| and |context|.
+  const EventEmitter::Listeners& GetEventListenersForTesting(

[jbroman, 2016/11/02 19:50:14]

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?

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

Sure, done.

+      const std::string& event_name,
+      v8::Local<v8::Context> context);
+
+ private:
+  struct APIEventPerContextData : public base::SupportsUserData::Data {

[jbroman, 2016/11/02 19:50:14]

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.)

[Devlin, 2016/11/02 22:02:21]

Done.

+    APIEventPerContextData();
+    ~APIEventPerContextData() override;
+
+    // A map from event name -> event emitter.
+    std::map<std::string, std::unique_ptr<EventEmitter>> event_data;

[jbroman, 2016/11/02 19:50:14]

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. :)

[Devlin, 2016/11/02 22:02:21]

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.

+  };
+
+  // Method to run a given v8::Function. Curried in for testing.
+  binding::RunJSFunction call_js_;
+
+  base::WeakPtrFactory<APIEventHandler> weak_factory_;

[jbroman, 2016/11/02 19:50:14]

Does anything use |weak_factory_|?

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

Not anymore; removed.

+
+  DISALLOW_COPY_AND_ASSIGN(APIEventHandler);
+};
+
+}  // namespace extensions
+
+#endif  // EXTENSIONS_RENDERER_API_EVENT_HANDLER_H_