Closed Bug 1069694 Opened 10 years ago Closed 10 years ago

Kill OldDebugAPI

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: evilpie, Assigned: evilpie)

References

Details

Attachments

(11 files)

      No description provided.
As far as I can tell the same thing exists in jsapi.h already.
Assignee: nobody → evilpies
Attached patch move-pc-to-lineSplinter Review
Moves JS_PCToLineNumber to jsfriendapi and removes the unused JSContext arg.
Remove JS_LineNumberToPC
Attached patch move-defineSplinter Review
Moves JS_DefineProfilingFunctions and JS_DefineDebuggerObject to jsapi.h
Removes JS_GetScriptSourceMap.
Removes unimplemented JS_GetScriptLineExtent.
Attached patch pc-countSplinter Review
Removes JS_DumpPCCounts and moves JS_DumpCompartmentPCCounts to jsopcode.h. Only used by shell.
Attached patch dumpstackSplinter Review
Moves FormatStackDump to jsfriendapi.h
Attachment #8491942 - Attachment is patch: true
Anybody who wants to take those super simple reviews? (Currently doing a browser build, probably have to switch some includes from OldDebugAPI to friendapi.)
Status: NEW → ASSIGNED
Depends on: 1060936
Attachment #8491935 - Flags: review?(shu)
Attachment #8491936 - Flags: review?(shu)
Attachment #8491937 - Flags: review?(shu)
Attachment #8491938 - Flags: review?(shu)
Attachment #8491939 - Flags: review?(shu)
Attachment #8491940 - Flags: review?(shu)
Attachment #8491941 - Flags: review?(shu)
Attachment #8491942 - Flags: review?(shu)
Attachment #8491943 - Flags: review?(shu)
Attachment #8491935 - Flags: review?(shu) → review+
Comment on attachment 8491936 [details] [diff] [review]
remove-decompile-script

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

Good catch.
Attachment #8491936 - Flags: review?(shu) → review+
Attachment #8491937 - Flags: review?(shu) → review+
Comment on attachment 8491938 [details] [diff] [review]
remove-line-to-pc

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

Why is this removed instead of moved to jsfriendapi?
Attachment #8491939 - Flags: review?(shu) → review+
Comment on attachment 8491940 [details] [diff] [review]
remove-get-sourcemap

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

Ditto: why is this removed instead of moved? Because of no users in Gecko?
Comment on attachment 8491942 [details] [diff] [review]
pc-count

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

::: js/src/jsopcode.cpp
@@ +275,5 @@
>          ionCounts = ionCounts->previous();
>      }
>  }
>  
> +

Nit: extra newline.
Attachment #8491942 - Flags: review?(shu) → review+
Attachment #8491943 - Flags: review?(shu) → review+
I left the patches that remove API up for now. I'm not sure what our policy on removing functions that have no users in Gecko is. Do we care about possible use by embedders? We don't, I suppose, since we change the API all the time. Jason?
Flags: needinfo?(jorendorff)
Comment on attachment 8491938 [details] [diff] [review]
remove-line-to-pc

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

Waldo convinced me that pcs are a bad idea on public APIs anyways.
Attachment #8491938 - Flags: review?(shu) → review+
Attachment #8491941 - Flags: review?(shu) → review+
Checked in all the r+ patches.
https://tbpl.mozilla.org/?tree=Try&rev=ce0692f9bcb2
https://hg.mozilla.org/integration/mozilla-inbound/rev/db6a403deb38

So we are left with JS_GetFunctionScript, JS_GetScriptFilename, JS_GetScriptSourceMap and JS_GetScriptBaseLineNumber. JSTrapStatus needs to move as well. The dependency needs to be fixed and we should be able to remove the file.
Keywords: leave-open
(In reply to Shu-yu Guo [:shu] from comment #14)
> I left the patches that remove API up for now. I'm not sure what our policy
> on removing functions that have no users in Gecko is. Do we care about
> possible use by embedders? We don't, I suppose, since we change the API all
> the time. Jason?

We don't. Kill it.
Flags: needinfo?(jorendorff)
Attachment #8491940 - Flags: review?(shu) → review+
Attached patch finallySplinter Review
Remove OldDebugAPI \o/. Fixes js/src. I moved the last three functions to jsapi.h. JSTrapStatus moved to Debugger.h, I have a patch to rename this enum later.
Attachment #8502145 - Flags: review?(shu)
Attached patch finally-browserSplinter Review
I don't think there is anything surprising here. I don't know if the comment in TestHarness.h is still correct.
Attachment #8502146 - Flags: review?(shu)
Comment on attachment 8502146 [details] [diff] [review]
finally-browser

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

::: xpcom/tests/TestHarness.h
@@ +18,5 @@
>   * Including js/OldDebugAPI.h may cause build break with --disable-shared-js
>   * This is a workaround for bug 673616.
>   */
>  #define STATIC_JS_API
>  #endif

I'm guessing this #if is no longer needed, if there is no OldDebugAPI.h to include. I also think even this comment is outdated. Reading bug 673616, it had to do with linking failures due to when profiling function headers lived in jsdbgapi.h (which got renamed to vm/OldDebugAPI.h and had profiling functions moved).

I'll needinfo Makoto, since he pushed this hunk.
Attachment #8502146 - Flags: review?(shu) → review+
Comment on attachment 8502145 [details] [diff] [review]
finally

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

Huzzah

::: js/src/vm/Debugger.h
@@ +26,5 @@
> +    JSTRAP_CONTINUE,
> +    JSTRAP_RETURN,
> +    JSTRAP_THROW,
> +    JSTRAP_LIMIT
> +} JSTrapStatus;

This can probably be moved into js:: and renamed TrapStatus. With the removal of jsd1 I don't think there are any external users of this enum. Feel free to do that in a followup patch.
Attachment #8502145 - Flags: review?(shu) → review+
Makoto, is the #if block referred to in comment 23 still needed?
Flags: needinfo?(m_kato)
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/81af1b9eba5d
https://hg.mozilla.org/mozilla-central/rev/ca047d5fafba
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
(In reply to Shu-yu Guo [:shu] from comment #25)
> Makoto, is the #if block referred to in comment 23 still needed?

if needed, I will file new bug or fix it after this.
Flags: needinfo?(m_kato)
Blocks: 1082679
You need to log in before you can comment on or make changes to this bug.