Page MenuHomeWildfire Games

Remove obsolete JSI_GUIMouse, JSI_IGUIObject::construct, JS_THIS_OBJECT and cleanup JS GUI interface
Needs ReviewPublic

Authored by elexis on Dec 19 2018, 3:45 PM.

Details

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

JSI_GUIMouse:
rP666 introduced the GUI <-> JS Interface.
rP8657 removed the only use of JSI_GUIMouse, and because nowadays SDL events are converted to JS and since it doesn't really make sense to create a Mouse object in JS and send it to C++, the class should
be considered obsolete and removed.

JSI_IGUIObject::construct rP666 also introduced the constructor of JSI_IGUIObjects. But that constructor is (at least nowadays) only called form JS and adding new GUI objects from JS is something we
don't have a use case for yet, it would likely add to messy code. It is good to have every JS object defined in XML prior before they are accessed, leaving the new reader with a clear summary of which
objects exist before they start to dig into the code. If we ever run into a use case where we want to add a GUIObject by JS, there will have to be much more code, updating the GUI page with the new
object and such. Conclusively this constructor is unused and merely confusing the reader by pretending to be relevant.
Notice that JSI_GUISize and JSI_GUIColor actually are created from JS and passed to JS, so it makes sense to have these JSClasses.

Cleanup:

  • The headers are rectified a bit.
  • The JS functions receive the JSPROP_ENUMERATE, JSPROP_READONLY, JSPROP_PERMANENT flags, so that JS can't mess with them.
  • { 0 } -> JS_PS_END
  • Rename init to RegisterScriptClass for more transparency and consistency in

ScriptFunctions.cpp

JS_THIS_OBJECT: This macro was removed from Firefox 61, so it is likely to also be removed in SM then. The replacement looks cleaner anyhow. https://bugzilla.mozilla.org/show_bug.cgi?id=1255800

Renames that will not be done in this patch:

  • g_GUI should be g_GUIManager whereas CGUI should be CGUIPage
  • JSInterface_IGUIObject ought to be JSInterface_GUIObject for consistency with the other JSInterface_GUIfoo.
  • JSInterface_GUITypes.cpp should be split into JSInterface_GUISize.cpp and JSInterface_GUIColor.cpp, so it is easier to add new types and one doesn't have to read all types to understand one

file, but only one type.

Test Plan

Read the spidermonkey doc. Compile with this patch and notice that "everything still works". Possibly add debug_printfs to see that the removed code is never called and donate at
https://play0ad.com/community/donate/.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6513
Build 10772: Vulcan BuildJenkins
Build 10771: arc lint + arc unit

Event Timeline

elexis created this revision.Dec 19 2018, 3:45 PM
Vulcan added a subscriber: Vulcan.Dec 19 2018, 4:04 PM

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

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

Stan added a subscriber: Stan.Dec 19 2018, 4:33 PM
Stan added inline comments.
source/gui/scripting/JSInterface_IGUIObject.cpp
123

Range loop ?

elexis marked an inline comment as done.Dec 19 2018, 4:39 PM
elexis added inline comments.
source/gui/scripting/JSInterface_IGUIObject.cpp
123

Check again

elexis marked 2 inline comments as done.Dec 21 2018, 2:39 PM
elexis added inline comments.
source/gui/scripting/JSInterface_IGUIObject.cpp
630

Also this is the only place that uses toPrivate. The code looks kind of wrong: where would that IGUIObject instance created (if not ConstructObject in Xeromyces_ReadObject)? The two statements look like it would would parse the existing private value to an IGUIObject pointer and set that pointer back to the private object, i.e. noop with at most errors occuring.

elexis updated the Trac tickets for this revision.Dec 21 2018, 5:50 PM
wraitii requested changes to this revision.Dec 31 2018, 9:51 AM
wraitii added a subscriber: wraitii.

This will conflict with the SM45 upgrade as you are using JS_FS

This revision now requires changes to proceed.Dec 31 2018, 9:51 AM
elexis requested review of this revision.Dec 31 2018, 11:36 AM

This is not how reviews are supposed to work I think.

wraitii edited reviewers, added: Restricted Owners Package; removed: wraitii.Apr 20 2019, 4:49 PM

All I remember is that these should be JS_FN instead of JS_FS. Haven't looked at the rest.