880330 - Refactor ContextOptions from a bitfield into a proper struct

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bobby Holley (:bholley)]

Right now, they live on the JSContext, which makes no sense. When we want to JIT something, we read these flags off whatever JSContext we happen to be using, which often gives us the wrong behavior (see bug 776798).

This is also something that needs to happen for the de-cxification work.

Comment 1

[Jesse Ruderman]

Will this allow shell scripts to control JIT options (including eagerness) for each global?  Because that would be awesome for differential testing.

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Jesse Ruderman from comment #1)
> Will this allow shell scripts to control JIT options (including eagerness)
> for each global?  Because that would be awesome for differential testing.

Eagerness is a kind of "process-wide" setting right now, both in the browser and shell. We could expose a function to the shell to change these values though.

Comment 3

[Jesse Ruderman]

Setting --ion-eager globally, and then turning off ion entirely for one global, would work too.

Comment 4

[Jesse Ruderman]

This would allow me to greatly expand differential testing (see also bug 909997).

Comment 5

[Bobby Holley (:bholley)]

Eddy is going to take this \o/

Comment 7

[Luke Wagner [:luke]]

I was thinking: could the flags instead be on the JSRuntime?  I know we have separate chrome/content options, but we also, inside the JS engine, have cx->runningWithTrustedPrincipals() so the JSRuntime could just have separate ionJitInTrustedCode/ionJitInUntrustedCode options.

Comment 8

[Bobby Holley (:bholley)]

(In reply to Luke Wagner [:luke] from comment #7)
> I was thinking: could the flags instead be on the JSRuntime?  I know we have
> separate chrome/content options, but we also, inside the JS engine, have
> cx->runningWithTrustedPrincipals() so the JSRuntime could just have separate
> ionJitInTrustedCode/ionJitInUntrustedCode options.

I think there are at least some flags that we want to be per-compartment (like strict), at which point we'll need that machinery anyway.

Comment 9

[Luke Wagner [:luke]]

Anything other than strict?  Btw, who is running with strict?

Comment 10

[Bobby Holley (:bholley)]

Well also, I think we _want_ to be able to twiddle the jit flags on a per-compartment basis, for fuzz testing and whatnot.

Comment 11

[Luke Wagner [:luke]]

I know we want to twiddle JIT flags dynamically; I wasn't aware of the desire to do it on a per-compartment basis.  If we do really need to be able to set certain options on an ad hoc per-compartment basis, I still think we should have JSRuntime-global flags but also allow a JSCompartment to optionally override them.  If we wanted to add special (enable|disable)(Ion|Baseline|Strict)InThisCompartment functions, I think that's the behavior that we'd want (i.e., when you flip javascript.options.ion.content, you don't clobber the flag in a compartment that has explicitly set the flag.

My underlying concern here is, based on the bad experience with putting the flags on JSContext, having it be simple to know that we are correctly propagating flags everywhere we should and there are a lot fewer JSRuntimes and places manipulating JSRuntimes than JSCompartments.

Comment 12

[Bobby Holley (:bholley)]

(In reply to Luke Wagner [:luke] from comment #11)
> I know we want to twiddle JIT flags dynamically; I wasn't aware of the
> desire to do it on a per-compartment basis.

See comment 1.

If we do really need to be able
> to set certain options on an ad hoc per-compartment basis, I still think we
> should have JSRuntime-global flags but also allow a JSCompartment to
> optionally override them.  If we wanted to add special
> (enable|disable)(Ion|Baseline|Strict)InThisCompartment functions, I think
> that's the behavior that we'd want (i.e., when you flip
> javascript.options.ion.content, you don't clobber the flag in a compartment
> that has explicitly set the flag.

That sounds reasonable.

> My underlying concern here is, based on the bad experience with putting the
> flags on JSContext, having it be simple to know that we are correctly
> propagating flags everywhere we should and there are a lot fewer JSRuntimes
> and places manipulating JSRuntimes than JSCompartments.

That's a fair point. I think this is much easier to do with JSCompartments/globals than it is with JSContexts (since the former don't fly all over the place), but I'd be fine to put it directly on the runtime too and add overrides wherever we need them.

Comment 13

[Eddy Bruel [:ejpbruel]]

Attachment: patch

As a first step, I want to get rid of the options bitfield on the JSContext, and replace it with a struct with proper getter/setters. Since everything else kind of hinges on this, I'd like to make sure you agree with the approach. Let me know what you think of the proposed patch.

Open questions:
- Do we really need a JS::Options function in JSAPI? If so, does it really need to be in the JS namespace? (I don't know what namespace is supposed to contain what).

- Do we really need these getter/setters on ContextOptions? Currently, they don't have any side effects, but that may change in the future, and having them around can't hurt. Also, setters returning a reference to this allows us to chain setter calls, which improves readability.

Comment 14

[Bobby Holley (:bholley)]

Comment on attachment 803897 [details] [diff] [review]
patch

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

> Open questions:
> - Do we really need a JS::Options function in JSAPI?

As opposed to what? We need some way to toggle these things. I'm somewhat open to leaving the bitfield for context options (since that's all going away), but the name JS_{Get,Set}Options would probably need to change, at which point we might as well just eliminate the bitfield entirely.

> If so, does it really
> need to be in the JS namespace? (I don't know what namespace is supposed to
> contain what).

I think it should be in namespace JS.

> - Do we really need these getter/setters on ContextOptions? Currently, they
> don't have any side effects, but that may change in the future, and having
> them around can't hurt. Also, setters returning a reference to this allows
> us to chain setter calls, which improves readability.

One option would be to do what CompileOptions does here, and use setters but not getters. I've often wondered what's preferred here. Let's let luke make the call.

Comment 15

[Luke Wagner [:luke]]

Making ContextOptions similar to CompileOptions is a generally good goal, but I think it was a mistake to give CompileOptions public fields.  I just moved two CompileOptions fields to be private so I could so something interesting.  So let's do what you did: private fields w/ getters/setters.

Also, since we're moving away from "Context" (what *is* a Context anyway?), how about JS::RuntimeOptions (contrasts nicely with JS::CompileOptions)?

Comment 16

[Bobby Holley (:bholley)]

(In reply to Luke Wagner [:luke] from comment #15)
> Making ContextOptions similar to CompileOptions is a generally good goal,
> but I think it was a mistake to give CompileOptions public fields.  I just
> moved two CompileOptions fields to be private so I could so something
> interesting.  So let's do what you did: private fields w/ getters/setters.
> 
> Also, since we're moving away from "Context" (what *is* a Context anyway?),
> how about JS::RuntimeOptions (contrasts nicely with JS::CompileOptions)?

Because, at the moment, some of these options need to live on the context (like JSOPTION_NO_DEFAULT_COMPARTMENT_OBJECT). Eddy is going to make a separate structure for runtime options, and migrate what he can.

Comment 17

[Luke Wagner [:luke]]

Ohhh, I missed that.  So the goal of ContextOptions is for it to eventually be very small or empty?

Comment 18

[Bobby Holley (:bholley)]

(In reply to Luke Wagner [:luke] from comment #17)
> Ohhh, I missed that.  So the goal of ContextOptions is for it to eventually
> be very small or empty?

Yes. Empty may or may not happen before JSContext goes away entirely from the perspective of embedders.

Comment 19

[Eddy Bruel [:ejpbruel]]

Attachment: Replace the options bitfield on JSContext with a struct

This patch replaces the options bitfield on JSContext with a struct ContextOptions. This struct can be referenced using JS::Options(cx).

I've made sure the old options API forwards to the new one, so we can upgrade consumers one by one.

Once all consumers have been upgraded to use the new options API, the old API can be removed.

Comment 20

[Eddy Bruel [:ejpbruel]]

Attachment: Upgrade the shell to use the new options API

This patch upgrades the JS shell to use the new options API.

Comment 21

[Eddy Bruel [:ejpbruel]]

There is a common pattern that we use where we save the old context options, toggle one or more options, then restore the old context options.

Would it make sense to have a helper struct like this:

...
{
    ContextOptionScope scope(cx); // Store both a copy of and a reference to Options(cx)
    scope->setExtraWarnings(true); // operator-> returns the stored reference to Options(cx)
    scope->setWerror(true);
    ... // Do stuff
}
... // Assign the copy of Options(cx) back to the reference to Options(cx)

Comment 22

[Eddy Bruel [:ejpbruel]]

Comment on attachment 804657 [details]
Replace the options bitfield on JSContext with a struct

This patch had some build errors on Linux. I have a new patch coming up that also adds a AutoSetContextOptions RAII class.

Comment 23

[Eddy Bruel [:ejpbruel]]

Attachment: Replace options bitfield in JSContext with a struct

New and improved patch which presumably doesnt break on Linux (Linux doesn't like me shadowing getter functions with variables of the same name).

Comment 24

[Eddy Bruel [:ejpbruel]]

Attachment: Refactor the shell to use the new options API

I've improved this patch to use the AutoSetContextOptions RAII class I added in the previous patch. This makes it less likely for people to forget to reset the options back to their old values. Although not that useful within the shell code, it makes things easier to read in my opinion, and it will allow us to drop a lot of JS_SetOptions(cx, oldOptions) calls in error handling code in xpconnect.

Comment 25

[Eddy Bruel [:ejpbruel]]

I need to go for a while, but I have more patches coming up later today.

Comment 26

[Bobby Holley (:bholley)]

Comment on attachment 805399 [details] [diff] [review]
Replace options bitfield in JSContext with a struct

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

Nice. ContextOptions is kinda big, and I'm mildly concerned about the impact on compile times given that jsapi gets included everywhere. But given how much this stuff is in flux, I don't think this is the time to optimize that.

I assume there are followup patches to use AutoSaveContextOptions in the browser?

Comment 27

[Bobby Holley (:bholley)]

Oh, actually - GetContextOptionsRef should be ContextOptionsRef. r=me with that for comment 26.

Comment 28

[Bobby Holley (:bholley)]

Comment on attachment 805403 [details] [diff] [review]
Refactor the shell to use the new options API

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

This is great cleanup. r=bholley with comments addressed.

::: js/src/shell/js.cpp
@@ +1023,5 @@
> +
> +        {
> +            JS::AutoSaveContextOptions asco(cx);
> +            JS::GetContextOptionsRef(cx).setCompileAndGo(compileAndGo)
> +                                        .setNoScriptRval(noScriptRval);

We're using namespace JS here, so no need for the JS:: prefixing. This happens elsewhere in the patch as well - please fix all of them.

@@ +2013,5 @@
> +
> +    {
> +        JS::AutoSaveContextOptions asco(cx);
> +        JS::GetContextOptionsRef(cx).setCompileAndGo(true)
> +                                 .setNoScriptRval(true);

line up the dots?

Comment 29

[Eddy Bruel [:ejpbruel]]

(In reply to Bobby Holley (:bholley) from comment #28)
> Comment on attachment 805403 [details] [diff] [review]
> Refactor the shell to use the new options API
> 
> Review of attachment 805403 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is great cleanup. r=bholley with comments addressed.
> 
> ::: js/src/shell/js.cpp
> @@ +1023,5 @@
> > +
> > +        {
> > +            JS::AutoSaveContextOptions asco(cx);
> > +            JS::GetContextOptionsRef(cx).setCompileAndGo(compileAndGo)
> > +                                        .setNoScriptRval(noScriptRval);
> 
> We're using namespace JS here, so no need for the JS:: prefixing. This
> happens elsewhere in the patch as well - please fix all of them.
> 

That is deliberate. Removing the JS:: prefix causes clang to complain:

error: use of undeclared identifier 'ContextOptionsRef'; did you mean 'JS::ContextOptionsRef

I don't see any using namespace JS declaration in the file either. Should we add one?

For some strange reason, CompileOptions (which is also defined in namespace JS) seems to work without a prefix, but ContextOptions does not. I can't figure out why, so I gathered it is probably best to leave things like this.

Comment 30

[Eddy Bruel [:ejpbruel]]

(In reply to Eddy Bruel [:ejpbruel] from comment #29)
> (In reply to Bobby Holley (:bholley) from comment #28)
> > Comment on attachment 805403 [details] [diff] [review]
> > Refactor the shell to use the new options API
> > 
> > Review of attachment 805403 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This is great cleanup. r=bholley with comments addressed.
> > 
> > ::: js/src/shell/js.cpp
> > @@ +1023,5 @@
> > > +
> > > +        {
> > > +            JS::AutoSaveContextOptions asco(cx);
> > > +            JS::GetContextOptionsRef(cx).setCompileAndGo(compileAndGo)
> > > +                                        .setNoScriptRval(noScriptRval);
> > 
> > We're using namespace JS here, so no need for the JS:: prefixing. This
> > happens elsewhere in the patch as well - please fix all of them.
> > 
> 
> That is deliberate. Removing the JS:: prefix causes clang to complain:
> 
> error: use of undeclared identifier 'ContextOptionsRef'; did you mean
> 'JS::ContextOptionsRef
> 
> I don't see any using namespace JS declaration in the file either. Should we
> add one?
> 
> For some strange reason, CompileOptions (which is also defined in namespace
> JS) seems to work without a prefix, but ContextOptions does not. I can't
> figure out why, so I gathered it is probably best to leave things like this.

Nevermind! I just discovered NamespaceImports.h. Not very obvious that I needed to change that though.

Comment 31

[Eddy Bruel [:ejpbruel]]

Attachment: Refactor the jsapi-tests to use the new options API

It turns out testSlowScript.cpp is never actually compiled, and uses option flags that no longer exist, so I haven't bothered to refactor that file. Bug 917241 is the followup for that.

Comment 32

[Eddy Bruel [:ejpbruel]]

Attachment: Refactor xpconnect to use the new options API

Comment 33

[Bobby Holley (:bholley)]

Comment on attachment 806011 [details] [diff] [review]
Refactor xpconnect to use the new options API

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

This cleanup is amazing. I can't believe we did it that way in mozJSComponentLoader.

All these files should have |using namespace JS;| - please remove the JS::prefixing.

r=bholley with that.

Comment 34

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 803897 [details] [diff] [review]
patch

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

::: js/src/jscntxt.h
@@ +434,5 @@
> +        varObjFix_ = varObjFix;
> +        return *this;
> +    }
> +
> +    bool privateIsNSISupports() const { return privateIsNSISupports_; }

Dunno about the case of "nsISupports" here...

Comment 35

[Eddy Bruel [:ejpbruel]]

Attachment: Refactor xpconnect shell to use the new options API

I forgot to update the xpconnect shell as well, so here is a separate patch for that.

Comment 36

[Eddy Bruel [:ejpbruel]]

Attachment: Refactor dom to use the new options API (1/2)

Storing the default options as a member on a nJSContext makes no sense to me, because we always assign them to the options member of mContext. Whenever we need mDefaultJSOptions, we can just grab ContextOptionsRef(mContext) instead.

Moreover, the way we use mDefaultJSOptions makes it a pain to switch to the new options API. The new API isnt a bitfield but a struct, so we can't conveniently or two option sets together (as we do with 'mDefaultJSOptions |= ::JS_GetOptions(mContext);').

Since mDefaultJSOptions seems to be unnecessary to begin with, I've decided to get rid of it. Here's a patch that does that.

Flagging khuey for review because he's familiar with this code iirc. If somebody else needs to review it please be so kind as to flag that person for me.

Comment 37

[Eddy Bruel [:ejpbruel]]

Attachment: Refactor dom to use the new options API (2/2)

With that change in place, refactoring the DOM code to use the new options API is a breeze.

Fwiw, I managed to get a green try run for this stuff, so I don't think I broke any invariants with this refactor.

Comment 38

[Eddy Bruel [:ejpbruel]]

(In reply to Eddy Bruel [:ejpbruel] from comment #37)
> Created attachment 810600 [details] [diff] [review]
> Refactor dom to use the new options API (2/2)
> 
> With that change in place, refactoring the DOM code to use the new options
> API is a breeze.
> 
> Fwiw, I managed to get a green try run for this stuff, so I don't think I
> broke any invariants with this refactor.

Oh, I should also mention that I removed two assertions because they relied on bitfield comparisons. We could do that with the new options API by overloading the == and < operator for equality and subset, respectively, but I didn't feel like it was worth the effort just so we could keep two assertions.

If you think we really *should* keep those assertions, just let me know.

Comment 39

[Eddy Bruel [:ejpbruel]]

Attachment: Refactor jsd to use the new options API

I've been told sfink is the module owner for jsd, so flagging him for review ;-)

jsdContext unfortunately exposes implementation details from the old options API, by allowing consumers to access the options bitfield directly. For backwards compatibility, I've simulated the old options API in terms of the new one. I didn't bother to copy over the comments because this stuff is (hopefully) going to die some time soon anyway.

Comment 40

[Eddy Bruel [:ejpbruel]]

Attachment: Refactor crypto to use the new options API

I had to change a single line in nsCrypto.cpp. I'm not sure who to flag for this. https://wiki.mozilla.org/Modules/All says Honza is a peer for security/manager, so I'm trying him.

Comment 41

[Steve Fink [:sfink] [:s:]]

Comment on attachment 810607 [details] [diff] [review]
Refactor jsd to use the new options API

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

I checked the firebug source code, and it currently doesn't use any of these options.

Still, a firebug extension could be using it, so maintaining compatibility is still good for now. Thanks!

::: js/jsd/jsd_xpc.cpp
@@ +1667,5 @@
> +           | (JS::ContextOptionsRef(mJSCx).baseline() ? JSOPTION_BASELINE : 0)
> +           | (JS::ContextOptionsRef(mJSCx).typeInference() ? JSOPTION_TYPE_INFERENCE : 0)
> +           | (JS::ContextOptionsRef(mJSCx).strictMode() ? JSOPTION_STRICT_MODE : 0)
> +           | (JS::ContextOptionsRef(mJSCx).ion() ? JSOPTION_ION : 0)
> +           | (JS::ContextOptionsRef(mJSCx).asmJS() ? JSOPTION_ASMJS : 0);

You don't want to do

#define JSOPTION_NO_SCRIPT_RVAL 12
...
   | (JS::ContextOptionsRef(mJSCx).noScriptRval() << JSOPTION_NO_SCRIPT_RVAL)
...
?

(Just kidding. Please don't bother.)

@@ +1701,5 @@
>  NS_IMETHODIMP
>  jsdContext::GetPrivateData(nsISupports **_rval)
>  {
>      ASSERT_VALID_EPHEMERAL;
> +    if (JS::ContextOptionsRef(mJSCx).privateIsNSISupports()) 

Trailing space

Comment 42

[Eddy Bruel [:ejpbruel]]

Attachment: Remove the old options API

And with that, we can finally get rid of the old options API.

The next step will be to create a RuntimeOptions struct and migrate most options from ContextOptions to there.

Comment 43

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 810598 [details] [diff] [review]
Refactor dom to use the new options API (1/2)

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

::: dom/base/nsJSEnvironment.cpp
@@ +735,2 @@
>    else
> +    options &= ~JSOPTION_EXTRA_WARNINGS;

Since you're here, brace these ifs.

@@ +780,2 @@
>    else
> +    options &= ~JSOPTION_TYPE_INFERENCE;

and all of these too.

@@ +859,1 @@
>      // Make sure the new context gets the default context options

You should add a comment that the other options will be set when InitContext is called.

Comment 44

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 810600 [details] [diff] [review]
Refactor dom to use the new options API (2/2)

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

::: dom/base/nsJSEnvironment.cpp
@@ +773,5 @@
>  
> +  JS::ContextOptionsRef(cx).setTypeInference(useTypeInference)
> +                           .setBaseline(useBaselineJIT)
> +                           .setIon(useIon)
> +                           .setAsmJS(useAsmJS);

Or you can just get rid of the ifs entirely.  That works too ;-)

::: dom/base/nsJSUtils.cpp
@@ +209,3 @@
>      }
>    }
>    ~AutoDontReportUncaught() {

Please assert here that DontReportUncaught is set.

::: dom/workers/RuntimeService.cpp
@@ +125,5 @@
>  const uint32_t kNoIndex = uint32_t(-1);
>  
> +const JS::ContextOptions kRequiredJSContextOptions =
> +  JS::ContextOptions().setDontReportUncaught(true)
> +                      .setNoScriptRval(true);

It's unfortunate that this will introduce a static constructor.  Can we just move this into LoadJSContextOptions to avoid that?

Comment 45

[Bobby Holley (:bholley)]

Comment on attachment 810596 [details] [diff] [review]
Refactor xpconnect shell to use the new options API

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

::: js/xpconnect/shell/xpcshell.cpp
@@ +597,4 @@
>  static bool
>  Options(JSContext *cx, unsigned argc, jsval *vp)
>  {
> +    jsval *argv = JS_ARGV(cx, vp);

CallArgsFromVp

@@ +627,5 @@
> +                return false;
> +            }
> +
> +            JS_ReportError(cx, msg);
> +            free(msg);

you can just JS_ReportError(cx, someSprintfFormatString, opt.ptr())

instead of doing all that junk.

@@ +645,5 @@
> +    }
> +    if (!names && oldOptions.strictMode()) {
> +        names = JS_sprintf_append(names, "%s%s", found ? "," : "", "strict_mode");
> +        found = true;
> +    }

What's the point of |found| here? Can't we just test |names|? And moreover, what's the point of appending to the string if we _only_ get here when names is null?

I think this stuff could use a bit more thinking through, and end up drastically simpler than the old code.

Comment 46

[Bobby Holley (:bholley)]

Comment on attachment 810639 [details] [diff] [review]
Remove the old options API

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

\o/

Comment 47

[Eddy Bruel [:ejpbruel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ffa34d7c9236

Comment 48

[Eddy Bruel [:ejpbruel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e437294a2dbe

Comment 49

[Eddy Bruel [:ejpbruel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a15782f2e779

Comment 50

[Eddy Bruel [:ejpbruel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/70466645fa43

Comment 51

[Eddy Bruel [:ejpbruel]]

Attachment: Refactor xpconnect shell to use the new options API

Comment 52

[Bobby Holley (:bholley)]

Comment on attachment 816590 [details] [diff] [review]
Refactor xpconnect shell to use the new options API

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

::: js/xpconnect/src/XPCShellImpl.cpp
@@ +549,4 @@
>  static bool
>  Options(JSContext *cx, unsigned argc, jsval *vp)
>  {
> +    JS::CallArgs args = CallArgsFromVp(argc, vp);

No need for the JS:: prefixing in this file.

@@ +574,4 @@
>          }
>      }
> +
> +    char *names = NULL;

Please add some comments here explaining that this function returns a list of options (in string form) that were previously set.

@@ +579,5 @@
> +        names = JS_sprintf_append(names, "%s", "strict");
> +        if (!names) {
> +            JS_ReportOutOfMemory(cx);
> +            return false;
> +        }

Let's just replace this check and the others below with NS_ENSURE_TRUE(names, false);

It won't report OOM on the cx, but returning null without setting an exception indicates OOM anyway, and I'd rather this code be a little more concise.

Comment 53

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ffa34d7c9236
https://hg.mozilla.org/mozilla-central/rev/e437294a2dbe
https://hg.mozilla.org/mozilla-central/rev/a15782f2e779
https://hg.mozilla.org/mozilla-central/rev/70466645fa43

Comment 54

[Bobby Holley (:bholley)]

Eddy, what is the status here? We're branching this week, and I really don't want this bug to be split across releases.

Comment 55

[Eddy Bruel [:ejpbruel]]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/f03299c9d48a

Comment 56

[Eddy Bruel [:ejpbruel]]

  https://hg.mozilla.org/integration/mozilla-inbound/rev/e18d850abc31

Comment 57

[Eddy Bruel [:ejpbruel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/99a97151f20d

Comment 58

[Eddy Bruel [:ejpbruel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b7dbe16ff21

Comment 59

[Eddy Bruel [:ejpbruel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/88310e106cab

Comment 60

[Eddy Bruel [:ejpbruel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/87e380e5b6c1

Comment 61

[Eddy Bruel [:ejpbruel]]

Please leave this bug open for now. I will rename it and split two follow up bugs off from it. It can be closed after that.

Here's the try run for the patches I just landed:
https://tbpl.mozilla.org/?tree=Try&rev=384d652067a4

Comment 62

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f03299c9d48a
https://hg.mozilla.org/mozilla-central/rev/e18d850abc31
https://hg.mozilla.org/mozilla-central/rev/99a97151f20d
https://hg.mozilla.org/mozilla-central/rev/5b7dbe16ff21
https://hg.mozilla.org/mozilla-central/rev/88310e106cab
https://hg.mozilla.org/mozilla-central/rev/87e380e5b6c1

Comment 63

[Bobby Holley (:bholley)]

Eddy, does this still need to be open?

Comment 64

[Eddy Bruel [:ejpbruel]]

(In reply to Bobby Holley (:bholley) from comment #63)
> Eddy, does this still need to be open?

Nope.

Comment 65

[Jeff Walden [:Waldo]]

Eddy, this needs to be documented:

https://developer.mozilla.org/en-US/docs/SpiderMonkey/31

I just got totally confused searching for some JSOPTION in our codebase to double-check naming of it, and I only happened to learn that the entire existing API had been ripped out.  This is going to be pretty close to the top of the list of things JSAPI users are going to need to know to update, I'm sure!

Comment 66

[Eddy Bruel [:ejpbruel]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #65)
> Eddy, this needs to be documented:
> 
> https://developer.mozilla.org/en-US/docs/SpiderMonkey/31
> 
> I just got totally confused searching for some JSOPTION in our codebase to
> double-check naming of it, and I only happened to learn that the entire
> existing API had been ripped out.  This is going to be pretty close to the
> top of the list of things JSAPI users are going to need to know to update,
> I'm sure!

I'm no longer working on this, and won't be able to spend time on this any time soon. Is it enough if I file a followup bug for this?