Page MenuHomeWildfire Games

Lobby ScriptConversion to remove stanza property name hardcoding and duplication
Needs ReviewPublic

Authored by elexis on Nov 9 2018, 4:53 PM.

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Trac Tickets
#5340
Summary

Whenever the JS lobby developers wanted to show a new property in the gamelist stanza, the XmppClient.cpp file had to be updated with a hardcoded list of property names.
But these properties can be read out directly, there is no reason to hardcode them.
After the removal of hardcoded property names, the functions are identical, and the remains are a ScriptConversion function as known from GuiScriptConversions.cpp, EngineScriptConversions.cpp, ScriptConversions.cpp etc.
The "push" and Eval calls are quite ugly too, see #5340.

Test Plan

Make sure that the JS side doesn't rely on all properties being given.
If the stanza must contain all property names, that should be ensured in some place that isn't code-logic.

Event Timeline

elexis created this revision.Nov 9 2018, 4:53 PM
Vulcan added a subscriber: Vulcan.Nov 9 2018, 4:56 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/differential/769/

elexis updated this revision to Diff 6971.Nov 9 2018, 5:00 PM
elexis marked an inline comment as done.

Missing new file

Stan added a subscriber: Stan.Nov 9 2018, 5:33 PM

Looks good. I was wondering about those Eval as well when I was doing the line color patch.

Stan added a reviewer: Restricted Owners Package.Nov 9 2018, 5:35 PM
Vulcan added a comment.Nov 9 2018, 5:44 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/770/

elexis marked 2 inline comments as done.Nov 14 2018, 12:26 PM

Notice all of this and the converse conversion work very well this way as long as we only work with attributes.

But we should probably use children instead of injecting JSON for the players and mods of the gamelist stanza, refs
rP18534 anymore. (https://trac.wildfiregames.com/ticket/3476#comment:11, irc 2016-06-15, 2016-06-16, 2016-09-02)
So perhaps the generic JS<->Stanza conversion approach must be dumped, as gloox Tag children (named) are not really the same as array items (no name).
XpartaMuPP also needs changes to work with children.

source/lobby/IXmppClient.h
55

refs D1668. If RVO doesnt work here, it should be changed for the other JSI too

This is what a generic JS<->gloox:Tag converison would look like, revealing the array problem:

template<> bool ScriptInterface::FromJSVal<glooxwrapper::Tag>(JSContext* cx, JS::HandleValue value, glooxwrapper::Tag& tag)
{
	JSAutoRequest rq(cx);

	if (!value.isObject())
		FAIL("the given value is not an object!");

	JS::RootedObject object(cx, &value.toObject());
	JS::AutoIdArray properties(cx, JS_Enumerate(cx, object));
	if (!properties)
		FAIL("the properties of the given object could not be enumerated!");

	for (size_t i = 0; i < properties.length(); ++i)
	{
		JS::RootedId propertyID(cx, properties[i]);
		JS::RootedValue propertyIDValue(cx);

		if (!JS_IdToValue(cx, propertyID, &propertyIDValue))
			FAIL("a property ID of the given object could not be converted to a value!");

		std::string propertyName;

		// TODO: numeric properties
		if (JS_IsArrayObject(cx, object))
			propertyName = "item";// + propertyName;
		else if (!ScriptInterface::FromJSVal(cx, propertyIDValue, propertyName))
			FAIL("a property ID value of the given object could not be converted to a string!");

		debug_printf("prop %s\n", propertyName.c_str());

		JS::RootedValue propertyValue(cx);
		if (!JS_GetPropertyById(cx, object, propertyID, &propertyValue))
			FAIL("a property value could not be read!");

		switch (JS_TypeOfValue(cx, propertyValue))
		{
		case JSTYPE_NUMBER: // fall through
		case JSTYPE_STRING:
		{
			std::string propertyValueString;
			if (!ScriptInterface::FromJSVal(cx, propertyValue, propertyValueString))
				FAIL("a property string could not be read!");

			if (JS_IsArrayObject(cx, object))
			{
				glooxwrapper::Tag* childTag = glooxwrapper::Tag::allocate("item");
				if (!ScriptInterface::FromJSVal(cx, propertyValue, *childTag))
					FAIL("a property that is a string could not be added as an array item!");

				tag.addChild(childTag);
			}
			else
				tag.addAttribute(glooxwrapper::string(propertyName), glooxwrapper::string(propertyValueString));

			break;
		}
		case JSTYPE_OBJECT:
		{
			glooxwrapper::Tag* childTag = glooxwrapper::Tag::allocate("item");
			if (!ScriptInterface::FromJSVal(cx, propertyValue, *childTag))
				FAIL("a property that is an object could not be read!");

			tag.addChild(childTag);
			break;
		}
		case JSTYPE_BOOLEAN:
			FAIL("TODO boolean");
		case JSTYPE_VOID:
			FAIL("TODO undefined");
		case JSTYPE_NULL:
			FAIL("TODO null");
		case JSTYPE_FUNCTION:
			FAIL("a property value is a function!");
		}
	}

	return true;
}

It's also intended by SleekXMPP that stanzas are parsed specifically. One needs to specify in advance which child elements (arrays) can be specified before they can be read.

vladislavbelov added inline comments.
source/lobby/IXmppClient.h
55

Shouldn't the D1668 be closed then?

source/lobby/LobbyScriptConversions.cpp
1 ↗(On Diff #6971)

Usually we put *ScriptConversions files into scripting subfolder, example: simulation2/scripting/EngineScriptConversions.cpp.

26 ↗(On Diff #6971)

The pretty long line, >120 symbols. Would it be better to format it a little bit? Like this:

template<>
void ScriptInterface::ToJSVal<GlooxTagList>(
	JSContext* cx, JS::MutableHandleValue ret, const GlooxTagList& tagList)
31 ↗(On Diff #6971)

Wrong type here, JS_SetElement accepts uint32_t: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_SetElement. So the type have to be u32.

source/lobby/XmppClient.cpp
548

u32.

source/lobby/XmppClient.h
30

If using is allowed, we need to use using instead of typedef.

source/lobby/scripting/JSInterface_Lobby.cpp
190

Here and below: operator −> has a higher priority than the *, so parenthesis are not needed. But it's not a problem I think.

elexis planned changes to this revision.Nov 15 2018, 11:57 AM
elexis marked 5 inline comments as done.

(To repeat) The current patch and the patch posted in the comments conceptually cannot work with child elements, while child elements should be used to remove the JSON encoded arrays in stanzas.

source/lobby/IXmppClient.h
55

Every source I checked says that RVO works well for strings, but I didn't see anything about JS::Values.

it should be changed for the other JSI too

such as JSInterface_VisualReplay which returns a JS::Value instead of setting a MutableHandleValue.

source/lobby/LobbyScriptConversions.cpp
1 ↗(On Diff #6971)

ah, yess

31 ↗(On Diff #6971)

yes

source/lobby/XmppClient.h
30

using supports templates whereas typedefdoesn't, or where is the difference? And then using is preferred because every typedef can become using, so the entire codebase uses less vocabulary if every typedef becomes using, or what is the reasoning?

elexis updated this revision to Diff 9733.Sep 13 2019, 4:05 AM

Rebase following rP22891 and 2019.

Adding support for child tags would really be great to remove the JSON, but will need some invention.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/147/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/lobby/XmppClient.h
|  24| 
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceStunClient{' is invalid C code. Use --std or --language to configure the language.

source/lobby/glooxwrapper/glooxwrapper.h
|  88| namespace·glooxwrapper
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceglooxwrapper{' is invalid C code. Use --std or --language to configure the language.

source/lobby/IXmppClient.h
|  24| namespace·StunClient·{
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceStunClient{' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/657/display/redirect

Since this guarantee the properties to exist anymore (wicked client inserting prepared game), the JS code needs to account for that, meh.
Still seems better to do that in JS than hardcoding all JS properties in C++.
Each time JS side wanted to add a new property, it had to change C++ code.
Mods would be allowed to pass their own JS, without having to hijack one of the existing properties.
(The mods can already send the custom properties, they just can't be received solely due to the C++ hardcoding.)
For example maxnbp in the afore mentioned rP18534.

source/lobby/XmppClient.cpp
604

Notice that the entire function could be removed if we would allow ourselves to bind JSInterface_ to XmppClient class instead of the IXmppClient interface. It would mean that people who want to implement IXmppClient with a different backend to gloox / glooxwrapper would have to provide their own interface as well.

The functions would then be registered in the implementation class of the interface class, so that would be easy to forget or get out of sync as it would be removed from the interface IXmppClass. Shame, silly proxy getter.

source/lobby/XmppClient.h
190–191

Gloox->Glooxwrapper

source/lobby/glooxwrapper/glooxwrapper.h
594

Returning gloox tags sounds to defeat the purpose of the glooxwrapper, should wrap the tags.

From gloox tag.h:

typedef std::list<Attribute*> AttributeList;

We only need the keys and I dont want to mirror / wrap the entire Attribute class now, -> attributeNames()

source/lobby/scripting/GlooxScriptConversions.cpp
62

TOJSVAL_VECTOR(const glooxwrapper::Tag*) (needs macro split)

76

Would be shorter with ScriptInterface::SetProperty due to the implicit ToJSVal conversion.
However obtaining the ScriptInterface instance via pcxPrivate or whatever it was was measurably slightly slower (see benchmark in D2128).
SetProperty might become static as well.
(Perhaps some specific conversion from CreateObject and SetProperty needs m_CustomObjectTypes or a different ScriptInterface member eventually (CreateCustomObject, for example CreateCustomObject("GUIObject"), but those are usually owned by C++.). It doesnt seem so problematic to change between static and member function as long as the code isnt bugged in random calls thereof.)

elexis updated this revision to Diff 9759.Sep 14 2019, 1:54 PM

Everything except JS changes.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/168/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/682/display/redirect