Bug 29099

Created attachment 39283 [details] Patch 1 for bug 29099 Fixes bug and updates LayoutTest.

Comment on attachment 39283 [details] Patch 1 for bug 29099 It's a little strange that this is the first use of Inf. Makes me wonder if it should be exported. Obviously it must already be in a header somewhere. In general this change looks fine. I just find the Inf thing a bit strange. Isn't there a posix function for detecting infitiy in a double? Only positive infinity? What about isinfinite(foo) instead? I think r- for the strange Inf export and no discussion of handling negative infinity or not. We should probably say "positive infinity" instead of "infinity" in the comments.

> Isn't there a posix function for detecting infitiy in a double? Yes, but I thought it better to use the JSC abstraction for Infinity, rather than resorting to platform-specifics. Inf is assigned in JavaScriptCore/runtime/JSCell.cpp, using different values dependent upon the platform. To safely test for infinity using platform-specifics, I'd have to repeat this logic. > Only positive infinity? Yes, we're only interested in positive infinity. All negative values are converted to 0 for these parameters anyway. > What about isinfinite(foo) instead? isinf() tests for (x == +Inf || x == -Inf), but I'm not sure it's as safe for use across multiple platforms.

wtf/MathExtras.h provides isinf(), so it seems we provide it for all platforms which don't support it natively.

Using Inf could be totally fine, I don't know. You'd have to ask a current JSC hacker. Try olliej, mjs, ggaren or gbarra in #webkit.

Created attachment 39315 [details] Patch 2 for bug 29099 I checked with olliej and it's safe to use (isinf(x) && x > 0). Have updated the patch to use this method, and fixed comments to use 'positive infinity'.

Comment on attachment 39315 [details] Patch 2 for bug 29099 So would: 109 if (!(isinf(timeoutNumber) && (timeoutNumber > 0))) { if (isfinite(timeoutNumber)) get you the same effect? negativeInfinity will be ignored, which is maybe the wrong behavior? Also, shouldn't we be testing negative infinity in the test cases? Do the tests test any negative numbers?

Created attachment 39339 [details] Patch 3 for bug 29099 > > > 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.

> if (isinfinite(timeout) || timeout < 0) oops, i meant ... if (isfinite(timeout) || timeout < 0)

> So are -Inf and +Inf always supposed to have the same behavior? No, -Inf and +Inf are not supposed to have the same behaviour. > I thought negative numbers were disallowed for someof these, no? No, the correct behaviour is to accept (ie not throw an exception for) any JS type passed for these PositionOptions properties. We then convert to a number using the standard JS conversions. The logic we follow is that I described previously. If the value is +Inf, we take the special-case action. Otherwise, we convert to int32 and then set negative values to zero, following the pattern of window.setTimeout.

Hi Eric, (In reply to comment #10) > > So are -Inf and +Inf always supposed to have the same behavior? This is all covered by the spec. Here is the relevant section: http://dev.w3.org/geo/api/spec-source.html#get-current-position So, in brief, any negative number, including -Inf, will be converted to 0. > > I thought negative numbers were disallowed for someof these, no? Again, this in the spec: any numbers are allowed. The spec says what the implementation must do with the numbers given to it. -Inf will be converted to 0. So this patch makes the implementation compliant with the spec. Thanks, Andrei

Comment on attachment 39339 [details] Patch 3 for bug 29099 Won't this break Chromium's builder? This needs v8 changes too, no? I mean, I'm OK approving as-is, but darin or dglazkov would probably yell at me. ;)

(In reply to comment #12) > (From update of attachment 39339 [details]) > Won't this break Chromium's builder? This needs v8 changes too, no? I mean, > I'm OK approving as-is, but darin or dglazkov would probably yell at me. ;) (Steve's on vacation so I will try to cover for him while he's gone) This change will not break chromium because, AFAIK, Chromium doesn't yet build Geolocation at all. The V8 bindings are missing. We have them written and they are in our queue of things to upstream. Dimitri is CCed to this bug so can probably confirm. And, anyway, if this change somehow manages to break Chromium, then he should yell at us, not at you :)

Shouldn't break Chromium build. If it does, you'll hear from me :)

Comment on attachment 39339 [details] Patch 3 for bug 29099 rs=me, based on Eric's feedback.

Comment on attachment 39339 [details] Patch 3 for bug 29099 Clearing flags on attachment: 39339 Committed r48503: <http://trac.webkit.org/changeset/48503>

All reviewed patches have been landed. Closing bug.