Bug 27254 – Geolocation PositionOptions does not use correct default values

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Steve Block]

The default values for the members of the Geolocation PositionOptions object should be as follows, taken from http://www.w3.org/TR/geolocation-API/#position-options.
- enableHighAccuracy = false
- timeout = +Inf
- maximumAge = 0

Currently, no default values are explicitly set, so the effective defaults are false, 0 and 0 respectively.

The default values should be explicitly set, probably in WebCore/bindings/js/JSGeolocationCustom.cpp.

[Greg Bolsinga]

I do not see those specific values for defaults listed, so the "undefined" values seem correct. I may be missing something however.

[Steve Block]

> I do not see those specific values for defaults listed,
It's true that the spec is worded such that the default values aren't made too explicit. Perhaps the spec should be updated to make this more clear?

Andrei, can you confirm that I've interpreted the spec correctly?

[Andrei Popescu]

(In reply to comment #2)
> > I do not see those specific values for defaults listed,
> It's true that the spec is worded such that the default values aren't made too
> explicit. Perhaps the spec should be updated to make this more clear?
> 
> Andrei, can you confirm that I've interpreted the spec correctly?

You are both right:

- the spec has a bug whereby the default values are not explicitly mentioned. I will fix this asap.
- the default values should indeed be:

- enableHighAccuracy = false
- timeout = +Inf
- maximumAge = 0

So it looks like 'timeout' defaults to 0 in WebKit, which should be fixed. I'll also have to check if WebIDL supports +Inf and what to do if it doesn't.

Andrei

[Steve Block]

Created attachment 34269 [details]
Patch 1 for bug 27254

[Darin Adler]

Comment on attachment 34269 [details]
Patch 1 for bug 27254

Generally looks good!

> +const long PositionOptions::infinity = -1;

static members of numeric types like this can, and should, be initialized in the class definition, rather than separately like this.

We normally try to avoid "in-band signaling". That means that for timeouts we'd prefer a design with a separate hasTimeout function rather than a magic value that means no timeout. Is this special value of -1 part of the DOM API or just internal to WebKit? If just internal, I suggest using the separate boolean style

> +    // No value is OK.
> +    if (value.isUndefinedOrNull()) {
>          return 0;
> +    }

Braces here are incorrect.

> +    Frame* frame = toJSDOMWindow(exec->lexicalGlobalObject())->impl()->frame();
> +    ASSERT(frame);

I don't particularly understand the value of the assertion here. If that frame() function is guaranteed to never return 0, I don't think the assertion adds much clarity. If it's not guaranteed to never return 0, then I think we need an actual null check. The assertion seems strangely in between.

> +static bool getNonNegativeLong(ExecState* exec, const JSValue& value, long* result)

We use references, not pointers, for "out" arguments like result.

> +    if (!value.isNumber() || (value.toNumber(exec) < 0)) {
> +        return false;
> +    }

Braces here are incorrect.

> +    *result = static_cast<long>(value.toNumber(exec));

Seems wrong to call toNumber twice. Can that have side effects like JavaScript execution? Also, I'm surprised that this is not toInt32 instead. Is this really correct for all edge cases? How is it different from toInt32? The use of "long" here is a little strange. We normally use "int" for this type of thing, and we've discussed moving to "int32_t", but "long" seems different from the norm for no good reason.

> +    if (value.isUndefinedOrNull()) {
> +        // Use default options.
> +        return options;
> +    }

This should be "return options.release()".

But also, I think for the error case it would be OK to call PositionOptions::create() right at the return statement instead of using an already allocated options object. We could create the options object below right where we start actually making progress on getting the options. Or maybe it's OK as-is.

> +        if (!enableHighAccuracyValue.isBoolean()) {
>              setDOMException(exec, TYPE_MISMATCH_ERR);
> -            return jsUndefined();
> +            return 0;
>          }

Is this really correct? This seems like abnormal behavior for a JavaScript binding. Normally you could pass a 1 to something that expects a boolean without getting a TYPE_MISMATCH_ERR and generally speaking type conversion would take place rather than strict type checking. I'd be quite wary of introducing this behavior unless we're certain the specification calls for it.

> +        long timeout;
> +        if (getNonNegativeLong(exec, timeoutValue, &timeout))

As I said above, the involvement of "long" here seems wrong.

> -    if (m_errorCallback && m_options)
> +    if (m_errorCallback && (m_options->timeout() != PositionOptions::infinity))

We normally wouldn't use the parentheses here. I think this would read more clearly if it was hasTimeout() rather than timeout() != infinity.

>  class PositionOptions : public RefCounted<PositionOptions> {

This is not new to this patch, but I don't understand why position options are a reference counted, heap allocated, class. I think functions could be changed to just take a const PositionOptions& and store a PositionOptions and get rid of the reference counting.

> +    void setTimeout(long t) {

Brace needs to go on the next line. Function braces go on separate lines. See WebKit coding style guild. You could also use the check-webkit-style script, I guess.

> +    void setMaximumAge(long a) {

Ditto.

> +        * geolocation: Added.

A top level directory for these new tests seems like overkill. I suggest putting this test into fast/dom/Geolocation since the fast/dom directory already has subdirectories for various other DOM classes, such as Window, HTMLButtonElement, and many others.

review- because most of these things need to be done

[Steve Block]

Created attachment 34524 [details]
Patch 2 for bug 27254

(In reply to comment #5)

> We normally try to avoid "in-band signaling". That means that for timeouts we'd
> prefer a design with a separate hasTimeout function rather than a magic value
> that means no timeout. Is this special value of -1 part of the DOM API or just
> internal to WebKit? If just internal, I suggest using the separate boolean
> style
No, the -1 was just internal to WebKit. I've updated the patch to use hasTimeout().

> Braces here are incorrect.
Fixed

> I don't particularly understand the value of the assertion here. If that
> frame() function is guaranteed to never return 0, I don't think the assertion
> adds much clarity. If it's not guaranteed to never return 0, then I think we
> need an actual null check. The assertion seems strangely in between.
Previously the code used 'if (frame)'. I don't see a reason why frame would ever be 0, but I could be missing something. I've removed the ASSERT.

> Also, I'm surprised that this is not toInt32 instead. Is this really
> correct for all edge cases? How is it different from toInt32? The use of "long"
> here is a little strange. We normally use "int" for this type of thing, and
> we've discussed moving to "int32_t", but "long" seems different from the norm
> for no good reason.
My mistake. The IDL definition for these parameters uses long and I didn't realize that IDL long is int32. Fixed to use int types. I've also changed the behaviour to use toInt32 (which wraps values outside this range) and to set negative values to zero, to match that of window.setTimeout(). 
 
> This should be "return options.release()".
Fixed.

> But also, I think for the error case it would be OK to call
> PositionOptions::create() right at the return statement instead of using an
> already allocated options object. We could create the options object below
> right where we start actually making progress on getting the options. Or maybe
> it's OK as-is.
Creating the options later, but before we need them, doesn't save us much, so I'd rather leave it as it is.

> Is this really correct? This seems like abnormal behavior for a JavaScript
> binding. Normally you could pass a 1 to something that expects a boolean
> without getting a TYPE_MISMATCH_ERR and generally speaking type conversion
> would take place rather than strict type checking. I'd be quite wary of
> introducing this behavior unless we're certain the specification calls for it.
Updated to allow numbers or boolean.

> A top level directory for these new tests seems like overkill. I suggest
> putting this test into fast/dom/Geolocation since the fast/dom directory
> already has subdirectories for various other DOM classes, such as Window,
> HTMLButtonElement, and many others.
Fixed. Note that fast/dom/Geolocation and the JS test template will be added in Bug 27716 before this patch lands.

[Darin Adler]

Comment on attachment 34524 [details]
Patch 2 for bug 27254

> +    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.

[Steve Block]

(In reply to comment #7)
Thanks for the comments Darin. I'll update the patch to allow the default, lenient conversion to number or boolean for all three PositionOptions properties.

How about the PositionOptions object literal itself? Currently we throw an exception if it's not null, undefined or an object. Is this OK, or should we never throw, and simply treat anything other than an object as if nothing were passed?

[Steve Block]

Created attachment 34551 [details]
Patch 3 for bug 27254

Updated to use generic conversion to int32 and boolean for PositionOptions properties. Also uses generic conversion for PositionOptions argument itself, as per Web IDL spec.

[Darin Adler]

Comment on attachment 34551 [details]
Patch 3 for bug 27254

> +    // FIXME: We should check that the argument is a Function object, as
> +    // the spec specifies 'FunctionOnly'.

A good way to keep track of this would be to have test cases that are failing in the test you are adding. Also, checking this is quite simple so I'm not sure why this is a FIXME and not just fixed. You can use one of these:

    value.isObject(&InternalFunction::info)
    object->inherits(&InternalFunction::info)

But I'm not sure what FunctionOnly's precise definition is. It would be worthwhile to test with both a function defined in JavaScript and also a built-in function such as Math.abs to be sure the check was right.

> +    // Argument is optional, and null is allowed.
> +    if (value.isUndefinedOrNull())

The code below the comment allows three things: 1) Missing argument, 2) null, 3) undefined. The comment mentions two these and not the third, so I think it's not an ideal comment because to me it just raises the question about the third case. Your test cases covers the third, though.

> +    // FIXME: We should check that the argument is a Function object, as
> +    // the spec specifies 'FunctionOnly'.

Same comment as above.

> +    // Argument is optional, and null is allowed.
> +    if (value.isUndefinedOrNull()) {

Same comment as above.

> +    // This will always yield an object.
> +    JSObject* object = value.toObject(exec);

Comment is not clear. Maybe you mean that the only cases where toObject can fail are undefined and null, and since both cases are tested above this can't fail. If so I think you need a slightly more specific comment to be clear. Still good to keep it short.

> +    JSValue enableHighAccuracyValue = object->get(exec, Identifier(exec, "enableHighAccuracy"));

If the object has a getter, this code could raise an exception. We may need code that detects the exception and returns early to get correct behavior. For example, it might be important to return early to avoid further side effects from fetching other values that might have getters. Or it might be important to not run additional code that might overwrite the exception with a different exception. The only real way to check an edge case like this is to have tests that involve getters.

> +    // If undefined, don't override default.
> +    if (!enableHighAccuracyValue.isUndefined())

This uses "undefined" as the special value. Is that right? Is the check specifically about not having the property at all, or is it about the value "undefined" too? I know I've been asking questions like this a lot, but I'd really like to get these things right so we don't have to revisit the code later. The key is having enough corner test cases or having a standard way we do things everywhere so we don't do anything original when we implement a new class like this one.

> -    if (exec->hadException())
> -        return 0;

You've removed these checks for exceptions, but I don't understand why. Operations like get and toInt32 can indeed raise exceptions when there are unusual objects involved. And we want predictable behavior in such cases. Maybe we can ignore the exception, but I'd like to be sure we can -- test cases are a good way to prove it's OK. It may be inconvenient to make all these test cases, though. You need objects with valueOf functions that raise exceptions to test toInt32 raising an exception and getters that raise exceptions to test get raising an exception. I could even imagine having a test that detects the order the properties are gotten and processed by using valueOf and getter functions that have side effects.

> +        // Wrap to int32 and force non-negative to match behavior of window.setTimeout.
> +        int timeout = timeoutValue.toInt32(exec);
> +        options->setTimeout(timeout >= 0 ? timeout : 0);

The above is is fine. Another way to write it is:

    options->setTimeout(max(0, timeoutValue.toInt32(exec));

I like the more-terse version, but some might prefer your version. And as I said above I think it should be followed by this:

    if (exec->hadException())
        return 0;

> +    static PassRefPtr<PositionOptions> create() { return adoptRef(new PositionOptions()); }

No need for the parentheses here after PositionOptions. I like the formatting better without it.

> +    int timeout() const { return m_timeout; }

I think this should have ASSERT(hasTimeout()) in it.

> +    void setTimeout(int t)

Could name this "timeout" instead of "t". I think people have been asking for that during reviews.

> +    void setMaximumAge(int a)

Could name this "age" instead of "a".

Test looks great and covers a lot more cases now. My remaining concern is that we want our behavior to match the specification and other browsers. While I have reviewed the patch and offered my opinion on how to handle edge cases I would love that behavior to perfectly match the specification and the other browsers that have implemented this. I'm not sure whether you've used this test case with, say, Firefox, to see how their implementation matches ours.

I'm going to say r=me because this is pretty good. But as you can see with my comments above there is still some room for improvement.

[Andrei Popescu]

Hi Darin,

(In reply to comment #10)
> (From update of attachment 34551 [details])
> > +    // FIXME: We should check that the argument is a Function object, as
> > +    // the spec specifies 'FunctionOnly'.
> 
> A good way to keep track of this would be to have test cases that are failing
> in the test you are adding. Also, checking this is quite simple so I'm not sure
> why this is a FIXME and not just fixed. You can use one of these:
> 
>     value.isObject(&InternalFunction::info)
>     object->inherits(&InternalFunction::info)
> 
> But I'm not sure what FunctionOnly's precise definition is. It would be
> worthwhile to test with both a function defined in JavaScript and also a
> built-in function such as Math.abs to be sure the check was right.
> 

After re-reading section 4.6 of the WebIDL spec, my understanding is that, when an interface is declared with the FunctionOnly argument (like PositionCallback in this case), any Function object, be that a built-in internal function or a function defined in JS, is considered to implement the interface.

[Darin Adler]

(In reply to comment #11)
> >     value.isObject(&InternalFunction::info)
> >     object->inherits(&InternalFunction::info)
> 
> After re-reading section 4.6 of the WebIDL spec, my understanding is that, when
> an interface is declared with the FunctionOnly argument (like PositionCallback
> in this case), any Function object, be that a built-in internal function or a
> function defined in JS, is considered to implement the interface.

OK, then the check against InternalFunction::info should do the right thing.

[Steve Block]

Created attachment 34661 [details]
Patch 4 for bug 27254

> A good way to keep track of this would be to have test cases that are failing
> in the test you are adding. Also, checking this is quite simple so I'm not sure
> why this is a FIXME and not just fixed.
It was a FIXME because I wasn't sure of the precise definition of FunctionOnly. Based on Andrei's comment, this is fixed with value.isObject(&InternalFunction::info). Added test with Math.abs.

> The code below the comment allows three things: 1) Missing argument, 2) null,
> 3) undefined. The comment mentions two these and not the third, so I think it's
> not an ideal comment because to me it just raises the question about the third
> case. Your test cases covers the third, though.
I thought that undefined is indistinguishable from a missing argument in JS? I've updated the comment to be more explicit.

> Comment is not clear. Maybe you mean that the only cases where toObject can
> fail are undefined and null, and since both cases are tested above this can't
> fail. If so I think you need a slightly more specific comment to be clear.
Exactly. Fixed comment.

> If the object has a getter, this code could raise an exception. We may need
> code that detects the exception and returns early to get correct behavior.
Fixed to return early. Added test cases.

> This uses "undefined" as the special value. Is that right? Is the check
> specifically about not having the property at all, or is it about the value
> "undefined" too? 
A value of undefined and no value specified should behave the same.

> You've removed these checks for exceptions, but I don't understand why.
> Operations like get and toInt32 can indeed raise exceptions when there are
> unusual objects involved.
Fixed to return early. Added test cases. 

> No need for the parentheses here after PositionOptions. I like the formatting
> better without it.
Fixed.

> I think this should have ASSERT(hasTimeout()) in it.
Fixed.

> Could name this "timeout" instead of "t". I think people have been asking for
> that during reviews.
Fixed.

[Darin Adler]

(In reply to comment #13)
> I thought that undefined is indistinguishable from a missing argument in JS?

They are distinguishable by checking the length of the arguments array. The built-in functions in the language always treat undefined and missing arguments the same way. But DOM functions have been designed in the past to treat them differently. I personally have argued to have the DOM binding for JavaScript match JavaScript's only built-in functions in this respect, but Mozilla's JavaScript binding seems to do a lot of argument array length checking and I am not sure what WebIDL ended up saying about this.

[Darin Adler]

Comment on attachment 34661 [details]
Patch 4 for bug 27254

> +    // The spec specifies 'FunctionOnly' for this object.
> +    if (!value.isObject(&InternalFunction::info)) {
> +        setDOMException(exec, TYPE_MISMATCH_ERR);
>          return 0;
> +    }
>  
> -    JSObject* object = asObject(value);
> +    JSObject* object = value.getObject();

Now that you are doing an isObject check above, it's more efficient to do asObject as the original code did than to do getObject, which does a type check. I know this is not clear from the names of the functions. I suggest you change this line of code back.

> +    // The spec specifies 'FunctionOnly' for this object.
> +    if (!value.isObject(&InternalFunction::info)) {
> +        setDOMException(exec, TYPE_MISMATCH_ERR);
>          return 0;
> +    }
>  
> -    JSValue timeoutValue = object->get(exec, Identifier(exec, "timeout"));
> +    JSObject* object = value.getObject();

Same thing here.

> +        options->setTimeout(std::max(0, timeoutValue.toInt32(exec)));

We normally prefer to do "using namespace std" at the start of the file rather than using "std::" throughout the file, even if it's only a small number of call sites.

> Index: WebCore/page/Geolocation.cpp
> ===================================================================
> --- WebCore/page/Geolocation.cpp	(revision 47004)
> +++ WebCore/page/Geolocation.cpp	(working copy)
> @@ -32,6 +32,7 @@
>  #include "Frame.h"
>  #include "Page.h"
>  #include "PositionError.h"
> +#include "PositionOptions.h"

What changed to require adding this include? The old code seems to already access PositionOptions's timeout member function, so somehow it was seeing the PositionOptions class definition. Is this added include necessary?

Seems really great! r=me despite the minor nitpicks above.

[Steve Block]

Created attachment 34676 [details]
Patch 5 for bug 27254

Thanks Darin. This final patch addresses your nits and updates the structure of the Layout tests to reflect the final decision in Bug 27716.

If the patch looks OK, please could you commit on my behalf once my patch in Bug 27716 has been committed - I don't have commit rights.

[Eric Seidel (no email)]

Comment on attachment 34676 [details]
Patch 5 for bug 27254

Rejecting patch 34676 from commit-queue.  This patch will require manual commit.

WebKitTools/Scripts/run-webkit-tests --no-launch-safari --quiet failed with exit code 1

[Eric Seidel (no email)]

Comment on attachment 34676 [details]
Patch 5 for bug 27254

I'm not sure why this patch was rejected.  I'm missing yesterday's commit-queue log, so I can't tell you either.  Adding it back.  I suspect that this caused some test to fail (or maybe the bots were failing when the commit-queue tried to land this).

[Eric Seidel (no email)]

Comment on attachment 34676 [details]
Patch 5 for bug 27254

Rejecting patch 34676 from commit-queue.  This patch will require manual commit.

W e b K i t T o o l s / S c r i p t s / b u i l d - w e b k i t failed with exit code 1

[Eric Seidel (no email)]

build-webkit failed with your patch applied.

/Users/eseidel/Projects/WebKit2/WebCore/bindings/js/JSGeolocationCustom.cpp: In function ‘WTF::PassRefPtr<WebCore::PositionCallback> WebCore::createPositionCallback(JSC::ExecState*, JSC::JSValue)’:
/Users/eseidel/Projects/WebKit2/WebCore/bindings/js/JSGeolocationCustom.cpp:46: error: incomplete type ‘JSC::InternalFunction’ used in nested name specifier
/Users/eseidel/Projects/WebKit2/WebCore/bindings/js/JSGeolocationCustom.cpp: In function ‘WTF::PassRefPtr<WebCore::PositionErrorCallback> WebCore::createPositionErrorCallback(JSC::ExecState*, JSC::JSValue)’:
/Users/eseidel/Projects/WebKit2/WebCore/bindings/js/JSGeolocationCustom.cpp:63: error: incomplete type ‘JSC::InternalFunction’ used in nested name specifier
distcc[26395] ERROR: compile /Users/eseidel/Projects/WebKit2/WebCore/bindings/js/JSGeolocationCustom.cpp on localhost failed
** BUILD FAILED **

[Darin Adler]

http://trac.webkit.org/changeset/47252