Correctly applies Geolocation timeout parameter.

Currently, the timeout is started when permissions are granted and stopped
when the success callback is made, thus rendering the timeout useless. The
correct behavior is to start the timeout when the request is started, and
to stop it as soon as a position fix is obtained.

Also, the timeout should always be applied, even if no error callback is present
and the request should be stopped on timeout.

This will be upstreamed to WebKit in bug 27256.

2 files changed, 40 insertions, 23 deletions

diff --git a/WebCore/page/Geolocation.cpp b/WebCore/page/Geolocation.cpp
index 7fa3a6d..aaf164d 100644
--- a/WebCore/page/Geolocation.cpp
+++ b/WebCore/page/Geolocation.cpp
@@ -56,9 +56,9 @@ void Geolocation::GeoNotifier::setFatalError(PassRefPtr<PositionError> error)
     m_timer.startOneShot(0);
 }
 
-void Geolocation::GeoNotifier::startTimer()
+void Geolocation::GeoNotifier::startTimerIfNeeded()
 {
-    if (m_errorCallback && m_options->hasTimeout())
+    if (m_options->hasTimeout())
         m_timer.startOneShot(m_options->timeout() / 1000.0);
 }
 
@@ -75,8 +75,11 @@ void Geolocation::GeoNotifier::timerFired(Timer<GeoNotifier>*)
         return;
     }
 
-    RefPtr<PositionError> error = PositionError::create(PositionError::TIMEOUT, "Timed out");
-    m_errorCallback->handleEvent(error.get());
+    if (m_errorCallback) {
+        RefPtr<PositionError> error = PositionError::create(PositionError::TIMEOUT, "Timed out");
+        m_errorCallback->handleEvent(error.get());
+    }
+    m_geolocation->requestTimedOut(this);
 }
 
 Geolocation::Geolocation(Frame* frame)
@@ -107,7 +110,9 @@ void Geolocation::getCurrentPosition(PassRefPtr<PositionCallback> successCallbac
         RefPtr<PositionError> error = PositionError::create(PositionError::PERMISSION_DENIED, permissionDeniedErrorMessage);
         notifier->setFatalError(error.release());
     } else {
-        if (!m_service->startUpdating(notifier->m_options.get())) {
+        if (m_service->startUpdating(notifier->m_options.get()))
+            notifier->startTimerIfNeeded();
+        else {
             if (notifier->m_errorCallback) {
                 RefPtr<PositionError> error = PositionError::create(PositionError::PERMISSION_DENIED, "Unable to Start");
                 notifier->m_errorCallback->handleEvent(error.get());
@@ -145,7 +150,9 @@ int Geolocation::watchPosition(PassRefPtr<PositionCallback> successCallback, Pas
         RefPtr<PositionError> error = PositionError::create(PositionError::PERMISSION_DENIED, permissionDeniedErrorMessage);
         notifier->setFatalError(error.release());
     } else {
-        if (!m_service->startUpdating(notifier->m_options.get())) {
+        if (m_service->startUpdating(notifier->m_options.get()))
+            notifier->startTimerIfNeeded();
+        else {
             if (notifier->m_errorCallback) {
                 RefPtr<PositionError> error = PositionError::create(PositionError::PERMISSION_DENIED, "Unable to Start");
                 notifier->m_errorCallback->handleEvent(error.get());
@@ -161,6 +168,15 @@ int Geolocation::watchPosition(PassRefPtr<PositionCallback> successCallback, Pas
     return sIdentifier;
 }
 
+void Geolocation::requestTimedOut(GeoNotifier* notifier)
+{
+    // If this is a one-shot request, stop it.
+    m_oneShots.remove(notifier);
+
+    if (!hasListeners())
+        m_service->stopUpdating();
+}
+
 void Geolocation::clearWatch(int watchId)
 {
     m_watchers.remove(watchId);
@@ -186,7 +202,6 @@ void Geolocation::setIsAllowed(bool allowed)
     m_allowGeolocation = allowed ? Yes : No;
     
     if (isAllowed()) {
-        startTimers();
         makeSuccessCallbacks();
     } else {
         WTF::RefPtr<WebCore::PositionError> error = PositionError::create(PositionError::PERMISSION_DENIED, permissionDeniedErrorMessage);
@@ -233,7 +248,6 @@ void Geolocation::sendPositionToOneShots(Geoposition* position)
         RefPtr<GeoNotifier> notifier = *it;
         ASSERT(notifier->m_successCallback);
         
-        notifier->m_timer.stop();
         notifier->m_successCallback->handleEvent(position);
     }
 }
@@ -248,40 +262,39 @@ void Geolocation::sendPositionToWatchers(Geoposition* position)
         RefPtr<GeoNotifier> notifier = *it;
         ASSERT(notifier->m_successCallback);
         
-        notifier->m_timer.stop();
         notifier->m_successCallback->handleEvent(position);
     }
 }
 
-void Geolocation::startTimer(Vector<RefPtr<GeoNotifier> >& notifiers)
+void Geolocation::stopTimer(Vector<RefPtr<GeoNotifier> >& notifiers)
 {
     Vector<RefPtr<GeoNotifier> >::const_iterator end = notifiers.end();
     for (Vector<RefPtr<GeoNotifier> >::const_iterator it = notifiers.begin(); it != end; ++it) {
         RefPtr<GeoNotifier> notifier = *it;
-        notifier->startTimer();
+        notifier->m_timer.stop();
     }
 }
 
-void Geolocation::startTimersForOneShots()
+void Geolocation::stopTimersForOneShots()
 {
     Vector<RefPtr<GeoNotifier> > copy;
     copyToVector(m_oneShots, copy);
     
-    startTimer(copy);
+    stopTimer(copy);
 }
 
-void Geolocation::startTimersForWatchers()
+void Geolocation::stopTimersForWatchers()
 {
     Vector<RefPtr<GeoNotifier> > copy;
     copyValuesToVector(m_watchers, copy);
     
-    startTimer(copy);
+    stopTimer(copy);
 }
 
-void Geolocation::startTimers()
+void Geolocation::stopTimers()
 {
-    startTimersForOneShots();
-    startTimersForWatchers();
+    stopTimersForOneShots();
+    stopTimersForWatchers();
 }
 
 void Geolocation::handleError(PositionError* error)
@@ -319,6 +332,9 @@ void Geolocation::geolocationServicePositionChanged(GeolocationService*)
 {
     ASSERT(m_service->lastPosition());
 
+    // Stop all currently running timers.
+    stopTimers();
+
     if (!isAllowed()) {
         // requestPermission() will ask the chrome for permission. This may be
         // implemented synchronously or asynchronously. In both cases,
diff --git a/WebCore/page/Geolocation.h b/WebCore/page/Geolocation.h
index 547d284..70a8196 100644
--- a/WebCore/page/Geolocation.h
+++ b/WebCore/page/Geolocation.h
@@ -78,7 +78,7 @@ private:
         static PassRefPtr<GeoNotifier> create(Geolocation* geolocation, PassRefPtr<PositionCallback> positionCallback, PassRefPtr<PositionErrorCallback> positionErrorCallback, PassRefPtr<PositionOptions> options) { return adoptRef(new GeoNotifier(geolocation, positionCallback, positionErrorCallback, options)); }
         
         void setFatalError(PassRefPtr<PositionError> error);
-        void startTimer();
+        void startTimerIfNeeded();
         void timerFired(Timer<GeoNotifier>*);
         
         Geolocation* m_geolocation;
@@ -99,10 +99,10 @@ private:
     void sendPositionToOneShots(Geoposition*);
     void sendPositionToWatchers(Geoposition*);
     
-    static void startTimer(Vector<RefPtr<GeoNotifier> >&);
-    void startTimersForOneShots();
-    void startTimersForWatchers();
-    void startTimers();
+    static void stopTimer(Vector<RefPtr<GeoNotifier> >&);
+    void stopTimersForOneShots();
+    void stopTimersForWatchers();
+    void stopTimers();
     
     void makeSuccessCallbacks();
     void handleError(PositionError*);
@@ -114,6 +114,7 @@ private:
     virtual void geolocationServiceErrorOccurred(GeolocationService*);
 
     void fatalErrorOccurred(GeoNotifier* notifier);
+    void requestTimedOut(GeoNotifier* notifier);
 
     typedef HashSet<RefPtr<GeoNotifier> > GeoNotifierSet;
     typedef HashMap<int, RefPtr<GeoNotifier> > GeoNotifierMap;