Closed
Bug 1315909
Opened 8 years ago
Closed 5 years ago
JS scalar and histogram adding functions should treat value arguments the same
Categories
(Toolkit :: Telemetry, defect, P1)
Toolkit
Telemetry
Tracking
()
RESOLVED
WORKSFORME
| Tracking | Status | |
|---|---|---|
| firefox52 | --- | wontfix |
People
(Reporter: gfritzsche, Assigned: brizental)
References
(Blocks 1 open bug)
Details
(Whiteboard: [measurement:client])
Currently the histogram JS add functions discard calls with arguments that are not numbers or booleans:
https://dxr.mozilla.org/mozilla-central/rev/86f702229e32c6119d092e86431afee576f033a1/toolkit/components/telemetry/TelemetryHistogram.cpp#1623
https://dxr.mozilla.org/mozilla-central/rev/86f702229e32c6119d092e86431afee576f033a1/toolkit/components/telemetry/TelemetryHistogram.cpp#1875
While the scalar add & set functions accept values that convert to Uint32:
https://dxr.mozilla.org/mozilla-central/rev/86f702229e32c6119d092e86431afee576f033a1/toolkit/components/telemetry/TelemetryScalar.cpp#227
https://dxr.mozilla.org/mozilla-central/rev/86f702229e32c6119d092e86431afee576f033a1/toolkit/components/telemetry/TelemetryScalar.cpp#248
We need to make the two APIs follow the same logic.
|
||
Updated•8 years ago
|
Priority: P3 → P2
|
|
||
Updated•8 years ago
|
Priority: P2 → P3
|
||
Comment 1•8 years ago
|
||
Mass wontfix for bugs affecting firefox 52.
|
Assignee | |
Updated•5 years ago
|
Assignee: nobody → brizental
|
|
Assignee | |
Updated•5 years ago
|
Priority: P3 → P1
|
|
Assignee | |
Comment 2•5 years ago
|
||
I did some investigation on this bug and, from what I understand, it is invalid.
The code has changed quite a bit since this was created. The Scalars code works similarly, but the Histogram doesn't have this bug anymore.
There is a function that coerces any JS type that it can, not only Number and Boolean, into Uint32. See https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/core/TelemetryHistogram.cpp#1660.
Since I am still pretty new to this codebase, I would like to confirm this with :chutten before resolving this as invalid, or not :)
Flags: needinfo?(chutten)
|
||
Comment 3•5 years ago
|
||
Looks like a solid conclusion from a thorough investigation. This bug's good to be RESOLVED as WORKSFORME.
Flags: needinfo?(chutten)
|
|
Assignee | |
Updated•5 years ago
|
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WORKSFORME
You need to log in
before you can comment on or make changes to this bug.
Description
•