--enable-trace-jscalls build is broken

RESOLVED FIXED in mozilla11

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: sfink, Assigned: sfink)

Tracking

unspecified
mozilla11
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

jsdbgapi.h was removed from jscntxt.h, which seems like a reasonable idea. But it breaks building with --enable-trace-jscalls because jscntxt.h uses the type JSFunctionCallback defined in jsdbgapi.h.

I added the type to jsfriendapi.h, but I'm really not sure what belongs in that file. It smells like it might be turning into the new dumping ground, replacing jsdbgapi.h. So for now, I did not move the two API calls (JS_{Set,Get}FunctionCallback) into jsfriendapi; instead, I left them in jsdbgapi and included jsfriendapi from there. This is probably all horribly wrong, so I'll tag Waldo for review since he seems to be a hygiene expert.
Created attachment 574660 [details] [diff] [review]
Move code around
Assignee: general → sphink
Status: NEW → ASSIGNED
Attachment #574660 - Flags: review?(jwalden+bmo)
Blocks: 580055
Comment on attachment 574660 [details] [diff] [review]
Move code around

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

I'm inclined to agree that jsfriendapi.h is becoming a dumping ground.  It is at least a segregated dumping ground.  But it's still a dumping ground.

That said.  If a method declaration in jsdbgapi.h requires some other declaration, that declaration should live in jsdbgapi.h, I think.  And if jscntxt.h requires that "some other declaration", then jscntxt.h should include it.  It's not clear to me why it should be a good thing for jscntxt.h to not include jsdbgapi.h if it can help it.  (Right now it clearly can't, of course.)

But maybe I'm missing something here.  r- from me, but maybe Luke will see some reason why I'm wrong about this, and if so I'm sure he'll point it out and inform me.
Attachment #574660 - Flags: review?(luke)
Attachment #574660 - Flags: review?(jwalden+bmo)
Attachment #574660 - Flags: review-

Comment 3

6 years ago
Comment on attachment 574660 [details] [diff] [review]
Move code around

Agree on not making jsfriendapi a dumping guard.  I think this is a good time to add that new dir sfink and I talked about to move the growing mass of profiling/instrumentation code/files (what was the name we came to?  was it devtools?).  Anyhow, you could then have, say, devtools/TraceCalls.h that would be #included by jscntxt.h.
Attachment #574660 - Flags: review?(luke) → review-
(In reply to Luke Wagner [:luke] from comment #3)
> 
> Agree on not making jsfriendapi a dumping guard.  I think this is a good
> time to add that new dir sfink and I talked about to move the growing mass
> of profiling/instrumentation code/files (what was the name we came to?  was
> it devtools?).  Anyhow, you could then have, say, devtools/TraceCalls.h that
> would be #included by jscntxt.h.

Yes, I have a stack of patches that all add stuff into js/devtools/.

These API entries need to be accessible to friendly external users (eg bug 580055). I don't know what the intent is for the eventual set of INSTALLED_HEADERS. Is it ok to have TraceCalls.h in that list? (As I understand it, all installed headers get flattened, so the name would be a little ambiguous.)
Any eta on this patch?
Created attachment 578611 [details] [diff] [review]
Move JS_SetFunctionCallback to JSAPI

(In reply to Andrew Halberstadt [:ahal] from comment #5)
> Any eta on this patch?

Sorry, I was off consuming the burned carcass of an overgrown chicken.

We decided to pull this into jsapi.h.
Attachment #574660 - Attachment is obsolete: true
Attachment #578611 - Flags: review?(luke)

Updated

6 years ago
Attachment #578611 - Flags: review?(luke) → review+
https://hg.mozilla.org/mozilla-central/rev/ab3a8fb0893d
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
You need to log in before you can comment on or make changes to this bug.