Last Comment Bug 714458 - Don't include jscntxt.h in xpcprivate.h
: Don't include jscntxt.h in xpcprivate.h
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: :Ms2ger (⌚ UTC+1/+2)
:
:
Mentors:
Depends on:
Blocks: 677079
  Show dependency treegraph
 
Reported: 2011-12-31 08:59 PST by :Ms2ger (⌚ UTC+1/+2)
Modified: 2012-01-11 05:00 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part a: Introduce JSAPI for JSContext's second private pointer. (4.10 KB, patch)
2012-01-02 10:22 PST, :Ms2ger (⌚ UTC+1/+2)
jwalden+bmo: review-
Details | Diff | Splinter Review
Part b: Provide the thread-related APIs xpcprivate.h needs. (5.95 KB, patch)
2012-01-02 10:22 PST, :Ms2ger (⌚ UTC+1/+2)
igor: review+
Details | Diff | Splinter Review
Part c: Don't include jscntxt.h in xpcprivate.h (10.35 KB, patch)
2012-01-02 10:24 PST, :Ms2ger (⌚ UTC+1/+2)
bobbyholley: review+
Details | Diff | Splinter Review
Part a v2 (4.15 KB, patch)
2012-01-06 15:14 PST, :Ms2ger (⌚ UTC+1/+2)
jwalden+bmo: review+
Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2011-12-31 08:59:09 PST
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.
Comment 1 :Ms2ger (⌚ UTC+1/+2) 2012-01-02 10:22:03 PST
Created attachment 585300 [details] [diff] [review]
Part a: Introduce JSAPI for JSContext's second private pointer.
Comment 2 :Ms2ger (⌚ UTC+1/+2) 2012-01-02 10:22:34 PST
Created attachment 585301 [details] [diff] [review]
Part b: Provide the thread-related APIs xpcprivate.h needs.
Comment 3 :Ms2ger (⌚ UTC+1/+2) 2012-01-02 10:24:17 PST
Created attachment 585302 [details] [diff] [review]
Part c: Don't include jscntxt.h in xpcprivate.h
Comment 4 Bobby Holley (:bholley) (busy with Stylo) 2012-01-02 11:15:33 PST
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_LOCK_GC, JS_UNLOCK_GC
>+
>+js_NextActiveContext, js::TriggerOperationCallback
>+JSString::charsHeapSize
>+c == cx->runtime->atomsCompartment
>+CollectCompartmentStatsForRuntime
>+        mWatchdogWakeup = JS_NEW_CONDVAR(mJSRuntime->gcLock);
>+        mJSRuntime->setActivityCallback(ActivityCallback, this);
>+#endif
>+

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.
Comment 5 Igor Bukanov 2012-01-02 12:10:25 PST
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)
>  {
>  #ifdef JS_THREADSAFE
> +    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.
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-03 06:16:51 PST
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.
Comment 7 :Ms2ger (⌚ UTC+1/+2) 2012-01-06 15:14:20 PST
Created attachment 586583 [details] [diff] [review]
Part a v2

It's still JS_GetSecondContextPrivate you're going to get; the gallery doesn't seem to be coming up with anything better.
Comment 8 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-06 21:15:37 PST
Comment on attachment 586583 [details] [diff] [review]
Part a v2

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

So it goes.

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