[webkit-reviews] review denied: [Bug 27254] Geolocation PositionOptions does not use correct default values : [Attachment 34524] Patch 2 for bug 27254

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied steveblock at google.com's request for
review:
Bug 27254: Geolocation PositionOptions does not use correct default values
https://bugs.webkit.org/show_bug.cgi?id=27254

Attachment 34524: Patch 2 for bug 27254
https://bugs.webkit.org/attachment.cgi?id=34524&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    JSValue enableHighAccuracyValue = object->get(exec, Identifier(exec,
"enableHighAccuracy"));
> +    if (!enableHighAccuracyValue.isUndefinedOrNull()) {
> +	   // We accept either a boolean or numeric value.
> +	   if (!enableHighAccuracyValue.isBoolean() &&
!enableHighAccuracyValue.isNumber()) {

I think you misinterpreted my comment. Functions in JavaScript that work on
numbers normally convert the passed-in item to a number and don't check
specifically for an actual JavaScript number. It's true that one case that
requires this is a boolean value, but another is a number object such as the
one you would create with "new Number(x)" in JavaScript. This turns undefined
into NaN, null into 0, false into 0, true into 1, and also in the general case
of an object it calls the valueOf function to allow the object to return a
number. That's the normal way the language works, but by going out of our way
to do specific type checking we're disabling that behavior.

It's strange that here we've decided instead to use a sort of strict type
checking approach. Is this called for by the Geolocation specification? If not,
I suggest we use toBoolean or toNumber or toInt32 directly without first doing
a type check, except for any checks for special values like undefined or null
that have special behavior. Do we need that special behavior?

I think you should include test cases that pass an actual "undefined" value as
well as "null" and the too-few-arguments case for the second and third
arguments, and both undefined and null for the first argument. And I think you
should include test cases of all the different types for the values of the
items in the options dictionary too.

I'm going to say review- because I think the type checking here is still
slightly surprising and probably wrong. Otherwise the patch looks good.