Bring back the ability to implement a C++ interface with a specific JavaScript object.

3 files changed, 121 insertions, 22 deletions

diff --git a/src/embind/embind.js b/src/embind/embind.js
index 0e3dfab1..2453a086 100644
--- a/src/embind/embind.js
+++ b/src/embind/embind.js
@@ -1830,10 +1830,11 @@ function __embind_create_inheriting_constructor(constructorName, wrapperType, pr
             throwBindingError("Pass correct 'this' to __construct");
         }
 
-        var inner = baseConstructor.__$implement.apply(
+        var inner = baseConstructor.implement.apply(
             undefined,
             [this].concat(arraySlice.call(arguments)));
         var $$ = inner.$$;
+        inner.notifyOnDestruction();
         $$.preservePointerOnDelete = true;
         Object.defineProperty(this, '$$', {
             value: $$
diff --git a/system/include/emscripten/bind.h b/system/include/emscripten/bind.h
index b2c83fc9..1be35826 100644
--- a/system/include/emscripten/bind.h
+++ b/system/include/emscripten/bind.h
@@ -879,13 +879,26 @@ namespace emscripten {
         };
     };
 
+
     ////////////////////////////////////////////////////////////////////////////////
     // CLASSES
     ////////////////////////////////////////////////////////////////////////////////
 
+    namespace internal {
+        class WrapperBase {
+        public:
+            void setNotifyJSOnDestruction(bool notify) {
+                notifyJSOnDestruction = notify;
+            }
+
+        protected:
+            bool notifyJSOnDestruction = false;
+        };
+    }
+
     // abstract classes
     template<typename T>
-    class wrapper : public T {
+    class wrapper : public T, public internal::WrapperBase {
     public:
         typedef T class_type;
 
@@ -906,22 +919,8 @@ namespace emscripten {
             return wrapped.call<ReturnType>(name, std::forward<Args>(args)...);
         }
 
-        template<typename ReturnType, typename... Args, typename Default>
-        ReturnType optional_call(const char* name, Default def, Args&&... args) const {
-            if (wrapped.has_implementation_defined_function<T>(name)) {
-                return call<ReturnType>(name, std::forward<Args>(args)...);
-            } else {
-                return def();
-            }
-        }
-
-        void setNotifyJSOnDestruction(bool notify) {
-            notifyJSOnDestruction = notify;
-        }
-
     private:
         val wrapped;
-        bool notifyJSOnDestruction = true;
     };
 
 #define EMSCRIPTEN_WRAPPER(T)                                           \
@@ -1165,15 +1164,15 @@ namespace emscripten {
             using namespace internal;
 
             auto cls = class_<WrapperType, base<ClassType>>(wrapperClassName)
+                .function("notifyOnDestruction", select_overload<void(WrapperType&)>([](WrapperType& wrapper) {
+                    wrapper.setNotifyJSOnDestruction(true);
+                }))
                 ;
             SmartPtrIfNeeded<PointerType> _(cls, pointerName);
 
             return
-                // rather than use an internal-yet-accessible
-                // constructor function, we could call something like
-                // _embind_register_wrapper_constructor
                 class_function(
-                    "__$implement",
+                    "implement",
                     &wrapped_new<PointerType, WrapperType, val, ConstructorArgs...>,
                     allow_raw_pointer<ret_val>())
                 .class_function(
diff --git a/tests/embind/embind.test.js b/tests/embind/embind.test.js
index b2c4c98b..ae421f27 100644
--- a/tests/embind/embind.test.js
+++ b/tests/embind/embind.test.js
@@ -1548,15 +1548,114 @@ module({
         });
     });
 
-    BaseFixture.extend("abstract methods", function() {
+    BaseFixture.extend("implementing abstract methods with JS objects", function() {
         test("can call abstract methods", function() {
             var obj = cm.getAbstractClass();
             assert.equal("from concrete", obj.abstractMethod());
             obj.delete();
         });
+
+        test("can implement abstract methods in JavaScript", function() {
+            var expected = "my JS string";
+            function MyImplementation() {
+                this.rv = expected;
+            }
+            MyImplementation.prototype.abstractMethod = function() {
+                return this.rv;
+            };
+
+            var impl = cm.AbstractClass.implement(new MyImplementation);
+            assert.equal(expected, impl.abstractMethod());
+            assert.equal(expected, cm.callAbstractMethod(impl));
+            impl.delete();
+        });
+
+        test("can implement optional methods in JavaScript", function() {
+            var expected = "my JS string";
+            function MyImplementation() {
+                this.rv = expected;
+            }
+            MyImplementation.prototype.optionalMethod = function() {
+                return this.rv;
+            };
+
+            var impl = cm.AbstractClass.implement(new MyImplementation);
+            // TODO: remove .implement() as a public API. It interacts poorly with Class.extend.
+            //assert.equal(expected, impl.optionalMethod(expected));
+            assert.equal(expected, cm.callOptionalMethod(impl, expected));
+            impl.delete();
+        });
+
+        test("if not implemented then optional method runs default", function() {
+            var impl = cm.AbstractClass.implement({});
+            assert.equal("optionalfoo", impl.optionalMethod("foo"));
+            // TODO: remove .implement() as a public API. It interacts poorly with Class.extend.
+            //assert.equal("optionalfoo", cm.callOptionalMethod(impl, "foo"));
+            impl.delete();
+        });
+
+        test("returning null shared pointer from interfaces implemented in JS code does not leak", function() {
+            var impl = cm.AbstractClass.implement({
+                returnsSharedPtr: function() {
+                    return null;
+                }
+            });
+            cm.callReturnsSharedPtrMethod(impl);
+            impl.delete();
+            // Let the memory leak test superfixture check that no leaks occurred.
+        });
+
+        test("returning a new shared pointer from interfaces implemented in JS code does not leak", function() {
+            var impl = cm.AbstractClass.implement({
+                returnsSharedPtr: function() {
+                    return cm.embind_test_return_smart_derived_ptr().deleteLater();
+                }
+            });
+            cm.callReturnsSharedPtrMethod(impl);
+            impl.delete();
+            // Let the memory leak test superfixture check that no leaks occurred.
+        });
+
+        test("void methods work", function() {
+            var saved = {};
+            var impl = cm.AbstractClass.implement({
+                differentArguments: function(i, d, f, q, s) {
+                    saved.i = i;
+                    saved.d = d;
+                    saved.f = f;
+                    saved.q = q;
+                    saved.s = s;
+                }
+            });
+
+            cm.callDifferentArguments(impl, 1, 2, 3, 4, "foo");
+
+            assert.deepEqual(saved, {
+                i: 1,
+                d: 2,
+                f: 3,
+                q: 4,
+                s: "foo",
+            });
+
+            impl.delete();
+        });
+
+        test("returning a cached new shared pointer from interfaces implemented in JS code does not leak", function() {
+            var derived = cm.embind_test_return_smart_derived_ptr();
+            var impl = cm.AbstractClass.implement({
+                returnsSharedPtr: function() {
+                    return derived;
+                }
+            });
+            cm.callReturnsSharedPtrMethod(impl);
+            impl.delete();
+            derived.delete();
+            // Let the memory leak test superfixture check that no leaks occurred.
+        });
     });
 
-    BaseFixture.extend("new-style class inheritance", function() {
+    BaseFixture.extend("constructor prototype class inheritance", function() {
         var Empty = cm.AbstractClass.extend("Empty", {
             abstractMethod: function() {
             }