From 386242d9c72c3f15d21840bbeb297b45807c5f41 Mon Sep 17 00:00:00 2001 From: ksuprynowicz Date: Sun, 15 Jan 2023 15:58:21 +0100 Subject: [PATCH] Fixed adding properties to methods --- .../src/v8/ScriptObjectV8Proxy.cpp | 27 ++++++++++- .../src/v8/ScriptObjectV8Proxy.h | 2 +- .../src/v8/ScriptValueV8Wrapper.cpp | 46 ++++++++++++++++++- tests/script-engine/src/ScriptEngineTests.cpp | 4 +- tests/script-engine/src/tests/004_require.js | 8 +++- 5 files changed, 80 insertions(+), 7 deletions(-) diff --git a/libraries/script-engine/src/v8/ScriptObjectV8Proxy.cpp b/libraries/script-engine/src/v8/ScriptObjectV8Proxy.cpp index cb77f87639..e71030012d 100644 --- a/libraries/script-engine/src/v8/ScriptObjectV8Proxy.cpp +++ b/libraries/script-engine/src/v8/ScriptObjectV8Proxy.cpp @@ -297,11 +297,23 @@ void ScriptObjectV8Proxy::investigate() { } v8::Local v8Object = objectTemplate->NewInstance(_engine->getContext()).ToLocalChecked(); + v8Object->SetAlignedPointerInInternalField(0, const_cast(internalPointsToQObjectProxy)); v8Object->SetAlignedPointerInInternalField(1, reinterpret_cast(this)); - v8Object->SetInternalField(2, v8::Object::New(_engine->getIsolate())); - + // Properties added later will be stored in this object + v8::Local propertiesObject = v8::Object::New(_engine->getIsolate()); + v8Object->SetInternalField(2, propertiesObject); _v8Object.Reset(_engine->getIsolate(), v8Object); + + // Add all the methods objects as properties - this allows adding properties to a given method later. Is used by Script.request. + // V8TODO: Should these be deleted when the script-owned object is destroyed? It needs checking if script-owned objects will be garbage-collected, or will self-referencing prevent it. + for (auto i = _methods.begin(); i != _methods.end(); i++) { + V8ScriptValue method = ScriptMethodV8Proxy::newMethod(_engine, qobject, V8ScriptValue(_engine->getIsolate(), v8Object), i.value().methods, i.value().numMaxParms); + if(!propertiesObject->Set(_engine->getContext(), i.value().name.constGet(), method.get()).FromMaybe(false)) { + Q_ASSERT(false); + } + } + } QString ScriptObjectV8Proxy::name() const { @@ -461,6 +473,16 @@ V8ScriptValue ScriptObjectV8Proxy::property(const V8ScriptValue& object, const V qDebug(scriptengine) << "Method with QMetaType::UnknownType " << metaObject->className() << " " << (*iter).name(); } } //V8TODO: is new method created during every call? It needs to be cached instead + bool isMethodDefined = false; + v8::Local property; + if(_v8Object.Get(_engine->getIsolate())->GetInternalField(2).As()->Get(_engine->getContext(), name.constGet()).ToLocal(&property)) { + if (!property->IsUndefined()) { + qDebug() << "Using existing method object"; + return V8ScriptValue(_engine->getIsolate(), property); + } + } + Q_ASSERT(false); + qDebug(scriptengine) << "(This should not happen) Creating new method object for " << metaObject->className() << " " << name.toQString(); return ScriptMethodV8Proxy::newMethod(_engine, qobject, object, methodDef.methods, methodDef.numMaxParms); } case SIGNAL_TYPE: { @@ -479,6 +501,7 @@ V8ScriptValue ScriptObjectV8Proxy::property(const V8ScriptValue& object, const V ScriptEngine::QObjectWrapOptions options = ScriptEngine::ExcludeSuperClassContents | //V8TODO ScriptEngine::ExcludeDeleteLater | ScriptEngine::PreferExistingWrapperObject; + //V8TODO: why is it returning new object every time? return ScriptObjectV8Proxy::newQObject(_engine, proxy, ScriptEngine::ScriptOwnership, options); //return _engine->newQObject(proxy, ScriptEngine::ScriptOwnership, options); } diff --git a/libraries/script-engine/src/v8/ScriptObjectV8Proxy.h b/libraries/script-engine/src/v8/ScriptObjectV8Proxy.h index 165152648d..5bd4f48336 100644 --- a/libraries/script-engine/src/v8/ScriptObjectV8Proxy.h +++ b/libraries/script-engine/src/v8/ScriptObjectV8Proxy.h @@ -112,7 +112,7 @@ private: // storage QPointer _object; // V8TODO Is this necessary? v8::UniquePersistent _v8ObjectTemplate; - // V8TODO Maybe it doesn't really need to point to itsef? + // Handle for its own object v8::UniquePersistent _v8Object; int pointerCorruptionTest = 12345678; diff --git a/libraries/script-engine/src/v8/ScriptValueV8Wrapper.cpp b/libraries/script-engine/src/v8/ScriptValueV8Wrapper.cpp index a5ac13f62d..6ca8409aa2 100644 --- a/libraries/script-engine/src/v8/ScriptValueV8Wrapper.cpp +++ b/libraries/script-engine/src/v8/ScriptValueV8Wrapper.cpp @@ -155,6 +155,39 @@ ScriptValue ScriptValueV8Wrapper::construct(const ScriptValue& arguments) { } ScriptValue ScriptValueV8Wrapper::data() const { + auto isolate = _engine->getIsolate(); + v8::Locker locker(isolate); + v8::Isolate::Scope isolateScope(isolate); + v8::HandleScope handleScope(isolate); + v8::Context::Scope contextScope(_engine->getContext()); + // Private properties are an experimental feature for now on V8, so we are using regular value for now + if (_value.constGet()->IsObject()) { + auto v8Object = v8::Local::Cast(_value.constGet()); + v8::Local data; + //bool createData = false; + if (!v8Object->Get(_engine->getContext(), v8::String::NewFromUtf8(isolate, "__data").ToLocalChecked()).ToLocal(&data)) { + data = v8::Undefined(isolate); + //createData = true; + } /*else { + if (data->IsUndefined()) { + createData = true; + } + } + if (createData) { + qDebug() << "ScriptValueV8Wrapper::data(): Data object doesn't exist, creating new one"; + // Create data object if it's non-existent or invalid + data = v8::Object::New(isolate); + if( !v8Object->Set(_engine->getContext(), v8::String::NewFromUtf8(isolate, "__data").ToLocalChecked(), data).FromMaybe(false)) { + qDebug() << "ScriptValueV8Wrapper::data(): Data object couldn't be created"; + Q_ASSERT(false); + } + }*/ + V8ScriptValue result(isolate, data); + return ScriptValue(new ScriptValueV8Wrapper(_engine, std::move(result))); + } else { + qDebug() << "ScriptValueV8Wrapper::data() was called on a value that is not an object"; + Q_ASSERT(false); + } //V8TODO I'm not sure how this would work in V8 //V8ScriptValue result = _value.data(); //return ScriptValue(new ScriptValueV8Wrapper(_engine, std::move(result))); @@ -255,7 +288,18 @@ void ScriptValueV8Wrapper::setData(const ScriptValue& value) { v8::HandleScope handleScope(isolate); v8::Context::Scope contextScope(_engine->getContext()); V8ScriptValue unwrapped = fullUnwrap(value); - (**_value.get()) = (**unwrapped.get()); + // V8TODO Check if it uses same isolate. Would pointer check be enough? + // Private properties are an experimental feature for now on V8, so we are using regular value for now + if (_value.constGet()->IsObject()) { + auto v8Object = v8::Local::Cast(_value.constGet()); + if( !v8Object->Set(_engine->getContext(), v8::String::NewFromUtf8(isolate, "__data").ToLocalChecked(), unwrapped.constGet()).FromMaybe(false)) { + qDebug() << "ScriptValueV8Wrapper::data(): Data object couldn't be created"; + Q_ASSERT(false); + } + } else { + qDebug() << "ScriptValueV8Wrapper::data() was called on a value that is not an object"; + Q_ASSERT(false); + } } void ScriptValueV8Wrapper::setProperty(const QString& name, const ScriptValue& value, const ScriptValue::PropertyFlags& flags) { diff --git a/tests/script-engine/src/ScriptEngineTests.cpp b/tests/script-engine/src/ScriptEngineTests.cpp index 9a55d62e47..249f92692d 100644 --- a/tests/script-engine/src/ScriptEngineTests.cpp +++ b/tests/script-engine/src/ScriptEngineTests.cpp @@ -87,7 +87,7 @@ void ScriptEngineTests::scriptTest() { qInfo() << "Running test script: " << script; ac->loadOneScript(script); }*/ - ac->loadOneScript("tests/003_vector_math.js"); + //ac->loadOneScript("tests/003_vector_math.js"); ac->loadOneScript("tests/004_require.js"); qDebug() << ac->getRunning(); @@ -95,7 +95,7 @@ void ScriptEngineTests::scriptTest() { // TODO: if I don't have infinite loop here, it exits before scripts finish. It also reports: QSignalSpy: No such signal: 'scriptCountChanged' for (int n = 0; n > -1; n++) { QSignalSpy spy(ac.get(), SIGNAL(scriptCountChanged)); - spy.wait(1000); + spy.wait(100000); qDebug() << "Signal happened"; } //spy.wait(5000); diff --git a/tests/script-engine/src/tests/004_require.js b/tests/script-engine/src/tests/004_require.js index 1cd0a481c1..6ed23d4a9e 100644 --- a/tests/script-engine/src/tests/004_require.js +++ b/tests/script-engine/src/tests/004_require.js @@ -1,3 +1,9 @@ +print("Script.require: " + JSON.stringify(Script.require)); +print("Script.require.cache: " + JSON.stringify(Script.require.cache)); +print(JSON.stringify("Script.require.test_prop before defining: " + Script.require.test_prop)); +Script.require.test_prop = "test property"; +print(JSON.stringify("Script.require.test_prop after defining: " + Script.require.test_prop)); print("Before require"); -Script.require("001_test_print.js"); +// TODO: find correct local path for the test module +Script.require("http://oaktown.pl/scripts/001_test_print.js"); print("After require");