[webkit-reviews] review requested: [Bug 27254] Geolocation PositionOptions does not use correct default values : [Attachment 34661] Patch 4 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 34661: Patch 4 for bug 27254
https://bugs.webkit.org/attachment.cgi?id=34661&action=review

------- Additional Comments from steveblock at google.com
> 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.
It was a FIXME because I wasn't sure of the precise definition of FunctionOnly.
Based on Andrei's comment, this is fixed with
value.isObject(&InternalFunction::info). Added test with Math.abs.

> 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.
I thought that undefined is indistinguishable from a missing argument in JS?
I've updated the comment to be more explicit.

> 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.
Exactly. Fixed comment.

> 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.
Fixed to return early. Added test cases.

> 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? 
A value of undefined and no value specified should behave the same.

> 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.
Fixed to return early. Added test cases. 

> No need for the parentheses here after PositionOptions. I like the formatting

> better without it.
Fixed.

> I think this should have ASSERT(hasTimeout()) in it.
Fixed.

> Could name this "timeout" instead of "t". I think people have been asking for

> that during reviews.
Fixed.