[webkit-reviews] review denied: [Bug 22847] Geolocation PositionOptions cannot be an arbitrary object : [Attachment 26006] Patch implementing the fix

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Dec 14 18:18:13 PST 2008


Darin Adler <darin at apple.com> has denied Greg Bolsinga <bolsinga at apple.com>'s
request for review:
Bug 22847: Geolocation PositionOptions cannot be an arbitrary object
https://bugs.webkit.org/show_bug.cgi?id=22847

Attachment 26006: Patch implementing the fix
https://bugs.webkit.org/attachment.cgi?id=26006&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> 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.


More information about the webkit-reviews mailing list