Last Comment Bug 732652 - Remove JS_GetScriptedCaller
: Remove JS_GetScriptedCaller
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: David Anderson [:dvander]
:
Mentors:
Depends on:
Blocks: IonMonkey
  Show dependency treegraph
 
Reported: 2012-03-02 19:12 PST by David Anderson [:dvander]
Modified: 2012-03-13 04:56 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1: introduce new API (4.79 KB, patch)
2012-03-05 16:13 PST, David Anderson [:dvander]
no flags Details | Diff | Splinter Review
fix websockets (1.31 KB, patch)
2012-03-05 16:28 PST, David Anderson [:dvander]
bugs: review+
Details | Diff | Splinter Review
fix workers (1.21 KB, patch)
2012-03-05 16:29 PST, David Anderson [:dvander]
bent.mozilla: review+
Details | Diff | Splinter Review
fix xpcshell (981 bytes, patch)
2012-03-05 16:29 PST, David Anderson [:dvander]
bobbyholley: review+
Details | Diff | Splinter Review
fix nsJSEnvironment (5.29 KB, patch)
2012-03-05 16:29 PST, David Anderson [:dvander]
mrbkap: review+
Details | Diff | Splinter Review
remove js_GetScriptedCaller (13.81 KB, patch)
2012-03-05 16:40 PST, David Anderson [:dvander]
no flags Details | Diff | Splinter Review
patch (17.89 KB, patch)
2012-03-05 21:41 PST, David Anderson [:dvander]
luke: review+
Details | Diff | Splinter Review

Description David Anderson [:dvander] 2012-03-02 19:12:08 PST
This API call is incompatible with IonMonkey, which does not create JSStackFrames. Most callers of this in the browser just need the topmost script/filename, which we can get easily in other ways.
Comment 1 David Anderson [:dvander] 2012-03-05 16:13:00 PST
Created attachment 603096 [details] [diff] [review]
part 1: introduce new API
Comment 2 David Anderson [:dvander] 2012-03-05 16:28:32 PST
Created attachment 603098 [details] [diff] [review]
fix websockets
Comment 3 David Anderson [:dvander] 2012-03-05 16:29:00 PST
Created attachment 603099 [details] [diff] [review]
fix workers
Comment 4 David Anderson [:dvander] 2012-03-05 16:29:20 PST
Created attachment 603100 [details] [diff] [review]
fix xpcshell
Comment 5 David Anderson [:dvander] 2012-03-05 16:29:39 PST
Created attachment 603101 [details] [diff] [review]
fix nsJSEnvironment
Comment 6 David Anderson [:dvander] 2012-03-05 16:40:23 PST
Created attachment 603109 [details] [diff] [review]
remove js_GetScriptedCaller

For the places where we can't get rid of the API outright, the details have been moved into the JS engine where they can be easily maintained.

Sending to try...
Comment 7 David Anderson [:dvander] 2012-03-05 21:37:35 PST
Comment on attachment 603101 [details] [diff] [review]
fix nsJSEnvironment

Note this patch removes the call to SetFrameReturnValue. Luke and I couldn't think of a use case for it - if there is one, I can add it back, but it's unlikely to work with IonMonkey anytime soon.
Comment 8 David Anderson [:dvander] 2012-03-05 21:41:02 PST
Created attachment 603162 [details] [diff] [review]
patch

New API addition and removal of the old API.
Comment 9 Bobby Holley (:bholley) (busy with Stylo) 2012-03-05 21:48:57 PST
Comment on attachment 603100 [details] [diff] [review]
fix xpcshell

I'm not familiar with this stuff but I'll trust dvander.

rs=bholley
Comment 10 Luke Wagner [:luke] 2012-03-06 09:48:04 PST
Comment on attachment 603162 [details] [diff] [review]
patch

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

::: js/src/jsapi.cpp
@@ +6668,5 @@
> +        *lineno = 0;
> +
> +    for (FrameRegsIter i(cx); !i.done(); ++i) {
> +        if (script)
> +            *script = i.script();

Instead of the loop, how about:

  FrameRegsIter i(cx);
  if (!i.done())
      return false;

?

::: js/src/jsapi.h
@@ +5407,5 @@
> + * Return the current script and line number of the most currently running
> + * frame. Returns true if a scripted frame was found, false otherwise.
> + */
> +extern JS_PUBLIC_API(JSBool)
> +JS_DescribeTopFrame(JSContext *cx, JSScript **script, unsigned *lineno);

I'd like to avoid any mention of "frame" in JSAPI.  How about JS_DescribeScriptedCaller?

::: js/src/jsfun.cpp
@@ +783,5 @@
>              return true;
>  
> +        StackFrame *frame = fp->prev();
> +        if (!frame)
> +            frame = js_GetTopStackFrame(cx, FRAME_EXPAND_ALL);

In view of the "if (!fp->prev())" above, this 'if' can be removed.
Comment 11 Olli Pettay [:smaug] 2012-03-06 12:16:23 PST
Comment on attachment 603098 [details] [diff] [review]
fix websockets

rs=me
Comment 12 David Anderson [:dvander] 2012-03-12 13:57:32 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/0215639e611a
Comment 13 Marco Bonardo [::mak] 2012-03-13 04:56:27 PDT
https://hg.mozilla.org/mozilla-central/rev/0215639e611a

Note You need to log in before you can comment on or make changes to this bug.