901162 - Move JS execution routines from nsIScriptContext to nsJSUtils

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Bobby Holley (:bholley)]

This is a continuation of the work I did in bug 824864.

Comment 1

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=650a5d0794c3

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 1 - Remove unused mExecuteDepth machinery. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 2 - Hoist EvaluateString into nsJSUtils. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 3 - Inline CompileScript into the one caller. v1

Given that this is just XUL stuff, I don't think we need the microtask and
script-disabled checks.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 4 - Inline ExecuteScript in the one caller. v1

Given that this is just XUL stuff, I don't think we need the microtask and
script-disabled checks.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 785524 [details] [diff] [review]
Part 1 - Remove unused mExecuteDepth machinery. v1

Rev the IID, and r=me

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 785525 [details] [diff] [review]
Part 2 - Hoist EvaluateString into nsJSUtils. v1

If !mScriptsEnabled && aRetValue, we need to set *aRetValue to JS::UndefinedValue().

>+  // Scope the JSAutoCompartment so that it gets destroyed before we pop the
>+  // cx and potentially call JS_RestoreFrameChain.

The bit about popping the cx doesn't make sense in the new setup, since we're not pushing the cx ourselves.  Is the JS_RestoreFrameChain bit just talking about what happens when we pop?

On the other hand, should we be asserting that the cx is already pushed?

>+  // Wrap the return value into whatever aCx was in.

"compartment" went AWOL from this comment.

r=me with the issues above addressed.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 785526 [details] [diff] [review]
Part 3 - Inline CompileScript into the one caller. v1

There is no microtask around at all, so not sure what that part of the commit comment is talking about.

But I'm not convinced about the behavior change to always run scripts in remote XUL... yes, I know it's an edge case to have that and turn off script, but seems like we still shouldn't run script then.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 785527 [details] [diff] [review]
Part 4 - Inline ExecuteScript in the one caller. v1

I do think we want the microtask here, though, in addition to the comments about not executing if script is disabled.  :(

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 3 - Inline CompileScript into the one caller. v2

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 4 - Inline ExecuteScript in the one caller. v2

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 787267 [details] [diff] [review]
Part 3 - Inline CompileScript into the one caller. v2

Don't we need to bail on mLangVersion being UNKNOWN?  Or can that not happen?

Also, we need to unmarkgray the scope.

r=me with those fixed.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 787268 [details] [diff] [review]
Part 4 - Inline ExecuteScript in the one caller. v2

Why is it ok to drop the unmarkgray calls?

Why is it safe to no longer do the things ScriptEvaluated used to do?  Especially the operation callback bits.

Comment 14

[Bobby Holley (:bholley)]

Attachment: Part 4 - Inline ExecuteScript in the one caller. v3

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 787641 [details] [diff] [review]
Part 4 - Inline ExecuteScript in the one caller. v3

r=me

Comment 16

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b0b4166c8807
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c4b14042bc32
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a429d61544af
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5ede20316d32

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b0b4166c8807
https://hg.mozilla.org/mozilla-central/rev/c4b14042bc32
https://hg.mozilla.org/mozilla-central/rev/a429d61544af
https://hg.mozilla.org/mozilla-central/rev/5ede20316d32