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

Description

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

Details

Committed

elexis

Sep 6 2019, 7:14 AM

Differential Revision

D2264: Create lobby GUI messages with arbitrary arguments

Parents

rP22855: Drop lobby presence GUI messages altogether for better performance and less…

Branches

Unknown

Tags

Unknown

Build Status

Buildable 9242
Build 15230: Trigger Windows Autobuild
Build 15229: Post-Commit Build	Jenkins

Event Timeline

elexis committed rP22856: Support creating lobby GUI messages with arbitrary arguments instead of forcing….Sep 6 2019, 7:14 AM

elexis added an edge: D2264: Create lobby GUI messages with arbitrary arguments.

Harbormaster completed building B9242: rP22856: Support creating lobby GUI messages with arbitrary arguments instead of forcing….Sep 6 2019, 7:21 AM

elexis mentioned this in D2266: Fix wrong "player is not a moderator anymore" message.Sep 6 2019, 7:57 AM

elexis mentioned this in rP22859: Fix wrong "player is not a moderator anymore" lobby chat message following….Sep 6 2019, 6:56 PM

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

/ps/trunk/source/lobby/XmppClient.cpp
647	Concern: 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 mentioned this in D2128: Use CreateObject in EngineScriptConversions.Sep 9 2019, 11:03 AM

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

/ps/trunk/source/lobby/XmppClient.cpp
647	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 mentioned this in D2271: Fix lobby utf8 conversion following rP22856 / D2264.Sep 9 2019, 3:45 PM

elexis mentioned this in D2274: Pass lobby certificate status as a separate value and allow Xmpp error strings to become static.Sep 9 2019, 6:14 PM

elexis mentioned this in rP22880: Pass XmppClient CertificateErrorToString independent of ConnectionErrorToString….Sep 9 2019, 11:27 PM

elexis mentioned this in D2282: Fix lobby GUI Kick messages historic property following arbitrary CreateGUIMessage arguments in rP22856/D2264.Sep 11 2019, 11:56 AM

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

/ps/trunk/source/lobby/XmppClient.cpp
641	Concern: 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.

elexis mentioned this in rP22887: Fix historic lobby kick messages following arbitrary CreateGUIMessage arguments….Sep 11 2019, 12:15 PM

elexis mentioned this in rP22891: Fix missing wstring_from_utf8 for multi-user-chat messages and translated….Sep 12 2019, 7:24 PM

elexis mentioned this in D2298: Fix GlooxScriptConversions include in rP22856 / D2264 triggering VS2015 build warning (but no clang, c++ warning, and should become reported on Phabricator).Sep 16 2019, 6:53 PM

elexis mentioned this in rP22919: Fix wrong GlooxScriptConversions include in rP22856 / D2264 triggering VS2015….Sep 17 2019, 10:39 PM

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 => {
                        addChatMessage({
-                               "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() {
	        [].keys
	        return (function() {
	            f();
	        });
	    }
	});
	f();

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.

elexis mentioned this in D2421: Update sprintf.js library.Nov 13 2019, 2:38 PM

elexis mentioned this in rP23157: Update sprintf.js library from 1.0.2 to 1.1.2..Nov 14 2019, 2:01 PM

Itms mentioned this in D2922: Fix a compartment mismatch in XmppClient / Possibly fix semi-random lobby / in game JS crash.Oct 28 2020, 6:55 PM

wraitii mentioned this in rP24116: Fix a compartment mismatch in XmppClient, causing crashes in MP games..Oct 31 2020, 11:14 AM