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.

1 files changed, 34 insertions, 18 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,