Remove debug.js usage from TelemetrySession.jsm

RESOLVED FIXED in Firefox 55

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

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

Tracking

(Blocks 1 bug)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

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

2 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
Assignee

Comment 2

2 years ago
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

2 years ago
Attachment #8856130 - Flags: review?(gfritzsche)
Reporter

Comment 3

2 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+
Assignee

Comment 4

2 years ago
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
Assignee

Updated

2 years ago
Attachment #8856130 - Attachment is obsolete: true
Reporter

Comment 5

2 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

2 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

2 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+
Assignee

Comment 8

2 years ago
Hi, patch updated, hopefully it's all good :-)
Assignee

Updated

2 years ago
Attachment #8856646 - Attachment is obsolete: true
Reporter

Comment 9

2 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

2 years ago
Keywords: checkin-needed

Comment 10

2 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/003d697d6686
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.