Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Add [ImplicitThis] behaviour #166

Merged
merged 4 commits into from
Mar 16, 2020
+1,150 −676
Merged

fix: Add [ImplicitThis] behaviour #166

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f365d33
feat(Global): Add `[ImplicitThis]` behaviour
ExE-Boss Feb 13, 2020
fdb0c75
fix: Use correct `[ImplicitThis]` behaviour
ExE-Boss Feb 14, 2020
6d07394
fix: Add `globalObject` to scope of `_internalSetup`
ExE-Boss Feb 14, 2020
1aed7f7
fix: Undefined `esValue` in attribute stringifiers
ExE-Boss Feb 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 15 additions & 13 deletions lib/constructs/attribute.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,19 +23,15 @@ class Attribute {

const onInstance = utils.isOnInstance(this.idl, this.interface.idl);

let objName = `this`;
if (onInstance) { // we're in a setup method
objName = `obj`;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you tell me if my understanding is correct: this isn't needed because for unforgeable methods/properties this always points to obj anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, yes, unless you were to use unforgeableMethod.call, in which case this will be whatever was passed to call as the thisArg parameter.

let brandCheck = `
if (!this || !exports.is(this)) {
if (!exports.is(esValue)) {
throw new TypeError("Illegal invocation");
}
`;
let getterBody = `return utils.tryWrapperForImpl(${objName}[impl]["${this.idl.name}"]);`;
let setterBody = `${objName}[impl]["${this.idl.name}"] = V;`;
let getterBody = `return utils.tryWrapperForImpl(esValue[impl]["${this.idl.name}"]);`;
let setterBody = `esValue[impl]["${this.idl.name}"] = V;`;
if (conversions[this.idl.idlType.idlType]) {
getterBody = `return ${objName}[impl]["${this.idl.name}"];`;
getterBody = `return esValue[impl]["${this.idl.name}"];`;
}

const addMethod = this.static ?
Expand All @@ -51,8 +47,8 @@ class Attribute {
throw new Error("Unknown reflector type: " + this.idl.idlType.idlType);
}
const attrName = shouldReflect.rhs && shouldReflect.rhs.value.replace(/_/g, "-") || this.idl.name;
getterBody = reflector[this.idl.idlType.idlType].get(objName, attrName.toLowerCase());
setterBody = reflector[this.idl.idlType.idlType].set(objName, attrName.toLowerCase());
getterBody = reflector[this.idl.idlType.idlType].get("esValue", attrName.toLowerCase());
setterBody = reflector[this.idl.idlType.idlType].set("esValue", attrName.toLowerCase());
}

if (utils.getExtAttr(this.idl.extAttrs, "LenientThis")) {
Expand All @@ -71,6 +67,7 @@ class Attribute {
}

addMethod(this.idl.name, [], `
const esValue = this !== null && this !== undefined ? this : globalObject;
${brandCheck}
${getterBody}
`, "get", { configurable });
Expand All @@ -95,19 +92,22 @@ class Attribute {
}

addMethod(this.idl.name, ["V"], `
const esValue = this !== null && this !== undefined ? this : globalObject;
${brandCheck}
${idlConversion}
${setterBody}
`, "set", { configurable });
} else if (utils.getExtAttr(this.idl.extAttrs, "PutForwards")) {
addMethod(this.idl.name, ["V"], `
const esValue = this !== null && this !== undefined ? this : globalObject;
${brandCheck}
this.${this.idl.name}.${utils.getExtAttr(this.idl.extAttrs, "PutForwards").rhs.value} = V;
`, "set", { configurable });
} else if (utils.getExtAttr(this.idl.extAttrs, "Replaceable")) {
addMethod(this.idl.name, ["V"], `
const esValue = this !== null && this !== undefined ? this : globalObject;
${brandCheck}
Object.defineProperty(this, "${this.idl.name}", {
Object.defineProperty(esValue, "${this.idl.name}", {
configurable: true,
enumerable: true,
value: V,
Expand All @@ -118,10 +118,12 @@ class Attribute {

if (!this.static && this.idl.special === "stringifier") {
addMethod("toString", [], `
if (!this || !exports.is(this)) {
const esValue = this;
Comment on lines 120 to +121
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correct per spec: https://heycam.github.io/webidl/#es-stringifier, whatwg/webidl#855.

if (!exports.is(esValue)) {
throw new TypeError("Illegal invocation");
}
${getterBody};

${getterBody}
`, "regular", { configurable, writable: configurable });
}

Expand Down
6 changes: 3 additions & 3 deletions lib/constructs/interface.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1144,12 +1144,12 @@ class Interface {
const obj = exports.create(globalObject, constructorArgs, privateData);
return utils.implForWrapper(obj);
};
exports._internalSetup = function _internalSetup(obj) {
exports._internalSetup = function _internalSetup(obj, globalObject) {
`;

if (this.idl.inheritance) {
this.str += `
${this.idl.inheritance}._internalSetup(obj);
${this.idl.inheritance}._internalSetup(obj, globalObject);
`;
}

Expand All @@ -1160,7 +1160,7 @@ class Interface {
exports.setup = function setup(obj, globalObject, constructorArgs = [], privateData = {}) {
privateData.wrapper = obj;

exports._internalSetup(obj);
exports._internalSetup(obj, globalObject);
Object.defineProperty(obj, impl, {
value: new Impl.implementation(globalObject, constructorArgs, privateData),
configurable: true
Expand Down
5 changes: 3 additions & 2 deletions lib/constructs/operation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,13 +62,14 @@ class Operation {

if (!this.static) {
str += `
if (!this || !exports.is(this)) {
const esValue = this !== null && this !== undefined ? this : globalObject;
if (!exports.is(esValue)) {
throw new TypeError("Illegal invocation");
}
`;
}

const callOn = this.static ? "Impl.implementation" : "this[impl]";
const callOn = this.static ? "Impl.implementation" : `esValue[impl]`;
// In case of stringifiers, use the named implementation function rather than hardcoded "toString".
// All overloads will have the same name, so pick the first one.
const implFunc = this.idls[0].name || this.name;
Expand Down
Loading