[webkit-reviews] review granted: [Bug 27254] Geolocation PositionOptions does not use correct default values : [Attachment 34551] Patch 3 for bug 27254

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted 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 34551: Patch 3 for bug 27254
https://bugs.webkit.org/attachment.cgi?id=34551&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    // 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.