Closed
Bug 1354036
Opened 8 years ago
Closed 8 years ago
Remove debug.js usage from TelemetrySession.jsm
Categories
(Toolkit :: Telemetry, enhancement, P3)
Toolkit
Telemetry
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)
2.07 KB,
patch
|
gfritzsche
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•8 years ago
|
||
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
Reporter | ||
Updated•8 years ago
|
Attachment #8856130 -
Flags: review?(gfritzsche)
Reporter | ||
Comment 3•8 years ago
|
||
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
Reporter | ||
Comment 5•8 years ago
|
||
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)
Reporter | ||
Comment 6•8 years ago
|
||
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+
Reporter | ||
Comment 7•8 years ago
|
||
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+
Attachment #8856646 -
Attachment is obsolete: true
Reporter | ||
Comment 9•8 years ago
|
||
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+
Reporter | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 10•8 years ago
|
||
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
Comment 11•8 years ago
|
||
bugherder |
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.
Description
•