[Webkit-unassigned] [Bug 27254] Geolocation PositionOptions does not use correct default values

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=27254


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #34269|review?                     |review-
               Flag|                            |




--- Comment #5 from Darin Adler <darin at apple.com>  2009-08-07 07:33:06 PDT ---
(From update of attachment 34269)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.