Correctly sets default values for Geolocation PositionOptions.

This will be submitted to WebKit in bug 27254.

This was first commited in change 20268, but this caused problems in the automated build and was subsequently rolled back in change 20415.
This change includes the latest comments from the WebKit review.

3 files changed, 105 insertions, 84 deletions

diff --git a/WebCore/bindings/js/JSGeolocationCustom.cpp b/WebCore/bindings/js/JSGeolocationCustom.cpp
index 493166c..7636a44 100644
--- a/WebCore/bindings/js/JSGeolocationCustom.cpp
+++ b/WebCore/bindings/js/JSGeolocationCustom.cpp
@@ -39,109 +39,115 @@ using namespace JSC;
 
 namespace WebCore {
 
-static PassRefPtr<PositionOptions> createPositionOptions(ExecState* exec, JSValue value)
+static PassRefPtr<PositionCallback> createPositionCallback(ExecState* exec, JSValue value)
 {
-    if (!value.isObject())
+    // FIXME: We should check that the argument is a Function object, as
+    // the spec specifies 'FunctionOnly'.
+    JSObject* object = value.getObject();
+    if (!object) {
+        setDOMException(exec, TYPE_MISMATCH_ERR);
         return 0;
+    }
 
-    JSObject* object = asObject(value);
+    Frame* frame = toJSDOMWindow(exec->lexicalGlobalObject())->impl()->frame();
+    return JSCustomPositionCallback::create(object, frame);
+}
 
-    JSValue enableHighAccuracyValue = object->get(exec, Identifier(exec, "enableHighAccuracy"));
-    if (exec->hadException())
+static PassRefPtr<PositionErrorCallback> createPositionErrorCallback(ExecState* exec, JSValue value)
+{
+    // Argument is optional, and null is allowed.
+    if (value.isUndefinedOrNull())
         return 0;
-    bool enableHighAccuracy = enableHighAccuracyValue.toBoolean(exec);
-    if (exec->hadException())
+
+    // FIXME: We should check that the argument is a Function object, as
+    // the spec specifies 'FunctionOnly'.
+    JSObject* object = value.getObject();
+    if (!object) {
+        setDOMException(exec, TYPE_MISMATCH_ERR);
         return 0;
+    }
+
+    Frame* frame = toJSDOMWindow(exec->lexicalGlobalObject())->impl()->frame();
+    return JSCustomPositionErrorCallback::create(object, frame);
+}
+
+static PassRefPtr<PositionOptions> createPositionOptions(ExecState* exec, JSValue value)
+{
+    // Create default options.
+    RefPtr<PositionOptions> options = PositionOptions::create();
+
+    // Argument is optional, and null is allowed.
+    if (value.isUndefinedOrNull()) {
+        // Use default options.
+        return options.release();
+    }
+
+    // This will always yield an object.
+    JSObject* object = value.toObject(exec);
+
+    JSValue enableHighAccuracyValue = object->get(exec, Identifier(exec, "enableHighAccuracy"));
+    // If undefined, don't override default.
+    if (!enableHighAccuracyValue.isUndefined())
+        options->setEnableHighAccuracy(enableHighAccuracyValue.toBoolean(exec));
 
     JSValue timeoutValue = object->get(exec, Identifier(exec, "timeout"));
-    if (exec->hadException())
-        return 0;
-    unsigned timeout = timeoutValue.toUInt32(exec);
-    if (exec->hadException())
-        return 0;
+    // If undefined, don't override default.
+    if (!timeoutValue.isUndefined()) {
+        // Wrap to int32 and force non-negative to match behavior of window.setTimeout.
+        int timeout = timeoutValue.toInt32(exec);
+        options->setTimeout(timeout >= 0 ? timeout : 0);
+    }
 
     JSValue maximumAgeValue = object->get(exec, Identifier(exec, "maximumAge"));
-    if (exec->hadException())
-        return 0;
-    unsigned maximumAge = maximumAgeValue.toUInt32(exec);
-    if (exec->hadException())
-        return 0;
+    // If undefined, don't override default.
+    if (!maximumAgeValue.isUndefined()) {
+        // Wrap to int32 and force non-negative to match behavior of window.setTimeout.
+        int maximumAge = maximumAgeValue.toInt32(exec);
+        options->setTimeout(maximumAge >= 0 ? maximumAge : 0);
+    }
 
-    return PositionOptions::create(enableHighAccuracy, timeout, maximumAge);
+    return options.release();
 }
 
 JSValue JSGeolocation::getCurrentPosition(ExecState* exec, const ArgList& args)
 {
     // Arguments: PositionCallback, (optional)PositionErrorCallback, (optional)PositionOptions
-    RefPtr<PositionCallback> positionCallback;
-    JSObject* object = args.at(0).getObject();
+
+    RefPtr<PositionCallback> positionCallback = createPositionCallback(exec, args.at(0));
     if (exec->hadException())
         return jsUndefined();
-    if (!object) {
-        setDOMException(exec, TYPE_MISMATCH_ERR);
+    ASSERT(positionCallback);
+
+    RefPtr<PositionErrorCallback> positionErrorCallback = createPositionErrorCallback(exec, args.at(1));
+    if (exec->hadException())
         return jsUndefined();
-    }
 
-    if (Frame* frame = toJSDOMWindow(exec->lexicalGlobalObject())->impl()->frame())
-        positionCallback = JSCustomPositionCallback::create(object, frame);
-    
-    RefPtr<PositionErrorCallback> positionErrorCallback;
-    if (!args.at(1).isUndefinedOrNull()) {
-        JSObject* object = args.at(1).getObject();
-        if (!object) {
-            setDOMException(exec, TYPE_MISMATCH_ERR);
-            return jsUndefined();
-        }
-
-        if (Frame* frame = toJSDOMWindow(exec->lexicalGlobalObject())->impl()->frame())
-            positionErrorCallback = JSCustomPositionErrorCallback::create(object, frame);
-    }
-    
-    RefPtr<PositionOptions> positionOptions;
-    if (!args.at(2).isUndefinedOrNull()) {
-        positionOptions = createPositionOptions(exec, args.at(2));
-        if (exec->hadException())
-            return jsUndefined();
-    }
+    RefPtr<PositionOptions> positionOptions = createPositionOptions(exec, args.at(2));
+    if (exec->hadException())
+        return jsUndefined();
+    ASSERT(positionOptions);
 
     m_impl->getCurrentPosition(positionCallback.release(), positionErrorCallback.release(), positionOptions.release());
-    
     return jsUndefined();
 }
 
 JSValue JSGeolocation::watchPosition(ExecState* exec, const ArgList& args)
 {
     // Arguments: PositionCallback, (optional)PositionErrorCallback, (optional)PositionOptions
-    RefPtr<PositionCallback> positionCallback;
-    JSObject* object = args.at(0).getObject();
+
+    RefPtr<PositionCallback> positionCallback = createPositionCallback(exec, args.at(0));
     if (exec->hadException())
         return jsUndefined();
-    if (!object) {
-        setDOMException(exec, TYPE_MISMATCH_ERR);
+    ASSERT(positionCallback);
+
+    RefPtr<PositionErrorCallback> positionErrorCallback = createPositionErrorCallback(exec, args.at(1));
+    if (exec->hadException())
         return jsUndefined();
-    }
-    
-    if (Frame* frame = toJSDOMWindow(exec->lexicalGlobalObject())->impl()->frame())
-        positionCallback = JSCustomPositionCallback::create(object, frame);
-    
-    RefPtr<PositionErrorCallback> positionErrorCallback;
-    if (!args.at(1).isUndefinedOrNull()) {
-        JSObject* object = args.at(1).getObject();
-        if (!object) {
-            setDOMException(exec, TYPE_MISMATCH_ERR);
-            return jsUndefined();
-        }
-        
-        if (Frame* frame = toJSDOMWindow(exec->lexicalGlobalObject())->impl()->frame())
-            positionErrorCallback = JSCustomPositionErrorCallback::create(object, frame);
-    }
-    
-    RefPtr<PositionOptions> positionOptions;
-    if (!args.at(2).isUndefinedOrNull()) {
-        positionOptions = createPositionOptions(exec, args.at(2));
-        if (exec->hadException())
-            return jsUndefined();
-    }
+
+    RefPtr<PositionOptions> positionOptions = createPositionOptions(exec, args.at(2));
+    if (exec->hadException())
+        return jsUndefined();
+    ASSERT(positionOptions);
 
     int watchID = m_impl->watchPosition(positionCallback.release(), positionErrorCallback.release(), positionOptions.release());
     return jsNumber(exec, watchID);
diff --git a/WebCore/page/Geolocation.cpp b/WebCore/page/Geolocation.cpp
index 7010339..fd429c4 100644
--- a/WebCore/page/Geolocation.cpp
+++ b/WebCore/page/Geolocation.cpp
@@ -40,11 +40,15 @@ Geolocation::GeoNotifier::GeoNotifier(PassRefPtr<PositionCallback> successCallba
     , m_options(options)
     , m_timer(this, &Geolocation::GeoNotifier::timerFired)
 {
+    ASSERT(m_successCallback);
+    // If no options were supplied from JS, we should have created a default set
+    // of options in JSGeolocationCustom.cpp.
+    ASSERT(m_options);
 }
 
 void Geolocation::GeoNotifier::startTimer()
 {
-    if (m_errorCallback && m_options)
+    if (m_errorCallback && m_options->hasTimeout())
         m_timer.startOneShot(m_options->timeout() / 1000.0);
 }
 
diff --git a/WebCore/page/PositionOptions.h b/WebCore/page/PositionOptions.h
index 10845d3..d714902 100644
--- a/WebCore/page/PositionOptions.h
+++ b/WebCore/page/PositionOptions.h
@@ -33,26 +33,37 @@ namespace WebCore {
     
 class PositionOptions : public RefCounted<PositionOptions> {
 public:
-    static PassRefPtr<PositionOptions> create(bool highAccuracy, unsigned timeout, unsigned maximumAge) { return adoptRef(new PositionOptions(highAccuracy, timeout, maximumAge)); }
+    static PassRefPtr<PositionOptions> create() { return adoptRef(new PositionOptions()); }
 
     bool enableHighAccuracy() const { return m_highAccuracy; }
     void setEnableHighAccuracy(bool enable) { m_highAccuracy = enable; }
-    unsigned timeout() const { return m_timeout; }
-    void setTimeout(unsigned t) { m_timeout = t; }
-    unsigned maximumAge() const { return m_maximumAge; }
-    void setMaximumAge(unsigned a) { m_maximumAge = a; }
+    bool hasTimeout() { return m_hasTimeout; }
+    int timeout() const { return m_timeout; }
+    void setTimeout(int t)
+    {
+        ASSERT(t >= 0);
+        m_hasTimeout = true;
+        m_timeout = t;
+    }
+    int maximumAge() const { return m_maximumAge; }
+    void setMaximumAge(int a)
+    {
+        ASSERT(a >= 0);
+        m_maximumAge = a;
+    }
     
 private:
-    PositionOptions(bool highAccuracy, unsigned timeout, unsigned maximumAge)
-        : m_highAccuracy(highAccuracy)
-        , m_timeout(timeout)
-        , m_maximumAge(maximumAge)
+    PositionOptions()
+        : m_highAccuracy(false)
+        , m_hasTimeout(false)
+        , m_maximumAge(0)
     {
     }
     
     bool m_highAccuracy;
-    unsigned m_timeout;
-    unsigned m_maximumAge;
+    bool m_hasTimeout;
+    int m_timeout;
+    int m_maximumAge;
 };
     
 } // namespace WebCore