Bug 30676 - Geolocation maximumAge property is not applied
Summary: Geolocation maximumAge property is not applied
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 34084
Blocks: 65289
  Show dependency treegraph
 
Reported: 2009-10-22 08:49 PDT by Steve Block
Modified: 2011-07-28 07:59 PDT (History)
7 users (show)

See Also:


Attachments
Patch (11.36 KB, patch)
2010-02-02 08:51 PST, Steve Block
no flags Details | Formatted Diff | Diff
Patch (18.72 KB, patch)
2010-02-24 03:27 PST, Steve Block
no flags Details | Formatted Diff | Diff
Patch (23.00 KB, patch)
2010-02-26 10:03 PST, Steve Block
no flags Details | Formatted Diff | Diff
Patch (18.96 KB, patch)
2010-02-26 10:05 PST, Steve Block
no flags Details | Formatted Diff | Diff
Patch (19.27 KB, patch)
2010-03-10 12:01 PST, Steve Block
jorlow: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Steve Block 2009-10-22 08:49:09 PDT
The Geolocation API does not apply the PositionOptions.maximumAge property. See http://www.w3.org/TR/geolocation-API/#max-age.
Comment 1 Steve Block 2010-02-02 08:51:15 PST
Created attachment 47938 [details]
Patch
Comment 2 Steve Block 2010-02-02 08:55:34 PST
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
Comment 3 Steve Block 2010-02-24 03:27:59 PST
Created attachment 49374 [details]
Patch
Comment 4 Steve Block 2010-02-24 03:28:57 PST
(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.
Comment 5 WebKit Review Bot 2010-02-24 03:38:11 PST
Attachment 49374 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/304352
Comment 6 Steve Block 2010-02-24 05:08:10 PST
(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.
Comment 7 Kenneth Rohde Christiansen 2010-02-26 04:53:56 PST
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?
Comment 8 Steve Block 2010-02-26 09:48:53 PST
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.
Comment 9 Steve Block 2010-02-26 10:03:13 PST
Created attachment 49595 [details]
Patch
Comment 10 Steve Block 2010-02-26 10:05:23 PST
Created attachment 49596 [details]
Patch
Comment 11 Steve Block 2010-02-26 10:06:53 PST
(In reply to comment #10)
> Created an attachment (id=49596) [details]
> Patch
Minor update to use a string constant for a duplicated error message
Comment 12 WebKit Review Bot 2010-02-26 10:22:30 PST
Attachment 49596 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/313627
Comment 13 Steve Block 2010-03-09 16:43:45 PST
Ping!
Comment 14 Jeremy Orlow 2010-03-10 10:12:06 PST
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();
> +    });
> +}
Comment 15 Steve Block 2010-03-10 12:00:08 PST
> 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.
Comment 16 Steve Block 2010-03-10 12:01:01 PST
Created attachment 50420 [details]
Patch
Comment 17 Jeremy Orlow 2010-03-11 03:12:35 PST
Comment on attachment 50420 [details]
Patch

r=me
Comment 18 Steve Block 2010-03-11 03:39:21 PST
Committed r55841: <http://trac.webkit.org/changeset/55841>