Attachment 38373 Details for Bug 27256 – Patch 2 for bug 27256

[patch] Patch 2 for bug 27256

applyTimeoutCorrectly2.txt (text/plain), 11.06 KB, created by Steve Block on 2009-08-21 08:38:45 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Steve Block

Created: 2009-08-21 08:38:45 PDT

Size: 11.06 KB

patch

obsolete

>Index: WebCore/ChangeLog
>===================================================================
>--- WebCore/ChangeLog	(revision 47630)
>+++ WebCore/ChangeLog	(working copy)
>@@ -1,3 +1,37 @@
>+2009-08-21  Steve Block  <steveblock@google.com>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Correctly applies the Geolocation timeout property.
>+        https://bugs.webkit.org/show_bug.cgi?id=27256
>+
>+        If a timeout is specified, the timer is started as soon as the location acquistion process starts.
>+        If a position fix is not obtained within the timeout interval, the position request is cancelled 
>+        and the error callback (if specified) is invoked. If a position fix is obtained within the timeout
>+        interval, the timer is stopped. Permissions are then requested from the user if required. See
>+        http://www.w3.org/TR/geolocation-API/#position-options.
>+
>+        Note that this logic means that the time required to obtain permissions from the user is not
>+        included in the timeout interval.
>+
>+        No new tests possible until a mock Geolocation service is implemented.
>+
>+        * page/Geolocation.cpp: Modified.
>+        (WebCore::Geolocation::GeoNotifier::GeoNotifier): Modified. Takes Geolocation object as parameter.
>+        (WebCore::Geolocation::GeoNotifier::startTimerIfNeeded): Renamed from startTimer. Starts the timer if a timeout was specified.
>+        (WebCore::Geolocation::GeoNotifier::timerFired): Modified. Calls the error callback if specified, and reports the timeout to the Geolocation object.
>+        (WebCore::Geolocation::getCurrentPosition): Modified. Calls maybeStartTimer when starting the location acquisition process.
>+        (WebCore::Geolocation::watchPosition): Modified. Calls maybeStartTimer when starting the location acquisition process.
>+        (WebCore::Geolocation::requestTimedOut): Added. Cancels a request when it times out.
>+        (WebCore::Geolocation::setIsAllowed): Modified. No longer starts timers.
>+        (WebCore::Geolocation::sendPosition): Modified. No longer stops timers.
>+        (WebCore::Geolocation::stopTimer): Added. Stops the timers for a vector of notifiers.
>+        (WebCore::Geolocation::stopTimersForOneShots): Added. Stops the timers for all one-shots.
>+        (WebCore::Geolocation::stopTimersForWatchers): Added. Stops the timers for all watchers.
>+        (WebCore::Geolocation::stopTimers): Added. Stops the timers for all requests.
>+        (WebCore::Geolocation::geolocationServicePositionChanged): Modfified. Stops the timers for all requests.
>+        * page/Geolocation.h: Modified.
>+
> 2009-08-21  Beth Dakin  <bdakin@apple.com>
> 
>         Reviewed by Dan Bernstein.
>Index: WebCore/page/Geolocation.cpp
>===================================================================
>--- WebCore/page/Geolocation.cpp	(revision 47630)
>+++ WebCore/page/Geolocation.cpp	(working copy)
>@@ -35,8 +35,9 @@
> 
> namespace WebCore {
> 
>-Geolocation::GeoNotifier::GeoNotifier(PassRefPtr<PositionCallback> successCallback, PassRefPtr<PositionErrorCallback> errorCallback, PassRefPtr<PositionOptions> options)
>-    : m_successCallback(successCallback)
>+Geolocation::GeoNotifier::GeoNotifier(Geolocation* geolocation, PassRefPtr<PositionCallback> successCallback, PassRefPtr<PositionErrorCallback> errorCallback, PassRefPtr<PositionOptions> options)
>+    : m_geolocation(geolocation)
>+    , m_successCallback(successCallback)
>     , m_errorCallback(errorCallback)
>     , m_options(options)
>     , m_timer(this, &Geolocation::GeoNotifier::timerFired)
>@@ -47,9 +48,9 @@ Geolocation::GeoNotifier::GeoNotifier(Pa
>     ASSERT(m_options);
> }
> 
>-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);
> }
> 
>@@ -59,8 +60,11 @@ void Geolocation::GeoNotifier::timerFire
>     
>     m_timer.stop();
> 
>-    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)
>@@ -85,9 +89,11 @@ void Geolocation::disconnectFrame()
> 
> void Geolocation::getCurrentPosition(PassRefPtr<PositionCallback> successCallback, PassRefPtr<PositionErrorCallback> errorCallback, PassRefPtr<PositionOptions> options)
> {
>-    RefPtr<GeoNotifier> notifier = GeoNotifier::create(successCallback, errorCallback, options);
>+    RefPtr<GeoNotifier> notifier = GeoNotifier::create(this, successCallback, errorCallback, options);
> 
>-    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());
>@@ -100,9 +106,11 @@ void Geolocation::getCurrentPosition(Pas
> 
> int Geolocation::watchPosition(PassRefPtr<PositionCallback> successCallback, PassRefPtr<PositionErrorCallback> errorCallback, PassRefPtr<PositionOptions> options)
> {
>-    RefPtr<GeoNotifier> notifier = GeoNotifier::create(successCallback, errorCallback, options);
>+    RefPtr<GeoNotifier> notifier = GeoNotifier::create(this, successCallback, errorCallback, options);
> 
>-    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());
>@@ -117,6 +125,15 @@ int Geolocation::watchPosition(PassRefPt
>     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);
>@@ -141,10 +158,9 @@ void Geolocation::setIsAllowed(bool allo
> {
>     m_allowGeolocation = allowed ? Yes : No;
>     
>-    if (isAllowed()) {
>-        startTimers();
>+    if (isAllowed())
>         makeSuccessCallbacks();
>-    } else {
>+    else {
>         WTF::RefPtr<WebCore::PositionError> error = WebCore::PositionError::create(PositionError::PERMISSION_DENIED, "User disallowed GeoLocation");
>         handleError(error.get());
>     }
>@@ -184,7 +200,6 @@ void Geolocation::sendPosition(Vector<Re
>         RefPtr<GeoNotifier> notifier = *it;
>         ASSERT(notifier->m_successCallback);
>         
>-        notifier->m_timer.stop();
>         bool shouldCallErrorCallback = false;
>         notifier->m_successCallback->handleEvent(position, shouldCallErrorCallback);
>         if (shouldCallErrorCallback) {
>@@ -210,35 +225,35 @@ void Geolocation::sendPositionToWatchers
>     sendPosition(copy, 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)
>@@ -276,6 +291,9 @@ void Geolocation::geolocationServicePosi
> {
>     ASSERT_UNUSED(service, service == m_service);
>     ASSERT(m_service->lastPosition());
>+
>+    // Stop all currently running timers.
>+    stopTimers();
>     
>     if (!isAllowed()) {
>         // requestPermission() will ask the chrome for permission. This may be
>Index: WebCore/page/Geolocation.h
>===================================================================
>--- WebCore/page/Geolocation.h	(revision 47630)
>+++ WebCore/page/Geolocation.h	(working copy)
>@@ -73,18 +73,19 @@ private:
> 
>     class GeoNotifier : public RefCounted<GeoNotifier> {
>     public:
>-        static PassRefPtr<GeoNotifier> create(PassRefPtr<PositionCallback> positionCallback, PassRefPtr<PositionErrorCallback> positionErrorCallback, PassRefPtr<PositionOptions> options) { return adoptRef(new GeoNotifier(positionCallback, positionErrorCallback, options)); }
>+        static PassRefPtr<GeoNotifier> create(Geolocation* geolocation, PassRefPtr<PositionCallback> positionCallback, PassRefPtr<PositionErrorCallback> positionErrorCallback, PassRefPtr<PositionOptions> options) { return adoptRef(new GeoNotifier(geolocation, positionCallback, positionErrorCallback, options)); }
>         
>-        void startTimer();
>+        void startTimerIfNeeded();
>         void timerFired(Timer<GeoNotifier>*);
>         
>+        Geolocation* m_geolocation;
>         RefPtr<PositionCallback> m_successCallback;
>         RefPtr<PositionErrorCallback> m_errorCallback;
>         RefPtr<PositionOptions> m_options;
>         Timer<GeoNotifier> m_timer;
> 
>     private:
>-        GeoNotifier(PassRefPtr<PositionCallback>, PassRefPtr<PositionErrorCallback>, PassRefPtr<PositionOptions>);
>+        GeoNotifier(Geolocation*, PassRefPtr<PositionCallback>, PassRefPtr<PositionErrorCallback>, PassRefPtr<PositionOptions>);
>     };
> 
>     bool hasListeners() const { return !m_oneShots.isEmpty() || !m_watchers.isEmpty(); }
>@@ -97,10 +98,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*);
>@@ -111,6 +112,8 @@ private:
>     virtual void geolocationServicePositionChanged(GeolocationService*);
>     virtual void geolocationServiceErrorOccurred(GeolocationService*);
> 
>+    void requestTimedOut(GeoNotifier*);
>+
>     typedef HashSet<RefPtr<GeoNotifier> > GeoNotifierSet;
>     typedef HashMap<int, RefPtr<GeoNotifier> > GeoNotifierMap;
>

Flags:

abarth: review-

Actions: View | Formatted Diff | Diff

Attachments on bug 27256: 34970 | 38373 | 39262 | 39294