Adds a helper, because GetProperty still returns true, even if the property doesn't exist, what makes writing of HasPropery, GetProperty, FromJsVal too annoying.
It'd be useful for the CinemaPath conversation patch D317.
Details
Details
- Reviewers
elexis wraitii - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) - Commits
- rP19421: Add FromJSProperty helper function that gets the property of a JS object, thus…
- Try to replace any object property getting
Diff Detail
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/775/ for more details.
Comment Actions
We also have MutableHandleObject, this would avoid repeating the isObject and toObject calls for each FromJSProperty call.
However converting a HandleValue to a JS Object should be instantaneously, so even if done repeatedly onTick there should be no performance cost attached to it
and we can't directly pass a RootedObject as a MutableHandleObject, so we wouldn't gain anything afaics.
I've tested this with the following diff and we can observe that the code becomes nicer:
Index: source/simulation2/scripting/EngineScriptConversions.cpp =================================================================== --- source/simulation2/scripting/EngineScriptConversions.cpp (revision 19418) +++ source/simulation2/scripting/EngineScriptConversions.cpp (working copy) @@ -87,23 +87,16 @@ template<> bool ScriptInterface::FromJSVal<CColor>(JSContext* cx, JS::HandleValue v, CColor& out) { - if (!v.isObject()) - FAIL("JS::HandleValue not an object"); + if (!FromJSProperty(cx, v, "r", out.r)) + FAIL("Failed to get property CColor.r"); - JSAutoRequest rq(cx); - JS::RootedObject obj(cx, &v.toObject()); + if (!FromJSProperty(cx, v, "g", out.g)) + FAIL("Failed to get property CColor.g"); - JS::RootedValue r(cx); - JS::RootedValue g(cx); - JS::RootedValue b(cx); - JS::RootedValue a(cx); - if (!JS_GetProperty(cx, obj, "r", &r) || !FromJSVal(cx, r, out.r)) - FAIL("Failed to get property CColor.r"); - if (!JS_GetProperty(cx, obj, "g", &g) || !FromJSVal(cx, g, out.g)) - FAIL("Failed to get property CColor.g"); - if (!JS_GetProperty(cx, obj, "b", &b) || !FromJSVal(cx, b, out.b)) + if (!FromJSProperty(cx, v, "b", out.b)) FAIL("Failed to get property CColor.b"); - if (!JS_GetProperty(cx, obj, "a", &a) || !FromJSVal(cx, a, out.a)) + + if (!FromJSProperty(cx, v, "a", out.a)) FAIL("Failed to get property CColor.a"); return true;
Notice if we change the property name of f.e. "r" to "BIGFAILURE" we will just get that, so the patch works.
We could add few exemplary changes like this diff, but you said you prefer to not add it in one place only, so we won't.
Thanks for the patch!
| source/scriptinterface/ScriptConversions.h | ||
|---|---|---|
| 89 ↗ | (On Diff #1265) | FromJSProperty Might have been nice to add some comment on what it does, but on the other hand that should be self-evident already. |
| 91 ↗ | (On Diff #1265) | JSAutoRequest can be done just before we do something with the context |
| 101 ↗ | (On Diff #1265) | could split that into two if's, so that we don't reserve the rooted value if !hasProperty, but doesn't matter really, since if it FAILs, the microsecond performance difference is irrelevant |
| 103 ↗ | (On Diff #1265) | whitespace in empty lines |
| 104 ↗ | (On Diff #1265) | Guess we don't return ScriptInterface::FromJSVal... so that it can be extended easily in the future without changing this line |