HomeWildfire Games

Support creating lobby GUI messages with arbitrary arguments instead of forcing…


Support creating lobby GUI messages with arbitrary arguments instead of forcing every message type into the same struct type, refs #4877 / rP19514 / D339.

Creates the GUI messages directly as JS::Values and removes the intermediary rigid struct.
Implementation similar to ScriptInterface::CreateObject from D2080.

Differential Revision: https://code.wildfiregames.com/D2264
Tested on: clang 8.0.1, Jenkins

Event Timeline

elexis added inline comments.Sep 9 2019, 10:23 AM


This removes wstring_from_utf8 for all GUI messages.

For some its unneeded, but the gloox strings are UTF encoded and must be decoded before creation of the JS value.

Reproduce for example by posting ä into the chat.

elexis added inline comments.Sep 9 2019, 3:38 PM

The banal fix would be to add wstring_from_utf8 in every string property of every CreateObject call.

That would make the code longer, but the alternative is to make the code even shorter, by specializing ToJSVal<glooxwrapper::string>, i.e. the to_string() would be removed in every line instead of adding a new call in every line.

Notice that ToJSVal specializations were already proposed in D1667 for other instances and we can use it for the other gloox properties that are converted to strings as well.

elexis added inline comments.Sep 11 2019, 11:59 AM

It seemed cleaner to only set "historic" for historic messages, furthermore it seemed to work since JS interprets it as false for chat messages, but JS complains about this being undefined for the kick messages. So it seems cleaner to always set this boolean as it was before rather than dealing with undefined or true in JS. Fix at D2282.

If one would apply this optimization:

diff --git a/binaries/data/mods/public/gui/lobby/lobby.js b/binaries/data/mods/public/gui/lobby/lobby.js
index 66a3de4d14..aaa4df4dbe 100644
--- a/binaries/data/mods/public/gui/lobby/lobby.js
+++ b/binaries/data/mods/public/gui/lobby/lobby.js
@@ -196,9 +196,7 @@ var g_NetMessageTypes = {
                "leave": msg => {
-                               "text": "/special " + sprintf(translate("%(nick)s has left."), {
-                                       "nick": msg.nick
-                               }),
+                               "text": "/special " + sprintf(translate("%(nick)s has left."), msg),
                                "time": msg.time,
                                "isSpecial": true

Following this commit it would trigger a SpiderMonkey 45 crash described in #5636.

But it seems like this is not a defect in this commit but a bug in SpiderMonkey that can be produced without networking code too:

	Object.defineProperty(this, "f", {
	    get: function() {
	        return (function() {

and this sprintf diff also would fix the crash if the first diff was applied:

diff --git a/src/sprintf.js b/src/sprintf.js
index ccb78d8..1ade05d 100644
--- a/src/sprintf.js
+++ b/src/sprintf.js
@@ -41,7 +41,7 @@
                 if (ph.keys) { // keyword argument
                     arg = argv[cursor]
                     for (k = 0; k < ph.keys.length; k++) {
-                        if (!arg.hasOwnProperty(ph.keys[k])) {
+                        if (arg[ph.keys[k]] === undefined) {
                             throw new Error(sprintf('[sprintf] property "%s" does not exist', ph.keys[k]))
                         arg = arg[ph.keys[k]]

hence use the ticket for this topic.