[Webkit-unassigned] [Bug 30676] Geolocation maximumAge property is not applied

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=30676





--- Comment #8 from Steve Block <steveblock at google.com>  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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.