[webkit-reviews] review granted: [Bug 27254] Geolocation PositionOptions does not use correct default values : [Attachment 34661] Patch 4 for bug 27254

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted steveblock at google.com's request 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 Darin Adler <darin at apple.com>
> +    // The spec specifies 'FunctionOnly' for this object.
> +    if (!value.isObject(&InternalFunction::info)) {
> +	   setDOMException(exec, TYPE_MISMATCH_ERR);
>	   return 0;
> +    }
>  
> -    JSObject* object = asObject(value);
> +    JSObject* object = value.getObject();

Now that you are doing an isObject check above, it's more efficient to do
asObject as the original code did than to do getObject, which does a type
check. I know this is not clear from the names of the functions. I suggest you
change this line of code back.

> +    // The spec specifies 'FunctionOnly' for this object.
> +    if (!value.isObject(&InternalFunction::info)) {
> +	   setDOMException(exec, TYPE_MISMATCH_ERR);
>	   return 0;
> +    }
>  
> -    JSValue timeoutValue = object->get(exec, Identifier(exec, "timeout"));
> +    JSObject* object = value.getObject();

Same thing here.

> +	   options->setTimeout(std::max(0, timeoutValue.toInt32(exec)));

We normally prefer to do "using namespace std" at the start of the file rather
than using "std::" throughout the file, even if it's only a small number of
call sites.

> Index: WebCore/page/Geolocation.cpp
> ===================================================================
> --- WebCore/page/Geolocation.cpp	(revision 47004)
> +++ WebCore/page/Geolocation.cpp	(working copy)
> @@ -32,6 +32,7 @@
>  #include "Frame.h"
>  #include "Page.h"
>  #include "PositionError.h"
> +#include "PositionOptions.h"

What changed to require adding this include? The old code seems to already
access PositionOptions's timeout member function, so somehow it was seeing the
PositionOptions class definition. Is this added include necessary?

Seems really great! r=me despite the minor nitpicks above.