1069694 - Kill OldDebugAPI

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tom S. (please needinfo tschuster)]

Attachment: move-debug-functions

Comment 1

[Tom S. (please needinfo tschuster)]

Attachment: remove-decompile-script

As far as I can tell the same thing exists in jsapi.h already.

Comment 2

[Tom S. (please needinfo tschuster)]

Attachment: move-pc-to-line

Moves JS_PCToLineNumber to jsfriendapi and removes the unused JSContext arg.

Comment 3

[Tom S. (please needinfo tschuster)]

Attachment: remove-line-to-pc

Remove JS_LineNumberToPC

Comment 4

[Tom S. (please needinfo tschuster)]

Attachment: move-define

Moves JS_DefineProfilingFunctions and JS_DefineDebuggerObject to jsapi.h

Comment 5

[Tom S. (please needinfo tschuster)]

Attachment: remove-get-sourcemap

Removes JS_GetScriptSourceMap.

Comment 6

[Tom S. (please needinfo tschuster)]

Attachment: remove-lineextent

Removes unimplemented JS_GetScriptLineExtent.

Comment 7

[Tom S. (please needinfo tschuster)]

Attachment: pc-count

Removes JS_DumpPCCounts and moves JS_DumpCompartmentPCCounts to jsopcode.h. Only used by shell.

Comment 8

[Tom S. (please needinfo tschuster)]

Attachment: dumpstack

Moves FormatStackDump to jsfriendapi.h

Comment 9

[Tom S. (please needinfo tschuster)]

Anybody who wants to take those super simple reviews? (Currently doing a browser build, probably have to switch some includes from OldDebugAPI to friendapi.)

Comment 10

[Shu-yu Guo [:shu]]

Comment on attachment 8491936 [details] [diff] [review]
remove-decompile-script

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

Good catch.

Comment 11

[Shu-yu Guo [:shu]]

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?

Comment 12

[Shu-yu Guo [:shu]]

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 13

[Shu-yu Guo [:shu]]

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.

Comment 14

[Shu-yu Guo [:shu]]

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?

Comment 15

[Shu-yu Guo [:shu]]

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.

Comment 16

[Tom S. (please needinfo tschuster)]

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.

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/db6a403deb38

Comment 18

[Jason Orendorff [:jorendorff]]

(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.

Comment 19

[Tom S. (please needinfo tschuster)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa8375efaefc

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/fa8375efaefc

Comment 21

[Tom S. (please needinfo tschuster)]

Attachment: finally

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.

Comment 22

[Tom S. (please needinfo tschuster)]

Attachment: finally-browser

I don't think there is anything surprising here. I don't know if the comment in TestHarness.h is still correct.

Comment 23

[Shu-yu Guo [: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.

Comment 24

[Shu-yu Guo [:shu]]

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.

Comment 25

[Shu-yu Guo [:shu]]

Makoto, is the #if block referred to in comment 23 still needed?

Comment 26

[Tom S. (please needinfo tschuster)]

Try push looked good without that STATIC_JS_API hunk so I pushed: https://tbpl.mozilla.org/?tree=Try&rev=2ebc46e8df04
https://hg.mozilla.org/integration/mozilla-inbound/rev/81af1b9eba5d
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca047d5fafba

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/81af1b9eba5d
https://hg.mozilla.org/mozilla-central/rev/ca047d5fafba

Comment 28

[Makoto Kato [:m_kato]]

(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.