Geolocation PositionOptions cannot be an arbitrary object

[Greg Bolsinga]

Geolocation PositionOptions cannot be an arbitrary object

From webkit-dev:
Date: Fri, 12 Dec 2008 12:50:35 -0800 (PST)
From: Aurelian Maga aurelianmaga at yahoo com
To: [e-mail]

Hello,

I'm trying the geolocation api (gtk implementation with geoclue) and in a javascript I'm creating a postionoptions object like this:

var options = {enableHighAccuracy:true,timeout:30000};

and pass this to getCurrentPosition.  The GeolocationServiceGtk::startUpdating(PositionOptions* options) is being called but the options object is null.

Could someone point me in  the right direction to fix this?

Thank you,
Relu

[Greg Bolsinga]

Created attachment 26006 [details]
Patch implementing the fix

This is the patch from Sam.

[Darin Adler]

Comment on attachment 26006 [details]
Patch implementing the fix

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 39282)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,19 @@
> [phone]  Greg Bolsinga  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=22847
> +        
> +        PositionOptions can be any aribitrary object.
> +
> +        * DerivedSources.make:
> +        * WebCore.xcodeproj/project.pbxproj:
> +        * bindings/js/JSGeolocationCustom.cpp:
> +        (WebCore::createPositionOptions):
> +        (WebCore::JSGeolocation::getCurrentPosition):
> +        (WebCore::JSGeolocation::watchPosition):
> +        * page/PositionOptions.idl:

I'd like to see slightly more detailed comments explaining the changes.

Also, arbitrary is misspelled here.

> +    // FIXME: Appropriately deal with an exception being thrown.

Is this really something we have to put off? Can't you make a test case that throws an exception, and decide what we want to do in that case?

> +    unsigned timeout = timeoutValue->toInt32(exec);

Should be toUInt32 if you're storing this in an unsigned.

And are you sure we want the toUInt32 rules? This do modulo arithmetic. Another option is to use toNumber and then range check and convert to an integer. You should make test cases that are outside the 32-bit integer range and see what they do.

> Index: WebCore/page/PositionOptions.idl
> ===================================================================
> --- WebCore/page/PositionOptions.idl	(revision 39281)
> +++ WebCore/page/PositionOptions.idl	(working copy)
> @@ -25,9 +25,8 @@
>  
>  module core {
>  
> -    interface [
> -        GenerateConstructor
> -    ] PositionOptions {
> +    // This interface generates no binding for JavaScript.
> +    interface PositionOptions {
>          attribute boolean enableHighAccuracy;
>          attribute unsigned long timeout;
>      };

I think we should eliminate the IDL file entirely.

I see you taking things out of DerivedSources.make and the Xcode project. What about the build files for other platforms? Won't this break the build?

I'm going to say review- because I don't think it's OK to break all the other platforms. Please consider my other comments too.

[Sam Weinig]

Created attachment 26027 [details]
Updated patch

Greg, this is an updated version of the patch which hopefully addresses Darin's comments.  It would great if you could test this with some malformed PositionOptions to make sure all is well.

Example of malformed PositionOptions include:

// Exception being thrown
var o = { get timeout() { throw 1; } }

// Overflow (note UINT_MAX doesn't exist in JS, so you will have to fill it in yourself)
var o = { timeout: UINT_MAX + 1; }

[Darin Adler]

Comment on attachment 26027 [details]
Updated patch

r=me

[Sam Weinig]

Fixed in r39311.