795768 - Clean up various JSContext uses

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

I have found a bunch of simplifications involving JSContext, e.g. lots of dead parameters.  Patches coming shortly.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove lots of dead JSContext* occurrences.

This patch removes lots of dead JSContext* occurrences, mostly arguments.

- I converted a few internal JS_free() calls to js_free().  I didn't remove
  JS_free()'s JSContext* argument even though it's not used, since it's an
  API function.

- I also left JS_GetTypeName() untouched because it's an API function.

- You should check the jsprobes changes more closely than the others, 
  because that code isn't compiled by default and I haven't tested with the
  appropriate configuration(s).

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - De-macroize js_GetSrcNote().

This patch removes a loathesome and ancient (pre-Mercurial) macro that hides 
|cx| uses and caused me much grief while writing the previous patch.  Good 
riddance.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove lots of dead JSContext* occurrences.

I found another dead |cx| parameter.

Comment 4

[David Mandelin [:dmandelin]]

(In reply to Nicholas Nethercote [:njn] from comment #1)
> Created attachment 666401 [details] [diff] [review]
> (part 1) - Remove lots of dead JSContext* occurrences.
> 
> This patch removes lots of dead JSContext* occurrences, mostly arguments.
> 
> - I converted a few internal JS_free() calls to js_free().  I didn't remove
>   JS_free()'s JSContext* argument even though it's not used, since it's an
>   API function.
> 
> - I also left JS_GetTypeName() untouched because it's an API function.

Feel free to remove a few useless |cx|s from the API, if you think it helps.

Comment 5

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

Comment on attachment 666402 [details] [diff] [review]
(part 2) - De-macroize js_GetSrcNote().

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

Funny patch.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove lots of dead JSContext* occurrences.

This version removes the |cx| args from the JSAPI functions, as per
dmandelin's suggestion.

Comment 7

[Nicholas Nethercote [inactive]]

> This version removes the |cx| args from the JSAPI functions, as per
> dmandelin's suggestion.

Actually, this patch breaks the browser because I didn't make the necessary changes, and I've now decided that it's not worth the trouble.  Back to the previous patch, sorry for the noise.

Comment 8

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

Comment on attachment 666423 [details] [diff] [review]
(part 1) - Remove lots of dead JSContext* occurrences.

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

This patch seems great. I was surprised that several of these places don't need a cx (anymore?)

I should really go through the probes stuff. I bet I could get rid of most of those cx's now.

Comment 9

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

Comment on attachment 666423 [details] [diff] [review]
(part 1) - Remove lots of dead JSContext* occurrences.

Whoops, forgot the r+ mark.

Comment 10

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c656027f5c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/c92d00f777de

> I should really go through the probes stuff. I bet I could get rid of most
> of those cx's now.

I looked again and couldn't see any obviously dead ones.

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9c656027f5c2
https://hg.mozilla.org/mozilla-central/rev/c92d00f777de