Bug 30676 – Geolocation maximumAge property is not applied

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Steve Block]

The Geolocation API does not apply the PositionOptions.maximumAge property. See http://www.w3.org/TR/geolocation-API/#max-age.

[Steve Block]

Created attachment 47938 [details]
Patch

[Steve Block]

Note that this patch is incomplete, as it requires the Geolocation position cache. It is intended to provide context for Bug 34084, which adds the position cache and which blocks this bug.

The patch also lacks layout tests, which sill be added with the final patch

[Steve Block]

Created attachment 49374 [details]
Patch

[Steve Block]

(In reply to comment #3)
> Created an attachment (id=49374) [details]
> Patch
Now that Bug 34084 is fixed, this patch is complete, including LayoutTests, and ready for review.

[WebKit Review Bot]

Attachment 49374 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/304352

[Steve Block]

(In reply to comment #5)
> Attachment 49374 [details] did not build on chromium:
> Build output: http://webkit-commit-queue.appspot.com/results/304352
The Chromium build failure is a linker error because GeolocationPositionCache is missing. So either I've messed up adding GeolocationPositionCache.cpp to the Chromium build or the build-bot needs to make clean. Either way, I think the code is good.

[Kenneth Rohde Christiansen]

Comment on attachment 49374 [details]
Patch


> +void Geolocation::GeoNotifier::makeSuccessCallback(Geoposition* position)
> +{
> +    m_successCallback->handleEvent(position);
> +}

Why do you call these callbacks, when you are not really using them as callbacks? Because they are called async or so? or because you use them as notifiers?


> +                notifier->setFatalError(PositionError::create(PositionError::POSITION_UNAVAILABLE, "Failed to start Geolocation service"));

I think WebKit has a class to put strings that are supposed to be translated, though I have never used that.


> +void Geolocation::makeCachedPositionCallbacks()
> +{

I got a bit confused with your make callback terminology. So appearently you have callbacks and you add things for it to handle?

judging from 

void Geolocation::GeoNotifier::makeSuccessCallback(Geoposition* position)
{
    m_successCallback->handleEvent(position);
}

So is this some kind of "handleEventAsSuccess" ?


> +bool Geolocation::haveSuitableCachedPosition(PositionOptions* options)
> +{
> +    if (!m_positionCache->cachedPosition())
> +        return false;
> +    if (!options->hasMaximumAge())
> +        return true;
> +    if (!options->maximumAge())
> +        return false;

It is a bit confusing that hasMaximumAge is false for 0 (which I assume if this is compliant with the spec), but you still have the 0 in the maximumAge, which you check afterward.

>  void Geolocation::setIsAllowed(bool allowed)

setAllowed should be OK I guess.

>      m_allowGeolocation = allowed ? Yes : No;

Why Yes, No and not true false?

[Steve Block]

Thanks for the review Kenneth

> > +void Geolocation::GeoNotifier::makeSuccessCallback(Geoposition* position)
> > +{
> > +    m_successCallback->handleEvent(position);
> > +}
> 
> Why do you call these callbacks, when you are not really using them as
> callbacks? Because they are called async or so? or because you use them as
> notifiers?
m_successCallback is a pre-existing member of GeoNotifier, not added in this patch. It's the callback to JavaScript.

> > +                notifier->setFatalError(PositionError::create(PositionError::POSITION_UNAVAILABLE, "Failed to start Geolocation service"));
> 
> I think WebKit has a class to put strings that are supposed to be translated,
> though I have never used that.
OK, good to know, I'll look into it. But this isn't introduced by this patch, so probably shouldn't be fixed here.

> > +void Geolocation::makeCachedPositionCallbacks()
> > +{
> 
> I got a bit confused with your make callback terminology. So appearently you
> have callbacks and you add things for it to handle?
No, this is the method that invokes the success callback to JavaScript for all requests that should use a cached position but have been waiting for permissions to be granted.

> > +bool Geolocation::haveSuitableCachedPosition(PositionOptions* options)
> > +{
> > +    if (!m_positionCache->cachedPosition())
> > +        return false;
> > +    if (!options->hasMaximumAge())
> > +        return true;
> > +    if (!options->maximumAge())
> > +        return false;
> 
> It is a bit confusing that hasMaximumAge is false for 0 (which I assume if this
> is compliant with the spec), but you still have the 0 in the maximumAge, which
> you check afterward.
No, hasMaximumAge() returns a boolean which represents whether a maximum age is set. If it hasn't been set, we don't apply a maximum age, in which case any cached position is recent enough and we return true (ie we behave as though the maximum age were infinite). Zero is a valid value for the maximum age to be set to, in which case no cached position is recent enough and we return false. See https://bugs.webkit.org/show_bug.cgi?id=27254#c6 and https://bugs.webkit.org/show_bug.cgi?id=29099

FWIW, I think the third if would read much better as  ...

if (options->maximumAge() == 0)

to make clear it's a numerical equivalence, not a logical condition, but this doesn't meet WebKit style.

> >  void Geolocation::setIsAllowed(bool allowed)
> 
> setAllowed should be OK I guess.
> 
> >      m_allowGeolocation = allowed ? Yes : No;
> 
> Why Yes, No and not true false?
I think this is fine as it is, and it's not changed by this patch.

[Steve Block]

Created attachment 49595 [details]
Patch

[Steve Block]

Created attachment 49596 [details]
Patch

[Steve Block]

(In reply to comment #10)
> Created an attachment (id=49596) [details]
> Patch
Minor update to use a string constant for a duplicated error message

[WebKit Review Bot]

Attachment 49596 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/313627

[Steve Block]

Ping!

[Jeremy Orlow]

Comment on attachment 49596 [details]
Patch


> Index: WebCore/page/Geolocation.cpp
> ===================================================================
> --- WebCore/page/Geolocation.cpp	(revision 55163)
> +++ WebCore/page/Geolocation.cpp	(working copy)

> +void Geolocation::GeoNotifier::makeSuccessCallback(Geoposition* position)

Make seems like a confusing word.  run, exec, execute all seem more clear.

> +{
> +    m_successCallback->handleEvent(position);
> +}
> +
>  void Geolocation::GeoNotifier::startTimerIfNeeded()
>  {
>      if (m_options->hasTimeout())
> @@ -122,6 +136,14 @@ void Geolocation::GeoNotifier::timerFire
>          return;
>      }
>  
> +    if (m_useCachedPosition) {
> +        // Clear the cached position flag in case this is a watch request, which
> +        // will continue to run.
> +        m_useCachedPosition = false;
> +        m_geolocation->requestUsesCachedPosition(this);
> +        return;
> +    }
> +
>      if (m_errorCallback) {
>          RefPtr<PositionError> error = PositionError::create(PositionError::TIMEOUT, "Timeout expired");
>          m_errorCallback->handleEvent(error.get());
> @@ -155,6 +177,11 @@ void Geolocation::Watchers::remove(GeoNo
>      m_notifierToIdMap.remove(iter);
>  }
>  
> +bool Geolocation::Watchers::contains(GeoNotifier* notifier) const
> +{
> +    return m_notifierToIdMap.contains(notifier);
> +}
> +
>  void Geolocation::Watchers::clear()
>  {
>      m_idToNotifierMap.clear();
> @@ -178,6 +205,7 @@ Geolocation::Geolocation(Frame* frame)
>  #endif
>      , m_allowGeolocation(Unknown)
>      , m_shouldClearCache(false)
> +    , m_positionCache(new GeolocationPositionCache)
>  {
>      if (!m_frame)
>          return;
> @@ -249,14 +277,18 @@ PassRefPtr<Geolocation::GeoNotifier> Geo
>      if (isDenied())
>          notifier->setFatalError(PositionError::create(PositionError::PERMISSION_DENIED, permissionDeniedErrorMessage));
>      else {

Why isn't this just an "else if".  This seems like needless indentation.


> Index: LayoutTests/fast/dom/Geolocation/resources/maximum-age.js
> ===================================================================
> --- LayoutTests/fast/dom/Geolocation/resources/maximum-age.js	(revision 0)
> +++ LayoutTests/fast/dom/Geolocation/resources/maximum-age.js	(revision 0)
> +function testNonZeroMaximumAge() {
> +    // Update the mock service to report an error.
> +    window.layoutTestController.setMockGeolocationError(mockCode, mockMessage);
> +    // The maximumAge is non-zero, so we expect the cached position, not the error from the service.

How does this work, if you set the error here?  Shouldn't this happen after you get the zero age?

> +    navigator.geolocation.getCurrentPosition(function(p) {
> +        checkPosition(p);
> +        testZeroMaximumAgeError();
> +    }, function(e) {
> +        testFailed('Error callback invoked unexpectedly');
> +        window.layoutTestController.notifyDone();
> +    }, {maximumAge: 1000});
> +}
> +
> +function testZeroMaximumAgeError() {
> +    // The default maximumAge is zero, so we expect the error from the service.
> +    navigator.geolocation.getCurrentPosition(function(p) {
> +        testFailed('Success callback invoked unexpectedly');
> +        window.layoutTestController.notifyDone();
> +    }, function(e) {
> +        checkError(e);
> +        debug('<br /><span class="pass">TEST COMPLETE</span>');
> +        window.layoutTestController.notifyDone();
> +    });
> +}

[Steve Block]

> Make seems like a confusing word.  run, exec, execute all seem more clear.
Updated to use 'run'

> Why isn't this just an "else if".  This seems like needless indentation.
Good point, fixed.

> How does this work, if you set the error here?  Shouldn't this happen after you
> get the zero age?
testZeroMaximumAge(), which was called previously, tested that when maximumAge is zero, we go to the GeolocationService for the latest position. Here, we configure  the GeolocationService to report an error when it's next queried. However, we call getCurrentPosition with a non-zero maximumAge, so we don't go to the GeolocationService,  but instead return the cached position. The final test calls getCurrentPosition again with a zero maximumAge and expects to get the error from the GeolocationService.

[Steve Block]

Created attachment 50420 [details]
Patch

[Jeremy Orlow]

Comment on attachment 50420 [details]
Patch

r=me

[Steve Block]

Committed r55841: <http://trac.webkit.org/changeset/55841>