Closed Bug 1354036 Opened 8 years ago Closed 8 years ago

Remove debug.js usage from TelemetrySession.jsm

Categories

(Toolkit :: Telemetry, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: gfritzsche, Assigned: pauline.lallinec, Mentored)

References

Details

(Whiteboard: [measurement:client] [lang=js])

Attachments

(1 file, 2 obsolete files)

The use of NS_ASSERT() from debug.js in TelemetrySession.jsm looks rather historic. We can safely: - remove the debug.js import - change the NS_ASSERT() uses into Cu.reportError() calls
Pauline, are you interested in working on this bug? I think we should just change `NS_ASSERT()` uses [1] into `this._log.error()` calls, not `Cu.reportError()`: That matches the rest of the module and makes it follow Telemetry log configurations. After making changes we should: - `mach build` - see that tests pass: `mach test toolkit/components/telemetry/tests/unit` - check that linting passes: `mach eslint` 1: https://dxr.mozilla.org/mozilla-central/search?q=NS_ASSERT(+path%3Atoolkit%2Fcomponents%2Ftelemetry%2F&redirect=false
Assignee: nobody → pauline.lallinec
Hi Georg, Here is my patch. ESLint reported no issues and all tests passed. I noticed you mention there was only 1 use of `NS_ASSERT()`, however I found 2, and I replaced both of these uses. I also removed the debug.js import instructed in the first comment. Let me know if I need to change anything. Best, Pauline
Attachment #8856130 - Flags: review?(gfritzsche)
Comment on attachment 8856130 [details] [diff] [review] removed debug.js import; changed `NS_ASSERT()` calls (2) into `this._log.error()` calls Review of attachment 8856130 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this looks good already! One smaller thing: Lets change the commit message to match our standard format [1]: "Bug <NNN> - What changed. r=<reviewer>". I think the commit message can also be more higher-level, goal-oriented. E.g. "Removed use of NS_ASSERT() from Telemetry." For smaller details we can view the diff of the changes. I also commented on a small style improvement below. 1: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions ::: toolkit/components/telemetry/TelemetrySession.jsm @@ +1128,5 @@ > // isn't present, that indicates the distinguished amounts have changed > // and this file hasn't been updated appropriately. > let amount = mgr[amountName]; > + if (amount === undefined) { > + this._log.error("telemetry accessed an unknown distinguished amount"); While not strictly needed here, lets change the logs to the style of the other `this._log` statements: "<function name> - Some message."
Attachment #8856130 - Flags: review?(gfritzsche) → feedback+
Hi Georg, Here is the new patch. I have update the log messages to so that they mention the function in which they occur. I have also updated my commit message. Please let me know if anything else is amiss :-) Best, Pauline
Attachment #8856130 - Attachment is obsolete: true
Comment on attachment 8856646 [details] [diff] [review] Removed use of NS_ASSERT() from TelemetrySession.jsm Side-note: You can set review or feedback requests (flags) in bugzilla for specific people. When you "flag" me for review, i get special mail about it and it is listed with other flags in my TODOs.
Attachment #8856646 - Flags: review?(gfritzsche)
Comment on attachment 8856646 [details] [diff] [review] Removed use of NS_ASSERT() from TelemetrySession.jsm Review of attachment 8856646 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, thanks!
Attachment #8856646 - Flags: review?(gfritzsche) → review+
Comment on attachment 8856646 [details] [diff] [review] Removed use of NS_ASSERT() from TelemetrySession.jsm Review of attachment 8856646 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/TelemetrySession.jsm @@ +1129,5 @@ > // and this file hasn't been updated appropriately. > let amount = mgr[amountName]; > + if (amount === undefined) { > + this._log.error("gatherMemory - telemetry accessed an unknown distinguished amount"); > + } Actually, i overlooked one thing myself here: This logging is in a function that is not bound to the instance the surrounding method belongs to. Logging an error here would throw an exception, because the `this` is a different one than the `this` in the surrounding function. The easy fix here is to make `h()` an arrow function [1] that automatically binds the surrounding `this`: > let h = (id, units, amountName) => { > ... > }; I was able to test that relatively easily by: - temporary make this always log, not just on errors - ./mach build - ./mach run - open tools -> web developer -> browser console [2] - there, enter: - ts = Cu.import("resource://gre/modules/TelemetrySession.jsm" - ts.Impl.gatherMemory() - check that the error log shows up 1: https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Functions/Arrow_functions 2: https://developer.mozilla.org/en/docs/Tools/Browser_Console
Attachment #8856646 - Flags: review+ → feedback+
Hi, patch updated, hopefully it's all good :-)
Attachment #8856646 - Attachment is obsolete: true
Comment on attachment 8857061 [details] [diff] [review] Removed use of NS_ASSERT() from TelemetrySession.jsm Review of attachment 8857061 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this looks good now!
Attachment #8857061 - Flags: review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/003d697d6686 Removed use of NS_ASSERT() from TelemetrySession.jsm. r=gfritzsche
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: