Closed Bug 1065180 Opened 6 years ago Closed 6 years ago

Replace the JS shell's environment object with a getenv() function

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: jorendorff, Assigned: jorendorff)

Details

Attachments

(3 files, 1 obsolete file)

No description provided.
Attached patch Part 1 - Add os.getenv(). (obsolete) — Splinter Review
Assignee: nobody → jorendorff
Attachment #8486864 - Flags: review?(sphink)
This time with the .cpp file.
Attachment #8486864 - Attachment is obsolete: true
Attachment #8486864 - Flags: review?(sphink)
Attachment #8487200 - Flags: review?(sphink)
Comment on attachment 8487200 [details] [diff] [review]
Part 1 - Add os.getenv().

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

::: js/src/shell/OSObject.cpp
@@ +3,5 @@
> + * 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/. */
> +
> +// OSObject.h - os object for exposing posix system calls as needed in the JS shell

"as needed"? Needed by what? s/as needed//

@@ +12,5 @@
> +
> +using namespace JS;
> +
> +static bool
> +os_getenv(JSContext* cx, unsigned argc, JS::Value* vp)

This is the only place you used JS::. Though come to think of it, I don't have a problem with that. Value sounds pretty generic.

@@ +17,5 @@
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    RootedString key(cx, ToString(cx, args.get(0)));
> +    if (!key)
> +        return false;

So this looks up $undefined if you don't pass the arg? Seems weird.

@@ +46,5 @@
> +    RootedObject obj(cx, JS_NewObject(cx, nullptr, JS::NullPtr(), JS::NullPtr()));
> +    return obj &&
> +           JS_DefineFunctions(cx, obj, os_functions) &&
> +           JS_DefineProperty(cx, global, "os", obj, 0);
> +}

This whole file is short and sweet. I like it.

::: js/src/shell/OSObject.h
@@ +3,5 @@
> + * 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/. */
> +
> +// OSObject.h - os object for exposing posix system calls as needed in the JS shell

s/as needed//
Attachment #8487200 - Flags: review?(sphink) → review+
Attachment #8486871 - Flags: review?(sphink) → review+
Attachment #8486872 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink] from comment #5)
> > +// OSObject.h - os object for exposing posix system calls as needed in the JS shell
> 
> "as needed"? Needed by what? s/as needed//

Heh. OK.

For the record, the answer to "Needed by what?" was clear enough to me: the handful of shell users we care about.

> > +    CallArgs args = CallArgsFromVp(argc, vp);
> > +    RootedString key(cx, ToString(cx, args.get(0)));
> > +    if (!key)
> > +        return false;
> 
> So this looks up $undefined if you don't pass the arg? Seems weird.

Hmm. I guess that is egregious enough that I should have it throw an error instead.
https://hg.mozilla.org/mozilla-central/rev/5921f2808ade
https://hg.mozilla.org/mozilla-central/rev/4b19eda14b04
https://hg.mozilla.org/mozilla-central/rev/ba05f247d98c
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.