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.

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

384 - member box call error #353

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
13 changes: 11 additions & 2 deletions src/org/mozilla/javascript/ScriptableObject.java
Original file line number Diff line number Diff line change
Expand Up @@ -253,8 +253,17 @@ ScriptableObject getPropertyDescriptor(Context cx, Scriptable scope) {
if (getter == null && setter == null) {
desc.defineProperty("writable", (attr & READONLY) == 0, EMPTY);
}
if (getter != null) desc.defineProperty("get", getter, EMPTY);
if (setter != null) desc.defineProperty("set", setter, EMPTY);
if (getter != null)
{
if( getter instanceof MemberBox ) desc.defineProperty("get", new FunctionObject("f", ((MemberBox)getter).member(),scope), EMPTY);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sadly there is not a single code style for this huge, old code base, but generally we follow the style of the code "around us" when making a change. You can see that this change is pretty different from other Rhino code. Specifically:

  1. Please use spaces and not tabs
  2. Put the spaces on the other side of the parentheses (like you see elsewhere in the file)
  3. It's not a hard and fast rule yet, but using brackets for every "if" and "else" statement, even if there is only a single statement, avoids lots of errors later.

(I also think this is why the Travis CI build is failing for this PR.)

else if( getter instanceof Member ) desc.defineProperty("get", new FunctionObject("f",(Member)getter,scope), EMPTY);
else desc.defineProperty("get", getter, EMPTY);
}
if (setter != null) {
if( setter instanceof MemberBox ) desc.defineProperty("set", new FunctionObject("f", ((MemberBox)setter).member(),scope), EMPTY);
else if( setter instanceof Member ) desc.defineProperty("set", new FunctionObject("f",(Member)setter,scope), EMPTY);
else desc.defineProperty("set", setter, EMPTY);
}
return desc;
}

Expand Down
108 changes: 108 additions & 0 deletions testsrc/org/mozilla/javascript/tests/MemberBoxCallTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

package org.mozilla.javascript.tests;

import org.junit.Test;
import org.junit.Before;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNull;

import org.mozilla.javascript.*;
import org.mozilla.javascript.annotations.*;

public class MemberBoxCallTest {

Scriptable scope;

@Test
public void testPrototypeProperty() {
Context cx = Context.enter();
try {
assertEquals(evaluate(cx,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see a few of properties and functions on "AnnotatedHostObject," but it looks like this test is only checking for "foo." Is there a reason why we can't test the other properties? Similarly, are you sure that all the code paths in ScriptableObject are being exercised here for both Member and MemberBox?

"var hostObj = new AnnotatedHostObject(); " +
"var valueProperty = Object.getOwnPropertyDescriptor(Object.getPrototypeOf(hostObj), \"foo\");" +
"var result = 'failed';" +
"if( valueProperty.get && valueProperty.set ) {" +
"valueProperty.set.call(hostObj, 'superVal');" +
"result = valueProperty.get.call(hostObj);" +
"}"+
"result;"), "SUPERVAL");
} finally {
Context.exit();
}
}

private Object evaluate(Context cx, String str) {
return cx.evaluateString(scope, str, "<testsrc>", 0, null);
}


@Before
public void init() throws Exception {
Context cx = Context.enter();
try {
scope = cx.initStandardObjects();
ScriptableObject.defineClass(scope, AnnotatedHostObject.class);
} finally {
Context.exit();
}
}

public static class AnnotatedHostObject extends ScriptableObject {

String foo, bar = "bar";

public AnnotatedHostObject() {}

@Override
public String getClassName() {
return "AnnotatedHostObject";
}

@JSConstructor
public void jsConstructorMethod() {
put("initialized", this, Boolean.TRUE);
}

@JSFunction
public Object instanceFunction() {
return "instanceFunction";
}

@JSFunction("namedFunction")
public Object someFunctionName() {
return "namedFunction";
}

@JSStaticFunction
public static Object staticFunction() {
return "staticFunction";
}

@JSStaticFunction("namedStaticFunction")
public static Object someStaticFunctionName() {
return "namedStaticFunction";
}

@JSGetter
public String getFoo() {
return foo;
}

@JSSetter
public void setFoo(String foo) {
this.foo = foo.toUpperCase();
}

@JSGetter("bar")
public String getMyBar() {
return bar;
}

public void setBar(String bar) {
this.bar = bar.toUpperCase();
}
}
}