[webkit-reviews] review requested: [Bug 29099] Geolocation does not correctly handle Infinity for PositionOptions properties. : [Attachment 39339] Patch 3 for bug 29099

[bugzilla-daemon at webkit.org]

Steve Block <steveblock at google.com> has asked  for review:
Bug 29099: Geolocation does not correctly handle Infinity for PositionOptions
properties.
https://bugs.webkit.org/show_bug.cgi?id=29099

Attachment 39339: Patch 3 for bug 29099
https://bugs.webkit.org/attachment.cgi?id=39339&action=review

------- Additional Comments from Steve Block <steveblock at google.com>
> > >  Only positive infinity?
> > Yes, we're only interested in positive infinity.
> (From update of attachment 39315 [details])
> So would:
>  109	       if (!(isinf(timeoutNumber) && (timeoutNumber > 0))) {
> if (isfinite(timeoutNumber))
> get you the same effect?
No. The special case I want to handle (for both timeout and maximumAge) is
+Inf. I'm not interested in -Inf. These values represent time limits, where a
value of +Inf means 'no time limit applied'. All other values are converted to
int32 and then negative values are set to 0. (This behaviour was chosen to
follow the behaviour of window.setTimeout. See Bug 27254.)

The logic we need is ...

// Special case for +Inf - do nothing
if (timeout != +Inf)
  // Convert to Int32 and set negative values to 0

if (maximumAge == +Inf)
  // Special case for +Inf - clear maximum age
else
  // Convert to Int32 and set negative values to 0


Using ...

if (isfinite(timeout))

will cause a value of -Inf to skip the if, which is the wrong behaviour. It's
true that I could use ...

if (isinfinite(timeout) || timeout < 0)

but I thought it better to match the structure of the logic to that used in the
if statement for maximumAge.

> Also, shouldn't we be testing negative infinity in the test cases?
Given that the change adds a new code path only for +Inf, I thought testing
+Inf was sufficient. I've added tests for -Inf too.