Last Comment Bug 702740 - --enable-trace-jscalls build is broken
: --enable-trace-jscalls build is broken
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla11
Assigned To: Steve Fink [:sfink] [:s:]
:
Mentors:
Depends on:
Blocks: 580055
  Show dependency treegraph
 
Reported: 2011-11-15 13:12 PST by Steve Fink [:sfink] [:s:]
Modified: 2011-12-03 23:55 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Move code around (2.47 KB, patch)
2011-11-15 13:14 PST, Steve Fink [:sfink] [:s:]
luke: review-
jwalden+bmo: review-
Details | Diff | Review
Move JS_SetFunctionCallback to JSAPI (6.83 KB, patch)
2011-12-02 09:10 PST, Steve Fink [:sfink] [:s:]
luke: review+
Details | Diff | Review

Description Steve Fink [:sfink] [:s:] 2011-11-15 13:12:36 PST
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.
Comment 1 Steve Fink [:sfink] [:s:] 2011-11-15 13:14:10 PST
Created attachment 574660 [details] [diff] [review]
Move code around
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-16 10:30:13 PST
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.
Comment 3 Luke Wagner [:luke] 2011-11-16 10:40:30 PST
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.
Comment 4 Steve Fink [:sfink] [:s:] 2011-11-16 11:32:38 PST
(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.)
Comment 5 Andrew Halberstadt [:ahal] 2011-11-29 14:27:21 PST
Any eta on this patch?
Comment 6 Steve Fink [:sfink] [:s:] 2011-12-02 09:10:00 PST
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.
Comment 7 Marco Bonardo [::mak] 2011-12-03 03:28:21 PST
https://hg.mozilla.org/mozilla-central/rev/ab3a8fb0893d

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