Closed Bug 817012 Opened 8 years ago Closed 8 years ago

Use a richer interface to talk from about:telemetry to TelemetryPing.js

Categories

(Toolkit :: Telemetry, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: espindola, Assigned: espindola)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch patch (obsolete) — Splinter Review
This has two advantages:

* We don't use JSON to talk between two components in the same process!
* Having our own interface makes it a lot easier to make it async in the next patch.

https://tbpl.mozilla.org/?tree=Try&rev=edeeed311a96
Attachment #687112 - Flags: review?(vdjeric)
Attached patch patch (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=32a2754db96a

I missed some s/nsIObserver/nsITelemetryPing in the previous patch.
Attachment #687112 - Attachment is obsolete: true
Attachment #687112 - Flags: review?(vdjeric)
Attachment #687207 - Flags: review?(vdjeric)
Comment on attachment 687255 [details] [diff] [review]
Previous patch was missing a 'this' :-(

>diff --git a/toolkit/components/telemetry/TelemetryPing.js b/toolkit/components/telemetry/TelemetryPing.js
>-  getCurrentSessionPayloadAndSlug: function getCurrentSessionPayloadAndSlug(reason) {
>+  getCurrentSessionPayload: function getCurrentSessionPayloadAndSlug(reason) {

The name on the right-hand side should match the name on the left-hand side

>+  getPayload: function getPayload() {
>+    // This handler returns the current Telemetry payload to the caller.
>+    // We only gather startup info once.
>+    if (Object.keys(this._slowSQLStartup).length == 0)
>+      this.gatherStartupInformation();
>+    this.gatherMemory();
>+    return this.getCurrentSessionPayload("gather-payload");
>+  },

Update the comment, it's not an event handler anymore :)

>   classID: Components.ID("{55d6a5fa-130e-4ee6-a158-0133af3b86ba}"),
>-  QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver]),
>+  QueryInterface: XPCOMUtils.generateQI([Ci.nsITelemetryPing]),
> };

See next comment

>diff --git a/toolkit/components/telemetry/nsITelemetryPing.idl b/toolkit/components/telemetry/nsITelemetryPing.idl
>+/* -*- Mode: C++; c-basic-offset: 2; indent-tabs-mode: nil; tab-width: 8 -*- */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#include "nsIObserver.idl"
>+
>+[scriptable, uuid(077ee790-3a9d-11e2-81c1-0800200c9a66)]
>+interface nsITelemetryPing : nsIObserver {
>+  jsval getPayload();
>+};

I would prefer if the nsITelemetryPing interface was built on top of the base nsISupports interface instead. TelemetryPing would then multiply inherit from nsIObserver and nsITelemetryPing. You would just need to pass [Ci.nsIObserver, Ci.nsITelemetryPing] to generateQI above.
Attachment #687255 - Flags: review?(vdjeric) → review+
https://hg.mozilla.org/mozilla-central/rev/eeca66c44a67
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.