Closed
Bug 1069694
Opened 10 years ago
Closed 10 years ago
Kill OldDebugAPI
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: evilpie, Assigned: evilpie)
References
Details
Attachments
(11 files)
2.38 KB,
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
898 bytes,
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
12.06 KB,
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
3.07 KB,
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
2.46 KB,
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
2.49 KB,
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
1.53 KB,
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
6.65 KB,
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
16.56 KB,
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
16.73 KB,
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
20.14 KB,
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•10 years ago
|
||
As far as I can tell the same thing exists in jsapi.h already.
Assignee: nobody → evilpies
Assignee | ||
Comment 2•10 years ago
|
||
Moves JS_PCToLineNumber to jsfriendapi and removes the unused JSContext arg.
Assignee | ||
Comment 3•10 years ago
|
||
Remove JS_LineNumberToPC
Assignee | ||
Comment 4•10 years ago
|
||
Moves JS_DefineProfilingFunctions and JS_DefineDebuggerObject to jsapi.h
Assignee | ||
Comment 5•10 years ago
|
||
Removes JS_GetScriptSourceMap.
Assignee | ||
Comment 6•10 years ago
|
||
Removes unimplemented JS_GetScriptLineExtent.
Assignee | ||
Comment 7•10 years ago
|
||
Removes JS_DumpPCCounts and moves JS_DumpCompartmentPCCounts to jsopcode.h. Only used by shell.
Assignee | ||
Comment 8•10 years ago
|
||
Moves FormatStackDump to jsfriendapi.h
Assignee | ||
Updated•10 years ago
|
Attachment #8491942 -
Attachment is patch: true
Assignee | ||
Comment 9•10 years ago
|
||
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
Assignee | ||
Updated•10 years ago
|
Attachment #8491935 -
Flags: review?(shu)
Assignee | ||
Updated•10 years ago
|
Attachment #8491936 -
Flags: review?(shu)
Assignee | ||
Updated•10 years ago
|
Attachment #8491937 -
Flags: review?(shu)
Assignee | ||
Updated•10 years ago
|
Attachment #8491938 -
Flags: review?(shu)
Assignee | ||
Updated•10 years ago
|
Attachment #8491939 -
Flags: review?(shu)
Assignee | ||
Updated•10 years ago
|
Attachment #8491940 -
Flags: review?(shu)
Assignee | ||
Updated•10 years ago
|
Attachment #8491941 -
Flags: review?(shu)
Assignee | ||
Updated•10 years ago
|
Attachment #8491942 -
Flags: review?(shu)
Assignee | ||
Updated•10 years ago
|
Attachment #8491943 -
Flags: review?(shu)
Updated•10 years ago
|
Attachment #8491935 -
Flags: review?(shu) → review+
Comment 10•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8491937 -
Flags: review?(shu) → review+
Comment 11•10 years ago
|
||
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?
Updated•10 years ago
|
Attachment #8491939 -
Flags: review?(shu) → review+
Comment 12•10 years ago
|
||
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•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8491943 -
Flags: review?(shu) → review+
Comment 14•10 years ago
|
||
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 15•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8491941 -
Flags: review?(shu) → review+
Assignee | ||
Comment 16•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Keywords: leave-open
Comment 18•10 years ago
|
||
(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)
Updated•10 years ago
|
Attachment #8491940 -
Flags: review?(shu) → review+
Assignee | ||
Comment 19•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa8375efaefc
Assignee | ||
Comment 21•10 years ago
|
||
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)
Assignee | ||
Comment 22•10 years ago
|
||
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 23•10 years ago
|
||
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 24•10 years ago
|
||
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+
Comment 25•10 years ago
|
||
Makoto, is the #if block referred to in comment 23 still needed?
Flags: needinfo?(m_kato)
Assignee | ||
Comment 26•10 years ago
|
||
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
Assignee | ||
Updated•10 years ago
|
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
Comment 28•10 years ago
|
||
(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)
You need to log in
before you can comment on or make changes to this bug.
Description
•