Last Comment Bug 775317 - Add a JSAPI test for the JSOPTION_VAROBJFIX behavior of JS_EvaluateScript
: Add a JSAPI test for the JSOPTION_VAROBJFIX behavior of JS_EvaluateScript
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla17
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-18 14:51 PDT by Boris Zbarsky [:bz]
Modified: 2012-07-22 12:44 PDT (History)
2 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add a test for the JSOPTION_VAROBJFIX behavior of JS_EvaluateScript. (2.45 KB, patch)
2012-07-18 14:52 PDT, Boris Zbarsky [:bz]
jorendorff: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2012-07-18 14:51:01 PDT
Because I'm about to write some code that may depend on it.
Comment 1 Boris Zbarsky [:bz] 2012-07-18 14:52:09 PDT
Created attachment 643603 [details] [diff] [review]
Add a test for the JSOPTION_VAROBJFIX behavior of JS_EvaluateScript.
Comment 2 Jason Orendorff [:jorendorff] 2012-07-20 13:13:38 PDT
Comment on attachment 643603 [details] [diff] [review]
Add a test for the JSOPTION_VAROBJFIX behavior of JS_EvaluateScript.

Review of attachment 643603 [details] [diff] [review]:
-----------------------------------------------------------------

Nice test. Thanks very much.

::: js/src/jsapi-tests/testJSEvaluateScript.cpp
@@ +15,5 @@
> +
> +    static const char src[] = "var x = 5;";
> +
> +    JS::Value retval;
> +    CHECK(JS_EvaluateScript(cx, obj, src, sizeof(src) - 1, "", 0, &retval));

Nit: `__FILE__, __LINE__,` rather than `"", 0,` please, just for luck.

@@ +33,5 @@
> +
> +    CHECK(JS_EvaluateScript(cx, obj, src2, sizeof(src2) - 1, "", 0, &retval));
> +
> +    hasProp = JS_FALSE;
> +    CHECK(JS_AlreadyHasOwnProperty(cx, obj, "y", &hasProp));

Use plain JS_HasProperty.  The only special thing about JS_AlreadyHasOwnProperty is that it lets you peek at a particular implementation detail (resolve hooks) that isn't relevant here.

@@ +41,5 @@
> +    CHECK(JS_AlreadyHasOwnProperty(cx, global, "y", &hasProp));
> +    CHECK(!hasProp);
> +
> +    // And restore the context options
> +    JS_SetOptions(cx, options);

Oh, don't bother -- for the same reason that you didn't bother deleting that "x" property you created on the global: all this stuff is 100% throwaway. We'll create a fresh cx for the next test.
Comment 3 Boris Zbarsky [:bz] 2012-07-21 19:38:14 PDT
Made those changes, landed http://hg.mozilla.org/integration/mozilla-inbound/rev/f142f32a98a3
Comment 4 Jason Duell [:jduell] (needinfo me) 2012-07-21 20:07:46 PDT
Backed out due to bustage

 https://hg.mozilla.org/integration/mozilla-inbound/rev/ad5866c81a03
Comment 5 Boris Zbarsky [:bz] 2012-07-21 20:27:08 PDT
This patch had nothing to do with the bustage... and it continues after the backout.
Comment 6 Boris Zbarsky [:bz] 2012-07-22 07:22:10 PDT
Relanded as http://hg.mozilla.org/integration/mozilla-inbound/rev/fb0cac0021c5
Comment 7 Ryan VanderMeulen [:RyanVM] 2012-07-22 12:44:26 PDT
https://hg.mozilla.org/mozilla-central/rev/fb0cac0021c5

Note You need to log in before you can comment on or make changes to this bug.