Skip to content

Commit

Permalink
Address code review comments
Browse files Browse the repository at this point in the history 
In particular, rename GetterSlot to AccessorSlot, and re-name the new
ScriptableObject.defineProperty method from defineOwnProperty to match
the existing reflection-based way of defining a property.
  • Loading branch information
@gbrail
gbrail committed Jun 15, 2021
1 parent a368a9e commit d2b2c9e
Show file tree
Hide file tree
Showing 3 changed files with 63 additions and 55 deletions.
48 changes: 28 additions & 20 deletions src/org/mozilla/javascript/GetterSlot.java → src/org/mozilla/javascript/AccessorSlot.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@
* using Java and JavaScript functions. Unlike LambdaSlot, the fact that these values are accessed
* and mutated by functions is visible via the slot's property descriptor.
*/
public class GetterSlot extends Slot {
public class AccessorSlot extends Slot {
private static final long serialVersionUID = 1677840254177335827L;

GetterSlot(Slot oldSlot) {
AccessorSlot(Slot oldSlot) {
super(oldSlot);
}

Expand Down Expand Up @@ -83,42 +83,45 @@ Function getGetterFunction(String name, Scriptable scope) {
return getter.asGetterFunction(name, scope);
}

abstract static class Getter {
abstract Object getValue(Scriptable start);
interface Getter {
Object getValue(Scriptable start);

abstract Function asGetterFunction(final String name, final Scriptable scope);
Function asGetterFunction(final String name, final Scriptable scope);
}

/** This is a Getter that delegates to a Java function via a MemberBox. */
static final class MemberBoxGetter extends Getter {
static final class MemberBoxGetter implements Getter {
final MemberBox member;

MemberBoxGetter(MemberBox member) {
this.member = member;
}

Object getValue(Scriptable start) {
@Override
public Object getValue(Scriptable start) {
if (member.delegateTo == null) {
return member.invoke(start, ScriptRuntime.emptyArgs);
}
return member.invoke(member.delegateTo, new Object[] {start});
}

Function asGetterFunction(String name, Scriptable scope) {
@Override
public Function asGetterFunction(String name, Scriptable scope) {
return member.asGetterFunction(name, scope);
}
}

/** This is a getter that delegates to a JavaScript function. */
static final class FunctionGetter extends Getter {
static final class FunctionGetter implements Getter {
// The value of the function might actually be Undefined, so we need an Object here.
final Object target;

FunctionGetter(Object target) {
this.target = target;
}

Object getValue(Scriptable start) {
@Override
public Object getValue(Scriptable start) {
if (target instanceof Function) {
Function t = (Function) target;
Context cx = Context.getContext();
Expand All @@ -127,26 +130,28 @@ Object getValue(Scriptable start) {
return Undefined.instance;
}

Function asGetterFunction(String name, Scriptable scope) {
@Override
public Function asGetterFunction(String name, Scriptable scope) {
return target instanceof Function ? (Function) target : null;
}
}

abstract static class Setter {
abstract boolean setValue(Object value, Scriptable owner, Scriptable start);
interface Setter {
boolean setValue(Object value, Scriptable owner, Scriptable start);

abstract Function asSetterFunction(final String name, final Scriptable scope);
Function asSetterFunction(final String name, final Scriptable scope);
}

/** Invoke the setter on this slot via reflection using MemberBox. */
static final class MemberBoxSetter extends Setter {
static final class MemberBoxSetter implements Setter {
final MemberBox member;

MemberBoxSetter(MemberBox member) {
this.member = member;
}

boolean setValue(Object value, Scriptable owner, Scriptable start) {
@Override
public boolean setValue(Object value, Scriptable owner, Scriptable start) {
Context cx = Context.getContext();
Class<?>[] pTypes = member.argTypes;
// XXX: cache tag since it is already calculated in
Expand All @@ -163,22 +168,24 @@ boolean setValue(Object value, Scriptable owner, Scriptable start) {
return true;
}

Function asSetterFunction(String name, Scriptable scope) {
@Override
public Function asSetterFunction(String name, Scriptable scope) {
return member.asSetterFunction(name, scope);
}
}

/**
* Invoke the setter as a JavaScript function, taking care that it might actually be Undefined.
*/
static final class FunctionSetter extends Setter {
static final class FunctionSetter implements Setter {
final Object target;

FunctionSetter(Object target) {
this.target = target;
}

boolean setValue(Object value, Scriptable owner, Scriptable start) {
@Override
public boolean setValue(Object value, Scriptable owner, Scriptable start) {
if (target instanceof Function) {
Function t = (Function) target;
Context cx = Context.getContext();
Expand All @@ -187,7 +194,8 @@ boolean setValue(Object value, Scriptable owner, Scriptable start) {
return true;
}

Function asSetterFunction(String name, Scriptable scope) {
@Override
public Function asSetterFunction(String name, Scriptable scope) {
return target instanceof Function ? (Function) target : null;
}
}
Expand Down
60 changes: 30 additions & 30 deletions src/org/mozilla/javascript/ScriptableObject.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -543,45 +543,45 @@ public void setGetterOrSetter(
if (name != null && index != 0) throw new IllegalArgumentException(name);
checkNotSealed(name, index);

GetterSlot fslot;
AccessorSlot aSlot;
if (isExtensible()) {
Slot slot = slotMap.modify(name, index, 0);
if (slot instanceof GetterSlot) {
fslot = (GetterSlot) slot;
if (slot instanceof AccessorSlot) {
aSlot = (AccessorSlot) slot;
} else {
fslot = new GetterSlot(slot);
slotMap.replace(slot, fslot);
aSlot = new AccessorSlot(slot);
slotMap.replace(slot, aSlot);
}
} else {
Slot slot = slotMap.query(name, index);
if (slot instanceof GetterSlot) {
fslot = (GetterSlot) slot;
if (slot instanceof AccessorSlot) {
aSlot = (AccessorSlot) slot;
} else {
return;
}
}

int attributes = fslot.getAttributes();
int attributes = aSlot.getAttributes();
if ((attributes & READONLY) != 0) {
throw Context.reportRuntimeErrorById("msg.modify.readonly", name);
}

// Emulate old behavior where nothing happened if it wasn't actually a Function
if (isSetter) {
if (getterOrSetter instanceof Function) {
fslot.setter = new GetterSlot.FunctionSetter((Function) getterOrSetter);
aSlot.setter = new AccessorSlot.FunctionSetter((Function) getterOrSetter);
} else {
fslot.setter = null;
aSlot.setter = null;
}
} else {
if (getterOrSetter instanceof Function) {
fslot.getter = new GetterSlot.FunctionGetter((Function) getterOrSetter);
aSlot.getter = new AccessorSlot.FunctionGetter((Function) getterOrSetter);
} else {
fslot.getter = null;
aSlot.getter = null;
}
}

fslot.value = Undefined.instance;
aSlot.value = Undefined.instance;
}

/**
Expand Down Expand Up @@ -1483,20 +1483,20 @@ public void defineProperty(
}

Slot slot = slotMap.modify(propertyName, 0, 0);
GetterSlot getterSlot;
if (slot instanceof GetterSlot) {
getterSlot = (GetterSlot) slot;
AccessorSlot aSlot;
if (slot instanceof AccessorSlot) {
aSlot = (AccessorSlot) slot;
} else {
getterSlot = new GetterSlot(slot);
slotMap.replace(slot, getterSlot);
aSlot = new AccessorSlot(slot);
slotMap.replace(slot, aSlot);
}

getterSlot.setAttributes(attributes);
aSlot.setAttributes(attributes);
if (getterBox != null) {
getterSlot.getter = new GetterSlot.MemberBoxGetter(getterBox);
aSlot.getter = new AccessorSlot.MemberBoxGetter(getterBox);
}
if (setterBox != null) {
getterSlot.setter = new GetterSlot.MemberBoxSetter(setterBox);
aSlot.setter = new AccessorSlot.MemberBoxSetter(setterBox);
}
}

Expand Down Expand Up @@ -1577,22 +1577,22 @@ protected void defineOwnProperty(
}

if (isAccessor) {
GetterSlot fslot;
AccessorSlot fslot;

if (slot instanceof GetterSlot) {
fslot = (GetterSlot) slot;
if (slot instanceof AccessorSlot) {
fslot = (AccessorSlot) slot;
} else {
fslot = new GetterSlot(slot);
fslot = new AccessorSlot(slot);
slotMap.replace(slot, fslot);
}

Object getter = getProperty(desc, "get");
if (getter != NOT_FOUND) {
fslot.getter = new GetterSlot.FunctionGetter(getter);
fslot.getter = new AccessorSlot.FunctionGetter(getter);
}
Object setter = getProperty(desc, "set");
if (setter != NOT_FOUND) {
fslot.setter = new GetterSlot.FunctionSetter(setter);
fslot.setter = new AccessorSlot.FunctionSetter(setter);
}

fslot.value = Undefined.instance;
Expand Down Expand Up @@ -1627,7 +1627,7 @@ protected void defineOwnProperty(
* set directly and may not be retrieved by the getter.
* @param attributes the attributes to set on the property
*/
public void defineOwnProperty(
public void defineProperty(
String name, Supplier<Object> getter, Consumer<Object> setter, int attributes) {
Slot slot = slotMap.modify(name, 0, attributes);

Expand Down Expand Up @@ -2439,7 +2439,7 @@ private boolean putImpl(Object key, int index, Scriptable start, Object value) {
slot = slotMap.query(key, index);
if (!isExtensible
&& (slot == null
|| (!(slot instanceof GetterSlot)
|| (!(slot instanceof AccessorSlot)
&& (slot.getAttributes() & READONLY) != 0))
&& Context.isCurrentContextStrict()) {
throw ScriptRuntime.typeErrorById("msg.not.extensible");
Expand All @@ -2450,7 +2450,7 @@ private boolean putImpl(Object key, int index, Scriptable start, Object value) {
} else if (!isExtensible) {
slot = slotMap.query(key, index);
if ((slot == null
|| (!(slot instanceof GetterSlot)
|| (!(slot instanceof AccessorSlot)
&& (slot.getAttributes() & READONLY) != 0))
&& Context.isCurrentContextStrict()) {
throw ScriptRuntime.typeErrorById("msg.not.extensible");
Expand Down
10 changes: 5 additions & 5 deletions testsrc/org/mozilla/javascript/tests/LambdaPropertyTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ public void testBasicProperty() {
ScriptableObject.defineProperty(global, "o", testObj, 0);

SomeState state = new SomeState();
testObj.defineOwnProperty(
testObj.defineProperty(
"val",
() -> state.val,
(Object newVal) -> state.val = ScriptRuntime.toInt32(newVal),
Expand Down Expand Up @@ -76,7 +76,7 @@ public void testRedefineProperty() {
null);

SomeState state = new SomeState();
testObj.defineOwnProperty(
testObj.defineProperty(
"val",
() -> state.val,
(Object newVal) -> state.val = ScriptRuntime.toInt32(newVal),
Expand Down Expand Up @@ -111,7 +111,7 @@ public void testRedefinePropertyNonConfigurable() {
null);

SomeState state = new SomeState();
testObj.defineOwnProperty(
testObj.defineProperty(
"val",
() -> state.val,
(Object newVal) -> state.val = ScriptRuntime.toInt32(newVal),
Expand All @@ -136,7 +136,7 @@ public void testPropertyNoSetter() {
ScriptableObject.defineProperty(global, "o", testObj, 0);

SomeState state = new SomeState();
testObj.defineOwnProperty("val", () -> state.val, null, 0);
testObj.defineProperty("val", () -> state.val, null, 0);

// Set should do nothing
cx.evaluateString(global, "assertEquals(0, o.val)", "test.js", 1, null);
Expand All @@ -150,7 +150,7 @@ public void testPropertyNoGetter() {
ScriptableObject.defineProperty(global, "o", testObj, 0);

SomeState state = new SomeState();
testObj.defineOwnProperty(
testObj.defineProperty(
"val", null, (Object newVal) -> state.val = ScriptRuntime.toInt32(newVal), 0);

// Get should return the old value, which is unfortunately null
Expand Down

0 comments on commit d2b2c9e

Please sign in to comment.