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

[bugzilla-daemon at webkit.org]

steveblock at google.com has asked  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 steveblock at google.com
(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.