(dormant account) Assignee
	Description • 13 years ago

Currently the user is forced to pass in 0/1, even into boolean histograms. This is silly.

	Justin Lebar (not reading bugmail)
	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.

	Jason Orendorff [: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;

	(dormant account) Assignee
	Comment 4 • 13 years ago

http://hg.mozilla.org/integration/mozilla-inbound/rev/8da6beafa20b

	Marco Bonardo [:mak]
	Comment 5 • 13 years ago

http://hg.mozilla.org/mozilla-central/rev/8da6beafa20b