Closed Bug 677079 Opened 13 years ago Closed 12 years ago

Remove jscntxt.h from INSTALLED_HEADERS

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: Ms2ger, Assigned: Ms2ger)

References

()

Details

Attachments

(28 files, 5 obsolete files)

3.68 KB, patch
igor
: review+
Details | Diff | Splinter Review
8.15 KB, patch
bholley
: review+
Details | Diff | Splinter Review
4.58 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
8.98 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
1.80 KB, patch
luke
: review+
Details | Diff | Splinter Review
4.40 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
5.63 KB, patch
bhackett1024
: review+
Details | Diff | Splinter Review
4.41 KB, patch
igor
: review+
Details | Diff | Splinter Review
3.60 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
4.88 KB, patch
cdleary
: review+
Details | Diff | Splinter Review
4.34 KB, patch
bholley
: review+
Details | Diff | Splinter Review
5.23 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
3.30 KB, patch
gal
: review+
Details | Diff | Splinter Review
4.60 KB, patch
gal
: review+
Details | Diff | Splinter Review
3.37 KB, patch
bholley
: review+
Details | Diff | Splinter Review
3.22 KB, patch
gal
: review+
Details | Diff | Splinter Review
3.95 KB, patch
luke
: review+
Details | Diff | Splinter Review
12.55 KB, patch
billm
: review+
Details | Diff | Splinter Review
2.92 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
3.17 KB, patch
bhackett1024
: review+
Details | Diff | Splinter Review
3.50 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
5.68 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
6.23 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
6.19 KB, patch
mjrosenb
: review+
Details | Diff | Splinter Review
12.72 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
2.76 KB, patch
n.nethercote
: review+
Details | Diff | Splinter Review
1.28 KB, patch
dmandelin
: review+
Details | Diff | Splinter Review
2.43 KB, patch
sfink
: review+
bent.mozilla
: review+
Details | Diff | Splinter Review
See bug 672893 comment 13 and following.

Besides bug 332648, content/xul/document/src/nsXULContentSink.cpp and content/base/src/nsScriptLoader.cpp need js::VersionFlags::HAS_XML or VersionHasXML/VersionSetXML.

:jorendorff, :luke, and :billm are ready to take a 1/6 chance of having to fix this bug in order to get it fixed; three more heroes are required.
I volunteer.  Actually, I auto-volunteer for bugs removing any of the installed headers except js*api.h
Volunteer. Non-API includes must die.
"Give me the victim, give me the victim!"

http://www.youtube.com/watch?v=Qxys99Wn_NE#t=1m23s

I am happy to volunteer, with the caveat that I think I need to finish bug 586842 before doing this, should I be the lucky victim.
$ python -c 'import random; print random.choice("jorendorff, luke, billm, bhackett, cdleary, gal, Waldo".split(", "))'
Waldo

(I'll be removing one include in bug 677101, fwiw.)
Assignee: general → jwalden+bmo
Status: NEW → ASSIGNED
Depends on: 586842
Well then!  I guess that caveat will actually matter.  (Unless we were waiting for more people to volunteer?  It wasn't clear to me that everyone had a chance to see/volunteer, not in the space of eight hours on a Sunday morning.)

I will probably be busy with that for most, if not all, of the time until August 18, when I disappear for a couple weeks of vacation.  So it's unlikely this bug will see much progress before September.
Attached patch patch (obsolete) — Splinter Review
I had an odd urge to fix this last night, so here it is.

I definitely cheated here. Although jscntxt.h is no longer part of INSTALLED_HEADERS, I whitelisted several directories by added js/src to LOCAL_INCLUDES in their Makefiles. The directories are XPConnect, jsd, and dom/base. The reason for the last one is that nsGlobalWindow.cpp defines a wrapper, and I couldn't think of a good way to disentangle the wrapper code from jscntxt.h.

Nevertheless, I think the patch is valuable. It should shorten build times and it gets us closer to the goal of a real API. The main problems that we still have to tackle are wrappers and typed arrays. I'm worried that both of these will be difficult to fix.
Assignee: jwalden+bmo → wmccloskey
Attachment #562206 - Flags: review?(jwalden+bmo)
Comment on attachment 562206 [details] [diff] [review]
patch

>--- a/dom/workers/ListenerManager.cpp
>+++ b/dom/workers/ListenerManager.cpp

>+  nsTArray<jsval> listeners;
>-    if ((eventIsTrusted || listener->mWantsUntrusted) &&
>-        !listeners.append(listener->mListenerVal)) {
>+    if (eventIsTrusted || listener->mWantsUntrusted) {
>       return false;
>     }
>+    listeners.AppendElement(listener->mListenerVal);

If you use nsTArray infallibly, can you please make it an InfallibleTArray for clarity?

>   }
> 
>-  if (listeners.empty()) {
>+  if (!listeners.Length()) {

IsEmpty()

>--- a/js/src/Makefile.in
>+++ b/js/src/Makefile.in
>-		jscntxt.h \
>-		jscrashreport.h \
>-		jsdate.h \
>-		jsemit.h \
>-		jsfun.h \
>-		jsgc.h \
>-		jsgcchunk.h \
>-		jsgcstats.h \
>-		jsinterp.h \
>-		jsiter.h \
>-		jsparse.h \
>-		jsproxy.h \
>-		jsreflect.h \
>-		jsscan.h \
>-		jsscope.h \
>-		jstracer.h \
>-		jswrapper.h \

\o/

>-/* FIXME(bug 332648): Move this into a public header. */

Don't forget to close that bug.

>--- a/js/src/jsdbgapi.h
>+++ b/js/src/jsdbgapi.h
>+#include "jsprvtd.h"
> #include "jsopcode.h"
>-#include "jsprvtd.h"

Why this change?

>--- a/js/src/jsfriendapi.cpp
>+++ b/js/src/jsfriendapi.cpp
>+JS_FRIEND_API(JSVersion)
>+JS_VersionSetXML(JSVersion version, JSBool enable)
>+{
>+    js::VersionSetXML(&version, enable);

Any reason you didn't match the internal signature?


I wonder if nsXPConnect should go into a public header... That seems like a better approach than duplicating those static functions in mozilla::xpconnect. (In any case, Gecko should use those as xpconnect::Foo(), with a 'using namespace mozilla;' at the top of .cpps that aren't yet using namespaces.)

I'm not entirely sure I like all of this patch, but I do think there are a lot of improvements in it. I'll leave it to your reviewer to decide whether it's better to add the LOCAL_INCLUDES or to keep the file in INSTALLED_HEADERS for now, while we take the rest of the patch.

It's rather big, though, and I think the js_GetErrorMessage->JS_GetErrorMessage should definitely go in a separate patch, and perhaps AutoGCRooter and friends as well (if only for ease of review).
Comment on attachment 562206 [details] [diff] [review]
patch

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

So I mentioned the INSTALLED_HEADERS->LOCAL_INCLUDES thing to bhackett, and he thought it smelled a bit dodgy, too, even as a halfway step (especially since that opens Pandora's box for all the internal headers, not just for a small subset of them).  He also said you'd agreed to defer this to later, when dependencies can be cut more thoroughly, which makes sense to me.  So I'm going to cancel this review for now, and hopefully next time around we won't need this one-step-forward-one-and-a-half-steps-back approach.
Attachment #562206 - Flags: review?(jwalden+bmo)
Shouldn't we land the other parts of the patch, though? Even if we can't remove jscntxt.h from INSTALLED_HEADERS just yet, moving in that direction would be useful already.
I would probably be in favor of various other parts of the patch, but given all the parts are intermingled, it's hard to review and be sure which parts are needed for which changes.  I'm happy to review smaller, self-contained parts in new patches.
Attached patch autorooters patch (obsolete) — Splinter Review
I'm updating the this to completely remove jscntxt.h. This first patch just fixes up the autorooters. I put most of the autorooters in jsapi.h. The ones that depend on vectors I put in jsfriendapi.h. I also changed the autorooter implementation to use virtual function dispatch rather than the tagging scheme; I think this is cleaner.
Attachment #562206 - Attachment is obsolete: true
Attachment #564998 - Flags: review?(jwalden+bmo)
Attached patch main patch (obsolete) — Splinter Review
This patch mostly just adds a bunch of shadow declarations and accessors to jsfriendapi.h. This parallel's Brian's approach. There are a few other things here, but they're pretty local.

The one thing that might need explanation in the GC trigger stuff from nsJSEnvironment. Previously that code had been reading out of cx->rt->gcTriggerCompartment to figure out if we were doing a compartment GC. This was wrong, and it always got NULL (so it though every GC was a full GC). I fixed this by adding a new flag that is set correctly by the GC.
Attachment #565003 - Flags: review?(jwalden+bmo)
Comment on attachment 564998 [details] [diff] [review]
autorooters patch

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

::: js/src/jsapi.h
@@ +4840,1 @@
>  JS_END_EXTERN_C

Let's move this above this entire section, then we can avoid having oddnesses like this:

#ifdef __cplusplus
JS_END_EXTERN_C

...

JS_BEGIN_EXTERN_C
#endif /* __cplusplus */

/* yes, really there's *nothing in this space */

JS_END_EXTERN_C

@@ +4859,5 @@
> +        JS_ASSERT(this != reinterpret_cast<shadow::Context *>(cx)->autoGCRooters);
> +        reinterpret_cast<shadow::Context *>(cx)->autoGCRooters = this;
> +    }
> +
> +    ~AutoGCRooter() {

Is this not being virtual going to create Windows compiler warnings, given the virtual trace method below?  Or does the = 0 save you?

@@ +4967,5 @@
> +        JS_GUARD_OBJECT_NOTIFIER_INIT;
> +    }
> +
> +    void changeLength(size_t newLength) {
> +        length = ptrdiff_t(newLength);

This cast is no longer necessary, right?

@@ +5011,5 @@
> +};
> +
> +class AutoIdArray : private AutoGCRooter {
> +  public:
> +    AutoIdArray(JSContext *cx, JSIdArray *ida JS_GUARD_OBJECT_NOTIFIER_PARAM)

Newline and appropriate indentation after *ida.  There's no good way to express what the macro does, but at least getting it away from the arguments we expect callers to provide will hurt readability less.

::: js/src/jsfriendapi.h
@@ +300,5 @@
>  
> +static JS_ALWAYS_INLINE void
> +MakeRangeGCSafe(Value *vec, size_t len)
> +{
> +    PodZero(vec, len);

Add an assert that |vec <= vec + len|.

@@ +306,5 @@
> +
> +static JS_ALWAYS_INLINE void
> +MakeRangeGCSafe(Value *beg, Value *end)
> +{
> +    PodZero(beg, end - beg);

Add an assert that |beg <= end|.

@@ +312,5 @@
> +
> +static JS_ALWAYS_INLINE void
> +MakeRangeGCSafe(jsid *beg, jsid *end)
> +{
> +    for (jsid *id = beg; id != end; ++id)

Can you make this |id < end| while you're here?  Belt-and-suspenders protection against wild overwrites if garbage comes in.

@@ +326,5 @@
> +template<class T>
> +class AutoVectorRooter : protected AutoGCRooter
> +{
> +  public:
> +    explicit AutoVectorRooter(JSContext *cx JS_GUARD_OBJECT_NOTIFIER_PARAM)

Put the guard param on a new line.

@@ +380,5 @@
> +
> +    const T &back() const { return vector.back(); }
> +
> +  protected:
> +    typedef Vector<T, 8> VectorImpl;

It might be worth considering (not here, separate bug!) making 8 configurable.  The switch-on-tag scheme made that difficult, but with virtual dispatch it'd be pretty easy.  Worthwhile?  *shrug*  Just throwing the idea out.

@@ +401,5 @@
> +
> +    const jsval *jsval_end() const { return end(); }
> +    jsval *jsval_end() { return end(); }
> +
> +    JS_DECL_USE_GUARD_OBJECT_NOTIFIER

This should go in the protected section -- and at the end, so as not to distort offsets in debug builds versus in optimized builds.

@@ +417,5 @@
> +    {
> +        JS_GUARD_OBJECT_NOTIFIER_INIT;
> +    }
> +
> +    JS_DECL_USE_GUARD_OBJECT_NOTIFIER

Same.

@@ +433,5 @@
> +    {
> +        JS_GUARD_OBJECT_NOTIFIER_INIT;
> +    }
> +
> +    JS_DECL_USE_GUARD_OBJECT_NOTIFIER

Same.
Attachment #564998 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 565003 [details] [diff] [review]
main patch

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

So, honestly, I kind of feel like moving every helper method into friendapi, and not even trying to answer the question of whether whatever's being done would be better done another way, is a copout.

But I guess this is a step forward at least abstraction-wise.  And it looks like the same hackaround was used for jsobj.h already.

So I guess even if this is better (if only somewhat), I am decidely meh about it.  :-|

::: content/xul/document/src/nsXULContentSink.cpp
@@ +1060,5 @@
>                // By default scripts in XUL documents have E4X turned on. We use
>                // our implementation knowledge to reuse JSVERSION_HAS_XML as a
>                // safe version flag. This is still OK if version is
>                // JSVERSION_UNKNOWN (-1),
> +              version = JS_VersionSetXML(JSVersion(version), JS_TRUE);

This could be just |true| here, and for all the other callers.  Not important to change, just might be nicer to read.  And I suspect when PRBool->bool gets applied to JSBool, someday, this would happen eventually.  So there's an earlier-is-better contention here, I think.

::: js/src/jscntxt.h
@@ +263,5 @@
>                          js::DefaultHasher<void *>,
>                          js::SystemAllocPolicy> Map;
>  
> +    /* Opaque thread-id, from NSPR's PR_GetCurrentThread(). */
> +    void                *id;

This is more a comment for the entire file than for this line, but: any chance any of the members you've rearranged here, at all, could be made private at the same time you're doing all this work?  At least a followup or something would be nice.

@@ +352,5 @@
>  
>  }
>  
>  struct JSRuntime {
> +    /* Literal table maintained by jsatom.c functions. */

jsatom.cpp

::: js/src/jsfriendapi.cpp
@@ +222,5 @@
> +    return js_DateGetMsecSinceEpoch(cx, obj);
> +}
> +
> +JS_FRIEND_API(JSBool)
> +JS_WasLastGCCompartmental(JSContext *cx)

It seems pretty sad that we have/need this API right now.

::: js/src/jsfriendapi.h
@@ +131,5 @@
>  
>  #ifdef __cplusplus
>  
> +#include "jsatom.h"
> +#include "jsvector.h"

Including headers in the middle of a file like this seems worrisome.  Is there any way to avoid that, perhaps by moving this section to the top of the file or something?

::: js/src/xpconnect/shell/xpcshell.cpp
@@ +542,5 @@
>      JSRuntime *rt;
>      uint32 preBytes;
>  
> +    rt = JS_GetRuntime(cx);
> +    preBytes = JS_GetGCParameter(rt, JSGC_BYTES);

Unify the initializations with the declarations while you're here.

@@ +1002,5 @@
>  {
>      if ((errorNumber > 0) && (errorNumber < JSShellErr_Limit))
> +        return &jsShell_ErrorFormatString[errorNumber];
> +    else
> +        return NULL;

I'm guessing this was just copypasta'd from js.msg and uses, but isn't it just an error if an error is reported with a number outside the range its reporter is supposed to handle?  I think the latter half of this should be a JS_NOT_REACHED.

::: js/src/xpconnect/src/nsXPConnect.cpp
@@ +953,2 @@
>              cb.NoteScriptChild(nsIProgrammingLanguage::JAVASCRIPT,
> +                               js::GetContextGlobalObject(cx));

if (JSObject* global = js::Get...) to avoid repeating yourself?

::: js/src/xpconnect/src/xpcmaps.cpp
@@ +41,5 @@
>  /* Private maps (hashtables). */
>  
>  #include "xpcprivate.h"
>  #include "jsbit.h"
> +#include "jshash.h"

Why this?  Is this intentional?

::: js/src/xpconnect/src/xpcstring.cpp
@@ -90,5 @@
> -    JSAtom *atom;
> -    if (length == 0 && (atom = cx->runtime->atomState.emptyAtom))
> -    {
> -        return STRING_TO_JSVAL(atom);
> -    }

Mind-boggling.

::: toolkit/components/startup/nsAppStartup.cpp
@@ +706,5 @@
>  MaybeDefineProperty(JSContext *cx, JSObject *obj, const char *name, PRTime timestamp)
>  {
>    if (!timestamp)
>      return;
> +  JSObject *date = JS_NewDateObjectMsec(cx, timestamp/PR_USEC_PER_MSEC);

Spaces around the / here.
Attachment #565003 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 565003 [details] [diff] [review]
main patch

>--- a/dom/base/nsJSEnvironment.cpp
>+++ b/dom/base/nsJSEnvironment.cpp
>-    switch(cx->debugHooks->debuggerHandler(cx, script, ::JS_GetFramePC(cx, fp),
>+    JSDebugHooks *hooks = js::GetContextDebugHooks(cx);
>+    switch(hooks->debuggerHandler(cx, script, ::JS_GetFramePC(cx, fp),
>                                            &rval,
>-                                           cx->debugHooks->
>-                                           debuggerHandlerData)) {
>+                                           hooks->debuggerHandlerData)) {

Reindent, please.

>--- a/dom/workers/ListenerManager.cpp
>+++ b/dom/workers/ListenerManager.cpp
> #include "jsapi.h"
>-#include "jscntxt.h"
>+#include "jsfriendapi.h"
> #include "jsvector.h"

Can this include go now?
backed out for causing permanent failures in Windows PGO mochitests (1,4)

See philor's topic in tree-management
Here is some log:
https://tbpl.mozilla.org/php/getParsedLog.php?id=6981332&tree=Mozilla-Inbound
33463 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_bug664916.html | [SimpleTest/SimpleTest.js, window.onerror] - An error occurred: tagNameGetter.call is not a function at http://mochi.test:8888/tests/content/base/test/test_bug664916.html:34

https://tbpl.mozilla.org/php/getParsedLog.php?id=6981247&tree=Mozilla-Inbound
293 ERROR TEST-UNEXPECTED-FAIL | /tests/js/xpconnect/tests/mochitest/test_bug462428.html | [SimpleTest/SimpleTest.js, window.onerror] - An error occurred: getter.call is not a function at http://mochi.test:8888/tests/js/xpconnect/tests/mochitest/test_bug462428.html:34
341 ERROR TEST-UNEXPECTED-FAIL | /tests/js/xpconnect/tests/mochitest/test_bug585745.html | [SimpleTest/SimpleTest.js, window.onerror] - An error occurred: property descriptor's getter field is neither undefined nor a function at http://mochi.test:8888/tests/js/xpconnect/tests/mochitest/test_bug585745.html:29
373 ERROR TEST-UNEXPECTED-FAIL | /tests/js/xpconnect/tests/mochitest/test_bug628794.html | [SimpleTest/SimpleTest.js, window.onerror] - An error occurred: property descriptor's getter field is neither undefined nor a function at http://mochi.test:8888/tests/js/xpconnect/tests/mochitest/test_bug628794.html:29

Unfortunately there is no way to get pgo testing on Try atm, please contact releng to figure out a strategy for that.
Target Milestone: mozilla10 → ---
So far it looks like the autorooters portion of the patch is what's causing the orange. Unfortunately that's the part that's harder to bisect over. I guess I'll try converting only JSAutoObjectRooter and leave the other rooters in jscntxt.h.
Any news?
Blocks: 554088
I haven't had time for this lately. I did find that the part of the patch that causes orange in PGO. It's the change where I took the tag field out of AutoGCRooter and made the trace method be virtual. I don't know why that would cause problems, but it is. If someone wants to modify the patch to remove that part, I think it could land.

Ms2ger, to get useful results from tryserver, you need to change the mozconfigs to enable PGO builds. Normally we don't do PGO builds on tryserver (AFAIK), and the patch was only failing with PGO. See this page:
  https://wiki.mozilla.org/ReleaseEngineering/TryServer#Using_a_custom_mozconfig
By any chance does Ms2ger wish to drive this ship home?
Assignee: wmccloskey → Ms2ger
Depends on: 713340
Depends on: 713645
Depends on: 714264
Depends on: 714458
Used by XPCJSRuntime.cpp.
Attachment #564998 - Attachment is obsolete: true
Attachment #565003 - Attachment is obsolete: true
Attachment #586672 - Flags: review?(igor)
Attachment #586674 - Flags: review?(jwalden+bmo)
Attachment #586679 - Flags: review? → review?(igor)
Attachment #586697 - Flags: review? → review?(evilpies)
Attachment #586700 - Attachment description: Part y: Move JS_CHECK_RECURSION to jsfriendapi.h. → Part z: Move JS_CHECK_RECURSION to jsfriendapi.h.
Attachment #586701 - Flags: review?(evilpies)
Comment on attachment 586672 [details] [diff] [review]
Part a: Move AutoLockGC to jsfriendapi.h.

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

r+ with the comment below addressed.

::: js/src/jsfriendapi.h
@@ +460,5 @@
> +{
> +  public:
> +    explicit AutoLockGC(JSRuntime *rt = NULL
> +                        MOZILLA_GUARD_OBJECT_NOTIFIER_PARAM);
> +    ~AutoLockGC();

Add  couple of private static functions LockGC(JSRutime *rt), UnlockGC(JSRuntime *rt) functions and use them to implement the constructors/destructors. This way the compiler can optimize the case of no locking and can also see that a reference to AutoLockGC::runtime is not leaked so it can remove a stack slot for it.
Attachment #586672 - Flags: review?(igor) → review+
Comment on attachment 586679 [details] [diff] [review]
Part h: Expose debuggerHandler in jsfriendapi.h.

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

::: js/src/jsfriendapi.cpp
@@ +515,5 @@
> +JS_FRIEND_API(JSTrapStatus)
> +CallContextDebugHandler(JSContext *cx, JSScript *script, jsbytecode *bc, Value *rval)
> +{
> +    MOZ_ASSERT(CanCallContextDebugHandler(cx));
> +    return cx->debugHooks->debuggerHandler(cx, script, bc, rval,

We should fix the preexisting bug here - after the haddler was checked for existence it could have been removed. So do not assert that the handler is not null, rather return JSTRAP_RETURN in this case.
Attachment #586679 - Flags: review?(igor) → review+
Comment on attachment 586686 [details] [diff] [review]
Part n: Expose JSOPTION_UNROOTED_GLOBAL in jsfriendapi.h.

I am not a big fan of friend APIs, we should probably turn this stuff into public apis, but other related stuff around this api is friend to, so not a problem for this patch.
Attachment #586686 - Flags: review?(gal) → review+
Comment on attachment 586688 [details] [diff] [review]
Part o: Expose setActivityCallback in jsfriendapi.h.

What is that #if 0 garbage there?
Attachment #586688 - Flags: review?(gal) → review+
Comment on attachment 586690 [details] [diff] [review]
Part q: Expose gcLock in jsfriendapi.h.

Not the cleanest abstraction, but I don't have any better suggestion either. Well worth the end goal of getting rid of the header include and we can cleanup the interface from there.
Attachment #586690 - Flags: review?(gal) → review+
(In reply to Andreas Gal :gal from comment #57)
> Comment on attachment 586688 [details] [diff] [review]
> Part o: Expose setActivityCallback in jsfriendapi.h.
> 
> What is that #if 0 garbage there?

My todo list; it's gone entirely by the end of this patch queue. I could get rid of it, but I'd kind of rather not spend my time on the merging that will result.

(In reply to Andreas Gal :gal from comment #58)
> Comment on attachment 586690 [details] [diff] [review]
> Part q: Expose gcLock in jsfriendapi.h.
> 
> Not the cleanest abstraction, but I don't have any better suggestion either.
> Well worth the end goal of getting rid of the header include and we can
> cleanup the interface from there.

Yeah. Maybe a function of rt that returns JS_NEW_CONDVAR(rt->gcLock) would be somewhat better... I dunno.
Attachment #586702 - Flags: review?(n.nethercote) → review+
Attachment #586683 - Flags: review?(christopher.leary) → review+
Attachment #586680 - Flags: review?(mrbkap) → review+
Comment on attachment 586676 [details] [diff] [review]
Part e: Move js_GetSCOffset to jsfriendapi.h.

>-#include "jsclone.h"
>+#include "jscntxt.h" // structuredCloneCallbacks

I'm probably missing the grand plan, but, don't you mean 'jsfriendapi.h' ?
Attachment #586676 - Flags: review?(luke) → review+
Attachment #586691 - Flags: review?(luke) → review+
(In reply to Luke Wagner [:luke] from comment #60)
> Comment on attachment 586676 [details] [diff] [review]
> Part e: Move js_GetSCOffset to jsfriendapi.h.
> 
> >-#include "jsclone.h"
> >+#include "jscntxt.h" // structuredCloneCallbacks
> 
> I'm probably missing the grand plan, but, don't you mean 'jsfriendapi.h' ?

It needs both jsfriendapi and jscntxt at this point. I added jsfriendapi.h in attachment 586677 [details] [diff] [review], but it makes more sense to add it here, indeed.
Attachment #586695 - Flags: review?(bhackett1024) → review+
Attachment #586674 - Flags: review?(jwalden+bmo) → review+
Attachment #586697 - Flags: review?(evilpies) → review+
Attachment #586698 - Flags: review?(evilpies) → review+
Attachment #586701 - Flags: review?(evilpies) → review+
Attachment #586692 - Flags: review?(wmccloskey) → review+
Attachment #586675 - Flags: review?(jorendorff) → review+
Attachment #586685 - Flags: review?(jorendorff) → review+
Comment on attachment 586696 [details] [diff] [review]
Part v: Expose rt->compartments in jsfriendapi.h.

In jsfriendapi.h and also in the .cpp file:
>+JS_FRIEND_API(CompartmentVector&)
>+GetRuntimeCompartments(JSRuntime *rt)

const CompartmentVector &, please.

>-        js::CompartmentVector &vector = rt->compartments;
>+        js::CompartmentVector &vector = js::GetRuntimeCompartments(rt);
>         for (JSCompartment **p = vector.begin(); p != vector.end(); ++p) {

Add const at the beginning of the line you changed, please.

It would also be best to change the for-loop to insert the `const` keyword between the two stars, like so:

          for (JSCompartment * const *p = vector.begin() ......

and I encourage you to do it even though (due to a bug in js::Vector) C++ might let you get away without it.
Attachment #586696 - Flags: review?(jorendorff) → review+
Comment on attachment 586677 [details] [diff] [review]
Part f: Expose structuredCloneCallbacks in jsfriendapi.h.

Stealing review.
Attachment #586677 - Flags: review?(dvander) → review+
Attachment #586693 - Flags: review?(dvander) → review+
Comment on attachment 586681 [details] [diff] [review]
Part j: Stop using js::ContextAllocPolicy in ListenerManager.cpp.

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

I tried to avoid XPCOM-ish stuff in these js bindings in case we decided to move them into the js engine or autogenerate them somehow. I'd really prefer not to add nsTArray here, is it just not possible to use Vector outside of the engine?

Also, this patch adds jsfriendapi.h but as far as I can tell never uses anything in it.
Attachment #586681 - Flags: review?(bent.mozilla) → review-
Attachment #586703 - Flags: review?(dmandelin) → review+
We can't use js::ContextAllocPolicy outside the engine.
Attachment #586678 - Flags: review?(brendan) → review?(bhackett1024)
Comment on attachment 586678 [details] [diff] [review]
Part g: Expose VersionSetXML in jsfriendapi.h.

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

::: js/src/jsfriendapi.cpp
@@ +501,5 @@
>  
> +JS_FRIEND_API(JSVersion)
> +VersionSetXML(JSVersion version, bool enable)
> +{
> +    VersionSetXML(&version, enable);

Can you remove the VersionSetXML from jscntxt.h and put its implementation here, then change the single caller in jscntxt.h to use this function?
Attachment #586678 - Flags: review?(bhackett1024) → review+
Attachment #586700 - Flags: review?(brendan) → review?(mrosenberg)
Attachment #586700 - Flags: review?(mrosenberg) → review+
Like this more, perhaps?
Attachment #586681 - Attachment is obsolete: true
Attachment #587742 - Flags: review?(bent.mozilla)
Attachment #586673 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 586684 [details] [diff] [review]
Part l: Use the existing JSAPI for the global object for a context.


>         JSContext *cx = static_cast<JSContext*>(n);
>-        NS_ASSERTION(cx->globalObject, "global object NULL before unlinking");
>-        cx->globalObject = nsnull;
>+        JSAutoRequest ar(cx);
>+        NS_ASSERTION(JS_GetGlobalObject(cx), "global object NULL before unlinking");
>+        JS_SetGlobalObject(cx, NULL);

Uh oh. Does this mean we were racy before?
Attachment #586684 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 586689 [details] [diff] [review]
Part p: Remove AutoLockJSGC in favour of js::AutoLockGC.

Huzzah!
Attachment #586689 - Flags: review?(bobbyholley+bmo) → review+
Attachment #587776 - Flags: review?(sphink)
Attachment #587776 - Flags: review?(bent.mozilla)
Attachment #587742 - Attachment is obsolete: true
Attachment #587742 - Flags: review?(bent.mozilla)
Comment on attachment 587776 [details] [diff] [review]
Stop using js::ContextAllocPolicy in ListenerManager.cpp.

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

Thanks!
Attachment #587776 - Flags: review?(bent.mozilla) → review+
Comment on attachment 587776 [details] [diff] [review]
Stop using js::ContextAllocPolicy in ListenerManager.cpp.

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

I'm ok with this as is (modulo the constructor name fix), though the naming is clunky.

::: js/src/jsalloc.h
@@ +115,5 @@
>  
>      JS_FRIEND_API(void) reportAllocOverflow() const;
>  };
>  
> +class ExternallyUsableContextAllocPolicy

I don't understand the layering anymore. Could this live in js/public instead of here?

Would JSContextAllocPolicy be too confusing as a name?

@@ +120,5 @@
> +{
> +    JSContext *const cx;
> +
> +  public:
> +    ContextAllocPolicy(JSContext *cx) : cx(cx) {}

Cut & paste error -- wrong class name.
Attachment #587776 - Flags: review?(sphink) → review+
https://hg.mozilla.org/mozilla-central/rev/b2ef431401ff
https://hg.mozilla.org/mozilla-central/rev/1668811de954
https://hg.mozilla.org/mozilla-central/rev/4f665744b6e6
https://hg.mozilla.org/mozilla-central/rev/ebd92ba20fd1
https://hg.mozilla.org/mozilla-central/rev/10519108297f
https://hg.mozilla.org/mozilla-central/rev/6ffce4fb752b
https://hg.mozilla.org/mozilla-central/rev/58eef6ad44ad
https://hg.mozilla.org/mozilla-central/rev/2b2c3d92ae9f
https://hg.mozilla.org/mozilla-central/rev/f9fe45249420
https://hg.mozilla.org/mozilla-central/rev/be5a53604bc3
https://hg.mozilla.org/mozilla-central/rev/46b5d4c3b3d8
https://hg.mozilla.org/mozilla-central/rev/29a2d146c5ff
https://hg.mozilla.org/mozilla-central/rev/41ea9acca641
https://hg.mozilla.org/mozilla-central/rev/fe8240db5834
https://hg.mozilla.org/mozilla-central/rev/33d5a1f2af51
https://hg.mozilla.org/mozilla-central/rev/e798120914f7
https://hg.mozilla.org/mozilla-central/rev/35247789f332
https://hg.mozilla.org/mozilla-central/rev/fee37d705c87
https://hg.mozilla.org/mozilla-central/rev/346328bd1f2e
https://hg.mozilla.org/mozilla-central/rev/c8ba49dafe5e
https://hg.mozilla.org/mozilla-central/rev/d848942fc440
https://hg.mozilla.org/mozilla-central/rev/b2bffccbc28b
https://hg.mozilla.org/mozilla-central/rev/aa34f07b7f67
https://hg.mozilla.org/mozilla-central/rev/4b70a75df702
https://hg.mozilla.org/mozilla-central/rev/9ee7f6571842
https://hg.mozilla.org/mozilla-central/rev/cf1294a14a4c
https://hg.mozilla.org/mozilla-central/rev/9f706102086f
https://hg.mozilla.org/mozilla-central/rev/dfc618d2f28f

Thanks all!
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Depends on: 718300
You need to log in before you can comment on or make changes to this bug.