Closed Bug 714458 Opened 9 years ago Closed 9 years ago

Don't include jscntxt.h in xpcprivate.h


(Core :: XPConnect, defect)

Not set





(Reporter: Ms2ger, Assigned: Ms2ger)




(3 files, 1 obsolete file)

I'm doing this up front because xpcprivate.h doesn't actually need much from jscntxt.h, but most of xpconnect is including it this way, which makes it harder to measure progress. Patches coming up.
Attachment #585301 - Flags: review? → review?(igor)
Attachment #585302 - Flags: review?(bobbyholley+bmo)
Comment on attachment 585302 [details] [diff] [review]
Part c: Don't include jscntxt.h in xpcprivate.h

>diff --git a/js/xpconnect/src/XPCJSRuntime.cpp b/js/xpconnect/src/XPCJSRuntime.cpp

>+#include "jscntxt.h"
>+#if 0
>+        JS_ASSERT(acx->hasRunOption(JSOPTION_UNROOTED_GLOBAL));
>+        if (acx->globalObject)
>+            JS_CALL_OBJECT_TRACER(trc, acx->globalObject, "XPC global object");
>+            while ((acx = JS_ContextIterator(cx->runtime, &iter))) {
>+                if (!acx->hasRunOption(JSOPTION_UNROOTED_GLOBAL))
>+                    JS_ToggleOptions(acx, JSOPTION_UNROOTED_GLOBAL);
>+js_NextActiveContext, js::TriggerOperationCallback
>+c == cx->runtime->atomsCompartment
>+        mWatchdogWakeup = JS_NEW_CONDVAR(mJSRuntime->gcLock);
>+        mJSRuntime->setActivityCallback(ActivityCallback, this);

This should go away.

>+#include "jscntxt.h" // js::ThreadData, JS_TRACER_INIT, context->stackLimit, cx->outstandingRequests,
>+// cx->globalObject, sizeof(JSContext), js::CompartmentVector, cx->stack.empty()

I like these comments when we're just using one or two things from the header, but I don't think this is one of those cases. I'd prefer to just eliminate the comment.

r+ with those two fixes.
Attachment #585302 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 585301 [details] [diff] [review]
Part b: Provide the thread-related APIs xpcprivate.h needs.

Review of attachment 585301 [details] [diff] [review]:

::: js/src/jsapi.cpp
@@ +6330,5 @@
>  JS_PUBLIC_API(jsword)
>  JS_GetContextThread(JSContext *cx)
>  {
> +    return cx->thread() ? reinterpret_cast<jsword>(cx->thread()->id) : 0;

Remove also JS_THREAD_ID definition as you removed the single user of the macro.
Attachment #585301 - Flags: review?(igor) → review+
Comment on attachment 585300 [details] [diff] [review]
Part a: Introduce JSAPI for JSContext's second private pointer.

Review of attachment 585300 [details] [diff] [review]:

You'd think this would be a slam-dunk, but...

We've used a "2" prefix in the past to indicate the second edition of an API -- JS_DeleteProperty2, for example.  Using that naming pattern here would be a misnomer.

JS_GetSecondContextPrivate?  Ugh.  Past that, I got nothin'.  See what the IRC peanut gallery says when they show up.
Attachment #585300 - Flags: review?(jwalden+bmo) → review-
Attached patch Part a v2Splinter Review
It's still JS_GetSecondContextPrivate you're going to get; the gallery doesn't seem to be coming up with anything better.
Attachment #585300 - Attachment is obsolete: true
Attachment #586583 - Flags: review?(jwalden+bmo)
Comment on attachment 586583 [details] [diff] [review]
Part a v2

Review of attachment 586583 [details] [diff] [review]:

So it goes.
Attachment #586583 - Flags: review?(jwalden+bmo) → review+
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.