Closed Bug 963312 Opened 6 years ago Closed 6 years ago

Implement GetJSEngineTelemetryValue on Components.utils, not nsXPConnect

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

Attachments

(1 file)

Bug 670059 made nsXPConnect implement a new IDL interface, nsIJSEngineTelemetryStats.  All this does, nsXPConnect::GetTelemetryValue, is call some JS API functions without accessing any fields of nsXPConnect, so there surely must be some better way to implement this.

This requires updating
  toolkit/components/telemetry/TelemetryPing.jsm 
and also smaug found some addon r2d2b2g that would need to be updated in some trivial way.

In addition to general cleanliness, this appears to be the only way any addons or browser code access XPConnect from JS, so maybe with this out of the way we could de-XPIDL XPConnect.
Godspeed. Can we just put GetTelemetryValue on Components.utils?
Also, note that r2d2b2g is the b2g simulator, which is kind of a big deal.
(In reply to Bobby Holley (:bholley) from comment #2)
> Also, note that r2d2b2g is the b2g simulator, which is kind of a big deal.

Ah, yes.  Perhaps the way to do this is to add a new nicer interface, then at some later point deprecate the old one once we're sure the b2g simulator is updated.  It should be a simple fix, but we want to make sure it happens.
Depends on: 963323
Yeah. Given that the simulator is actively maintained, I'm also fine with dropping the interface at the same time we create the new one, and telling them to add a conditional somewhere.
From talking to jst, it sounds like r2d2b2g just contains a copy of Gecko, so fixing the telemetry.jsm is probably enough.  We'll want to double check, of course.
Assignee: nobody → continuation
With bonus marking nsIXPConnect noscript.  We'll see how that goes.
  https://tbpl.mozilla.org/?tree=Try&rev=d8ae5d0bddfd
Blocks: 963665
The body of the get telemetry value should be unchanged.

r?Taras for TelemetryPing.jsm, bholley for the rest.
Attachment #8365563 - Flags: review?(taras.mozilla)
Attachment #8365563 - Flags: review?(bobbyholley+bmo)
Comment on attachment 8365563 [details] [diff] [review]
Get JS engine telemetry values from Components.utils, not nsIXPConnect.

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

::: js/xpconnect/src/XPCComponents.cpp
@@ +3444,5 @@
>  
> +NS_IMETHODIMP
> +nsXPCComponents_Utils::GetJSEngineTelemetryValue(JSContext *cx, MutableHandleValue rval)
> +{
> +    RootedObject obj(cx, JS_NewObject(cx, nullptr, JS::NullPtr(), JS::NullPtr()));

nit - no JS:: prefixing in XPConnect cpp files. |using namespace JS|.

@@ +3452,5 @@
> +    unsigned attrs = JSPROP_ENUMERATE | JSPROP_READONLY | JSPROP_PERMANENT;
> +
> +    size_t i = JS_SetProtoCalled(cx);
> +    RootedValue v(cx, DoubleValue(i));
> +    if (!JS_DefineProperty(cx, obj, "setProto", v, nullptr, nullptr, attrs))

This should really pass JS_PropertyStub and JS_StrictPropertyStub instead of the nullptrs, since nullptr means 'use the PropertyOp from the JSClass', which is always scary (though for ObjectClass it's fine). You don't have to change it though.
Attachment #8365563 - Flags: review?(bobbyholley+bmo) → review+
Attachment #8365563 - Flags: review?(taras.mozilla) → review+
Thanks for the fast reviews!

https://hg.mozilla.org/integration/mozilla-inbound/rev/b182f29bb7b1

I kept the JS:: for NullPtr, because the compiler complained about ambiguity with some NullPtr() thing in the JS rooting API when I removed it.

a full try run was green: https://tbpl.mozilla.org/?tree=Try&rev=c6c74c797a99
No longer depends on: 963323
Duplicate of this bug: 963323
Summary: Remove nsIJSEngineTelemetryStats from nsXPConnect → Implement GetJSEngineTelemetryValue on Components.utils, not nsXPConnect
https://hg.mozilla.org/mozilla-central/rev/b182f29bb7b1
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.