[Webkit-unassigned] [Bug 22847] Geolocation PositionOptions cannot be an arbitrary object

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=22847


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #26006|review?                     |review-
               Flag|                            |




------- Comment #2 from darin at apple.com  2008-12-14 18:18 PDT -------
(From update of attachment 26006)
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 39282)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,19 @@
> +2008-12-13  Greg Bolsinga  <bolsinga at apple.com>
> +
> +        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.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.