Move JS execution routines from nsIScriptContext to nsJSUtils

RESOLVED FIXED in mozilla26

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla26
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

(Assignee)

Description

5 years ago
This is a continuation of the work I did in bug 824864.
(Assignee)

Comment 2

5 years ago
Created attachment 785524 [details] [diff] [review]
Part 1 - Remove unused mExecuteDepth machinery. v1
Attachment #785524 - Flags: review?(bzbarsky)
(Assignee)

Comment 3

5 years ago
Created attachment 785525 [details] [diff] [review]
Part 2 - Hoist EvaluateString into nsJSUtils. v1
Attachment #785525 - Flags: review?(bzbarsky)
(Assignee)

Comment 4

5 years ago
Created attachment 785526 [details] [diff] [review]
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.
Attachment #785526 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
Blocks: 901362
(Assignee)

Comment 5

5 years ago
Created attachment 785527 [details] [diff] [review]
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.
Attachment #785527 - Flags: review?(bzbarsky)
Comment on attachment 785524 [details] [diff] [review]
Part 1 - Remove unused mExecuteDepth machinery. v1

Rev the IID, and r=me
Attachment #785524 - Flags: review?(bzbarsky) → review+
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.
Attachment #785525 - Flags: review?(bzbarsky) → review+
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.
Attachment #785526 - Flags: review?(bzbarsky) → review-
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.  :(
Attachment #785527 - Flags: review?(bzbarsky) → review-
(Assignee)

Updated

5 years ago
Blocks: 902718
(Assignee)

Updated

5 years ago
No longer blocks: 901106
(Assignee)

Comment 10

5 years ago
Created attachment 787267 [details] [diff] [review]
Part 3 - Inline CompileScript into the one caller. v2
Attachment #787267 - Flags: review?(bzbarsky)
(Assignee)

Comment 11

5 years ago
Created attachment 787268 [details] [diff] [review]
Part 4 - Inline ExecuteScript in the one caller. v2
Attachment #785526 - Attachment is obsolete: true
Attachment #785527 - Attachment is obsolete: true
Attachment #787268 - Flags: review?(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.
Attachment #787267 - Flags: review?(bzbarsky) → review+
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.
Flags: needinfo?(bobbyholley+bmo)
(Assignee)

Comment 14

5 years ago
Created attachment 787641 [details] [diff] [review]
Part 4 - Inline ExecuteScript in the one caller. v3
Attachment #787268 - Attachment is obsolete: true
Attachment #787268 - Flags: review?(bzbarsky)
Attachment #787641 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
Flags: needinfo?(bobbyholley+bmo)
Comment on attachment 787641 [details] [diff] [review]
Part 4 - Inline ExecuteScript in the one caller. v3

r=me
Attachment #787641 - Flags: review?(bzbarsky) → review+
You need to log in before you can comment on or make changes to this bug.