Don't include jscntxt.h in xpcprivate.h

RESOLVED FIXED in mozilla12

Status

()

Core
XPConnect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Tracking

Trunk
mozilla12
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 585300 [details] [diff] [review]
Part a: Introduce JSAPI for JSContext's second private pointer.
Attachment #585300 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 2

6 years ago
Created attachment 585301 [details] [diff] [review]
Part b: Provide the thread-related APIs xpcprivate.h needs.
Attachment #585301 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #585301 - Flags: review? → review?(igor)
(Assignee)

Comment 3

6 years ago
Created attachment 585302 [details] [diff] [review]
Part c: Don't include jscntxt.h in xpcprivate.h
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_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.
Attachment #585302 - Flags: review?(bobbyholley+bmo) → review+

Comment 5

6 years ago
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.
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-
(Assignee)

Comment 7

6 years ago
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.
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+
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/mozilla-central/rev/996cc657dfba
https://hg.mozilla.org/mozilla-central/rev/0c55d7a26512
https://hg.mozilla.org/mozilla-central/rev/6324ddbe2668
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.