Implement [LegacyFactoryFunction]

[ExE-Boss]

This implements [LegacyFactoryFunction].

Unlike Chromium’s implementation, which assumes that a single interface will only ever have at most one legacy factory function, this implementation makes it possible to define more than one legacy factory function per interface, as can be seen with test/cases/LegacyFactoryFunction.webidl.

While working on this, I’ve noticed that exports.new(globalObject) will unconditionally throw a TypeError if the interface is a legacy platform object, because wrapper is defined as const wrapper, but the Proxy creation code uses:

wrapper = new Proxy(wrapper, proxyHandler);

Depends on:

• Fix new(globalObject) for legacy platform objects #214 (merged)
• Implement “internally create a new object implementing the interface” #239 (merged)

[domenic]

Unlike Chromium’s implementation, which assumes that a single interface will only ever have at most one legacy factory function, this implementation makes it possible to define more than one legacy factory function per interface, as can be seen with test/cases/LegacyFactoryFunction.webidl.

Let's remove that complexity. We can add a constraint to the Web IDL spec that prohibits more than one LegacyFactoryFunction instead.

[domenic]

The generated snapshots here seem overcomplicated, and it's not clear why you're modifying existing functionality (like new()).

I think the root of the problem is that you're trying to construct the thisArg. Instead, since it's a factory function, the impl class can do that. In other words, I'd expect the generated code

    const thisArgument = exports.new(globalObject, new.target);
    const result = Impl.legacyFactoryFunction.call(thisArgument, globalObject, ...args);
    return utils.tryWrapperForImpl(utils.isObject(result) ? result : thisArgument);

to instead be

    return utils.wrapperForImpl(Impl.legacyFactoryFunction(globalObject, ...args));

[ExE-Boss]

@domenic That won’t support the case when new.target !== activeFunctionObject.

[domenic]

Hmm. I see there are references to NewTarget in https://heycam.github.io/webidl/#legacy-factory-functions but I am unsure whether they are implemented, or if they are, whether they're necessary for web compatibility.

[ExE-Boss]

@domenic All browsers implement the NewTarget handling (web-platform-tests/wpt#27991), so it’s probably necessary for web compatibility.

[domenic]

@ExE-Boss want to rebase this and finish review and then we can do a new release?

[ExE-Boss]

@domenic

I decided to pass the thisValue as the first parameter, rather than this so that it’s possible to use arrow functions for this:

exports.legacyFactoryFunction = (thisValue, globalObject, [...args], { newTarget, wrapper }) => {
	// code
}

I also decided to include the new.target and wrapper in a privateData object to match constructor(globalObject, args, privateData).

[domenic]

Would jsdom's implementations of the relevant parts of the HTML spec actually use any of thisValue, newTarget, or wrapper? If not, let's exclude them.

[ExE-Boss]

We should at the very least include thisValue and pass the args as an array to leave open the possibility of a privateData bag in case https://html.spec.whatwg.org/multipage/media.html#dom-audio, https://html.spec.whatwg.org/multipage/embedded-content.html#dom-image and https://html.spec.whatwg.org/multipage/form-elements.html#dom-option are rewritten.

[domenic]

I'm still feeling like this is over-complex given that there are literally three cases and we know exactly how they behave. Our goal here is not to have a general framework to do any possible LegacyFactoryFunction; it's to implement Option, Audio, and Image.

In particular, I'd like to see these either support return override, or use the passed in thisValue, but not both. I suspect thisValue would be simpler, and perhaps necessary to support class X extends Audio {}. So I suspect the correct signature here will be (thisValue, globalObject, [...args]) with no support for return-override.

[domenic]

What value does having these three tests give, instead of just one of them? I can see zero-args vs. nonzero, but these three cases don't seem to differ in interesting ways.

[domenic]

Would jsdom's implementations of the relevant parts of the HTML spec actually use any of thisValue, newTarget, or wrapper? If not, let's exclude them.

[domenic]

This seems more likely to be a historical artifact of how the spec is written?

It seems like we should be able to support either return overload (like HTML uses for its LegacyFactoryFunctions), or generate a this value (like modern Web IDL-using constructor specs do). We shouldn't need both, I don't think?

In particular given that you can subclass Option per the tests you wrote, I suspect the first line of https://html.spec.whatwg.org/#dom-option is just incorrect.

[ExE-Boss]

It occurred to me that the order of Impl.legacyFactoryFunction and Impl.init calls is wrong, so I’m fixing that so that the object returned from Impl.legacyFactoryFunction is now the impl, and the result of makeWrapper is what’s returned from the LegacyFactoryFunction constructor.

---

This also makes it possible to support returning the result of new Impl.implementation.