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
:
Mentors:
Depends on:
Blocks: 677079
  Show dependency treegraph
 
Reported: 2011-12-31 08:59 PST by :Ms2ger
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
jwalden+bmo: review-
Details | Diff | Review
Part b: Provide the thread-related APIs xpcprivate.h needs. (5.95 KB, patch)
2012-01-02 10:22 PST, :Ms2ger
igor: review+
Details | Diff | Review
Part c: Don't include jscntxt.h in xpcprivate.h (10.35 KB, patch)
2012-01-02 10:24 PST, :Ms2ger
bobbyholley: review+
Details | Diff | Review
Part a v2 (4.15 KB, patch)
2012-01-06 15:14 PST, :Ms2ger
jwalden+bmo: review+
Details | Diff | Review

Description :Ms2ger 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 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 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 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 (PTO through June 13) 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 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.