Page MenuHomeWildfire Games

Rename JSInterface_GUITypes to JSInterface_GUISize
ClosedPublic

Authored by elexis on Sep 30 2019, 9:44 AM.

Details

Summary

rP22531 removed the JSI_GUIMouse class, rP22534 removed the JSI_GUIColor class, leaving only JSI_GUISize in this file.
New JS classes should be defined in a separate file.
Therefore this JSInterface_GUITypes.h file should be renamed and the macro dropped.
While at it rename init to RegisterScriptClass, for consistency with RegisterScriptFunctions and because there should be at most one class per file.
While at it move that function to the top of the file, since it's the first thing that happens, and at the top it reasonably stands out whereas at the bottom it seems unexpected to encounter such a function (after having read 120 lines of JSNatives).
While at that remove the apparently unused minArgs argument.
While at it move ToPercentString to the namespace, so that it cannot shadow a global declared in a header elsewhere.

Test Plan

Suspect that the minArgs argument of that function has some purpose.
Look up https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_InitClass but don't find it.
Start reading the spidermonkey code and find this:

JS_PUBLIC_API(JSObject*)
JS_InitClass(JSContext* cx, HandleObject obj, HandleObject parent_proto,
             const JSClass* clasp, JSNative constructor, unsigned nargs,
             const JSPropertySpec* ps, const JSFunctionSpec* fs,
             const JSPropertySpec* static_ps, const JSFunctionSpec* static_fs)
{
    AssertHeapIsIdle(cx);
    CHECK_REQUEST(cx);
    assertSameCompartment(cx, obj, parent_proto);
    return InitClass(cx, obj, parent_proto, Valueify(clasp), constructor,
                     nargs, ps, fs, static_ps, static_fs);
}


NativeObject*
js::InitClass(JSContext* cx, HandleObject obj, HandleObject protoProto_,
              const Class* clasp, Native constructor, unsigned nargs,
              const JSPropertySpec* ps, const JSFunctionSpec* fs,
              const JSPropertySpec* static_ps, const JSFunctionSpec* static_fs,
              NativeObject** ctorp, AllocKind ctorKind)
{
    RootedObject protoProto(cx, protoProto_);

    /* Check function pointer members. */
    MOZ_ASSERT(clasp->getProperty != JS_PropertyStub);
    MOZ_ASSERT(clasp->setProperty != JS_StrictPropertyStub);

    RootedAtom atom(cx, Atomize(cx, clasp->name, strlen(clasp->name)));
    if (!atom)
        return nullptr;

    /*
     * All instances of the class will inherit properties from the prototype
     * object we are about to create (in DefineConstructorAndPrototype), which
     * in turn will inherit from protoProto.
     *
     * When initializing a standard class (other than Object), if protoProto is
     * null, default to Object.prototype. The engine's internal uses of
     * js::InitClass depend on this nicety.
     */
    JSProtoKey key = JSCLASS_CACHED_PROTO_KEY(clasp);
    if (key != JSProto_Null &&
        !protoProto &&
        !GetBuiltinPrototype(cx, JSProto_Object, &protoProto))
    {
        return nullptr;
    }

    return DefineConstructorAndPrototype(cx, obj, key, atom, protoProto, clasp, constructor, nargs,
                                         ps, fs, static_ps, static_fs, ctorp, ctorKind);
}




static NativeObject*
DefineConstructorAndPrototype(JSContext* cx, HandleObject obj, JSProtoKey key, HandleAtom atom,
                              HandleObject protoProto, const Class* clasp,
                              Native constructor, unsigned nargs,
                              const JSPropertySpec* ps, const JSFunctionSpec* fs,
                              const JSPropertySpec* static_ps, const JSFunctionSpec* static_fs,
                              NativeObject** ctorp, AllocKind ctorKind)
{
    /*
     * Create a prototype object for this class.
     *
     * FIXME: lazy standard (built-in) class initialization and even older
     * eager boostrapping code rely on all of these properties:
     *
     * 1. NewObject attempting to compute a default prototype object when
     *    passed null for proto; and
     *
     * 2. NewObject tolerating no default prototype (null proto slot value)
     *    due to this js::InitClass call coming from js::InitFunctionClass on an
     *    otherwise-uninitialized global.
     *
     * 3. NewObject allocating a JSFunction-sized GC-thing when clasp is
     *    &JSFunction::class_, not a JSObject-sized (smaller) GC-thing.
     *
     * The JS_NewObjectForGivenProto and JS_NewObject APIs also allow clasp to
     * be &JSFunction::class_ (we could break compatibility easily). But
     * fixing (3) is not enough without addressing the bootstrapping dependency
     * on (1) and (2).
     */

    /*
     * Create the prototype object.  (GlobalObject::createBlankPrototype isn't
     * used because it won't let us use protoProto as the proto.
     */
    RootedNativeObject proto(cx, NewNativeObjectWithClassProto(cx, clasp, protoProto, SingletonObject));
    if (!proto)
        return nullptr;

    /* After this point, control must exit via label bad or out. */
    RootedNativeObject ctor(cx);
    bool named = false;
    bool cached = false;
    if (!constructor) {
        /*
         * Lacking a constructor, name the prototype (e.g., Math) unless this
         * class (a) is anonymous, i.e. for internal use only; (b) the class
         * of obj (the global object) is has a reserved slot indexed by key;
         * and (c) key is not the null key.
         */
        if (!(clasp->flags & JSCLASS_IS_ANONYMOUS) || !obj->is<GlobalObject>() ||
            key == JSProto_Null)
        {
            uint32_t attrs = (clasp->flags & JSCLASS_IS_ANONYMOUS)
                           ? JSPROP_READONLY | JSPROP_PERMANENT
                           : 0;
            RootedValue value(cx, ObjectValue(*proto));
            if (!DefineStandardSlot(cx, obj, key, atom, value, attrs, named))
                goto bad;
        }

        ctor = proto;
    } else {
        RootedFunction fun(cx, NewNativeConstructor(cx, constructor, nargs, atom, ctorKind));
        if (!fun)
            goto bad;

        /*
         * Set the class object early for standard class constructors. Type
         * inference may need to access these, and js::GetBuiltinPrototype will
         * fail if it tries to do a reentrant reconstruction of the class.
         */
        if (key != JSProto_Null) {
            SetClassObject(obj, key, fun, proto);
            cached = true;
        }

        RootedValue value(cx, ObjectValue(*fun));
        if (!DefineStandardSlot(cx, obj, key, atom, value, 0, named))
            goto bad;

        /*
         * Optionally construct the prototype object, before the class has
         * been fully initialized.  Allow the ctor to replace proto with a
         * different object, as is done for operator new.
         */
        ctor = fun;
        if (!LinkConstructorAndPrototype(cx, ctor, proto))
            goto bad;

        /* Bootstrap Function.prototype (see also JS_InitStandardClasses). */
        Rooted<TaggedProto> tagged(cx, TaggedProto(proto));
        if (ctor->getClass() == clasp && !ctor->splicePrototype(cx, clasp, tagged))
            goto bad;
    }

    if (!DefinePropertiesAndFunctions(cx, proto, ps, fs) ||
        (ctor != proto && !DefinePropertiesAndFunctions(cx, ctor, static_ps, static_fs)))
    {
        goto bad;
    }

    /* If this is a standard class, cache its prototype. */
    if (!cached && key != JSProto_Null)
        SetClassObject(obj, key, ctor, proto);

    if (ctorp)
        *ctorp = ctor;
    return proto;

bad:
    if (named) {
        ObjectOpResult ignored;
        RootedId id(cx, AtomToId(atom));

        // XXX FIXME - absurd to call this here; instead define the property last.
        DeleteProperty(cx, obj, id, ignored);
    }
    if (cached)
        ClearClassObject(obj, key);
    return nullptr;
}




js::NewNativeConstructor(ExclusiveContext* cx, Native native, unsigned nargs, HandleAtom atom,
                         gc::AllocKind allocKind /* = AllocKind::FUNCTION */,
                         NewObjectKind newKind /* = GenericObject */,
                         JSFunction::Flags flags /* = JSFunction::NATIVE_CTOR */)
{
    MOZ_ASSERT(flags & JSFunction::NATIVE_CTOR);
    return NewFunctionWithProto(cx, native, nargs, flags, nullptr, atom,
                                nullptr, allocKind, newKind);
}


JSFunction*
js::NewFunctionWithProto(ExclusiveContext* cx, Native native,
                         unsigned nargs, JSFunction::Flags flags, HandleObject enclosingDynamicScope,
                         HandleAtom atom, HandleObject proto,
                         gc::AllocKind allocKind /* = AllocKind::FUNCTION */,
                         NewObjectKind newKind /* = GenericObject */,
                         NewFunctionProtoHandling protoHandling /* = NewFunctionClassProto */)
{
    MOZ_ASSERT(allocKind == AllocKind::FUNCTION || allocKind == AllocKind::FUNCTION_EXTENDED);
    MOZ_ASSERT_IF(native, !enclosingDynamicScope);
    MOZ_ASSERT(NewFunctionScopeIsWellFormed(cx, enclosingDynamicScope));

    RootedObject funobj(cx);
    // Don't mark asm.js module functions as singleton since they are
    // cloned (via CloneFunctionObjectIfNotSingleton) which assumes that
    // isSingleton implies isInterpreted.
    if (native && !IsAsmJSModuleNative(native))
        newKind = SingletonObject;

    if (protoHandling == NewFunctionClassProto) {
        funobj = NewObjectWithClassProto(cx, &JSFunction::class_, proto, allocKind,
                                         newKind);
    } else {
        funobj = NewObjectWithGivenTaggedProto(cx, &JSFunction::class_, AsTaggedProto(proto),
                                               allocKind, newKind);
    }
    if (!funobj)
        return nullptr;

    RootedFunction fun(cx, &funobj->as<JSFunction>());

    if (allocKind == AllocKind::FUNCTION_EXTENDED)
        flags = JSFunction::Flags(flags | JSFunction::EXTENDED);

    /* Initialize all function members. */
    fun->setArgCount(uint16_t(nargs));
    fun->setFlags(flags);
    if (fun->isInterpreted()) {
        MOZ_ASSERT(!native);
        if (fun->isInterpretedLazy())
            fun->initLazyScript(nullptr);
        else
            fun->initScript(nullptr);
        fun->initEnvironment(enclosingDynamicScope);
    } else {
        MOZ_ASSERT(fun->isNative());
        MOZ_ASSERT(native);
        fun->initNative(native, nullptr);
    }
    if (allocKind == AllocKind::FUNCTION_EXTENDED)
        fun->initializeExtended();
    fun->initAtom(atom);

    return fun;
}




class JSFunction : public js::NativeObject
...
    // Can be called multiple times by the parser.
    void setArgCount(uint16_t nargs) {
        this->nargs_ = nargs;
    }


  private:
    uint16_t        nargs_;       /* number of formal arguments
                                     (including defaults and the rest parameter unlike f.length) */

    size_t nargs() const {
        return nargs_;
    }




    void initNative(js::Native native, const JSJitInfo* jitinfo) {
        MOZ_ASSERT(native);
        u.n.native = native;
        u.n.jitinfo = jitinfo;
    }

then lose track of where the argumentcount is used, since the constructor is called from the JS side.
Hence lose motivation and go to bed instead.
Notice that this problem doesn't invalidate the diff and that you can't keep stacking valid patches without committing them.
Add a debug_printf for the argument count in the constructor function and see that the JS side determines this number regardless of whether the number specified on C++ side is too large or too small.
Notice that the constructor can consume 0, 4 or 8 arguments, so every number is wrong if it is meant to be a precise count.
Notice that the ScriptInterface function calls it minArgs, but that we don't see such an indication in the cited spidermonkey code.
Notice that this way one can only decide for a wrong value arbitrarily, hence chose 0 because that would be the minimum amount of arguments for the constructor, and since specifying a lower limit of 0 is possible but an upper limit of 8 sounds more arbitrary (as it may change in the future too, more likely than 0).
Conclude to whatever.
Make sure there is no defect or behavior change introduced otherwise and that the includes are improved.

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

elexis created this revision.Sep 30 2019, 9:44 AM

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

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

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

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

This revision was not accepted when it landed; it landed in state Needs Review.Sep 30 2019, 10:20 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.