Closed
Bug 666309
Opened 13 years ago
Closed 13 years ago
Histogram.Add should accept boolean, double values
Categories
(Toolkit :: Telemetry, defect)
Tracking
()
RESOLVED
FIXED
mozilla7
People
(Reporter: taras.mozilla, Assigned: taras.mozilla)
References
Details
Attachments
(1 file)
(deleted),
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
Currently the user is forced to pass in 0/1, even into boolean histograms. This is silly.
Updated•13 years ago
|
Summary: Histogram.Add should accept boolean value → Histogram.Add should accept boolean, double values
Comment 1•13 years ago
|
||
Similarly, as discovered in bug 664758, you can't pass in a jsdouble (e.g. a PRInt64). Sounds like the plan is to fix both of these at once.
Assignee | ||
Comment 2•13 years ago
|
||
Assignee: nobody → tglek
Attachment #542335 -
Flags: review?(jorendorff)
Comment 3•13 years ago
|
||
Comment on attachment 542335 [details] [diff] [review] use jsapi stuff that converts -> double then into an int Review of attachment 542335 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the error handling fixed. ::: toolkit/components/telemetry/Telemetry.cpp @@ +192,1 @@ > return JS_FALSE; Before returning false, a JSNative should first report exactly one error. If you break this rule by reporting two errors, it's a little weird but no huge deal. If you break it by not reporting an error at all, we terminate JS execution without executing finally blocks and without any error message. It's bad. It is part of the contract for almost every JS API function with a cx argument that if it fails, it reports an error for you (with a few exceptions just to keep you on your toes). But in this case where you just don't like the arguments, you have to explicitly JS_ReportError(cx, SADFACE) before returning false. There was already a mistake like this in the original code, but let's fix it up now. @@ +198,5 @@ > + || !JS_ValueToECMAInt32(cx, v, &value)) > + { > + JS_ReportError(cx, "Not a number"); > + return JS_FALSE; > + } At first this seemed like a case of the opposite mistake: if JS_ValueToECMAInt32 fails here, then we get two error reports -- one from JS_ValueToECMAInt32 and one from JS_ReportError. As it happens, JS_ValueToECMAInt32 is infallible when the argument is a number or boolean. Still, it's better not to depend on that: if (!(JSVAL_IS_NUMBER(v) || JSVAL_IS_BOOLEAN(v)) { JS_ReportError(...); return JS_FALSE; } if (!JS_ValueToECMAInt32(...)) return JS_FALSE;
Attachment #542335 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 4•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/8da6beafa20b
Whiteboard: [inbound]
Target Milestone: --- → mozilla7
Comment 5•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/8da6beafa20b
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•13 years ago
|
Whiteboard: [inbound]
You need to log in
before you can comment on or make changes to this bug.
Description
•