Closed
Bug 817012
Opened 12 years ago
Closed 12 years ago
Use a richer interface to talk from about:telemetry to TelemetryPing.js
Categories
(Toolkit :: Telemetry, defect)
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: espindola, Assigned: espindola)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
vladan
:
review+
|
Details | Diff | 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)
Assignee | ||
Comment 1•12 years ago
|
||
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)
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #687207 -
Attachment is obsolete: true
Attachment #687207 -
Flags: review?(vdjeric)
Attachment #687255 -
Flags: review?(vdjeric)
Comment 3•12 years ago
|
||
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+
Assignee | ||
Comment 4•12 years ago
|
||
Comment 5•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 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.
Description
•