Geolocation service does not cache position information between browser sessions

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Steve Block]

Android Geolocation service does not cache position information between browser sessions.

This is required to fully implement the Geolocation maximumAge property.
See https://bugs.webkit.org/show_bug.cgi?id=30676

[Steve Block]

This problem is not specific to Android

[Steve Block]

Created attachment 47923 [details]
Patch

[Steve Block]

See Bug 30676 for the patch to implement the maximumAge property, which makes use of the position cache added in this bug.

[Steve Block]

ping?

[Kenneth Rohde Christiansen]

Comment on attachment 47923 [details]
Patch

Have you thought about some way of testing this?

[Steve Block]

> Have you thought about some way of testing this?
Yes, it will be tested through the Geolocation API using the maximumAge parameter. It's the maximumAge parameter that requires this change. See Bug 30676, which this bug blocks.

[Ariya Hidayat]

Comment on attachment 47923 [details]
Patch

I'll add more remarks as I am trying to understand the whole Geolocation stuff, but meanwhile..

> +        * Android.mk: Modified. Added GeolocationPositionCache.cpp
> +        * GNUmakefile.am: Modified. Added GeolocationPositionCache.[cpp|h]
> +        * WebCore.gypi: Modified. Added GeolocationPositionCache.[cpp|h]
> +        * WebCore.xcodeproj/project.pbxproj: Modified. Added GeolocationPositionCache.[cpp|h]

Even if Qt port does not fully support Geolocation yet, shouldn't you add GeolocationPositionCache.[cpp|h] to WebCore/WebCore.pro?

> +static const char* databaseName = "/CachedPosition.db";

Is there a standard name for this? Or can we just pick a sensible one?

> +void GeolocationPositionCache::setCachedPosition(Geoposition* cachedPosition)
> +{
> +    // We do not take owenership from the caller, but add our own ref count.
> +    *s_cachedPosition = cachedPosition;
> +}

Small typo there. Also. maybe mention why the ownership is not taken (following the usual practice of commenting why instead of what the next line does).

> +PassRefPtr<Geoposition> GeolocationPositionCache::readFromDB()
> +{
> +    SQLiteDatabase database;
> +    if (!s_databaseFile || !database.open(*s_databaseFile))
> +        return 0;

Also for other returns in this function: is it safe to return 0 here instead of "default" position (if default makes sense at all)?

> +void GeolocationPositionCache::writeToDB(Geoposition* position)

Can't the parameter be a const?

[Steve Block]

Thanks for the comments

> Even if Qt port does not fully support Geolocation yet, shouldn't you add
> GeolocationPositionCache.[cpp|h] to WebCore/WebCore.pro?
Will fix

> > +static const char* databaseName = "/CachedPosition.db";
> 
> Is there a standard name for this? Or can we just pick a sensible one?
I don't know of a standard name, I think this seems reasonable. Looking at the database API code, the precedent is to use a hardcoded database name and a platform-configurable path, as here.

> Small typo there. Also. maybe mention why the ownership is not taken (following
> the usual practice of commenting why instead of what the next line does).
I think the comment is probably redundant, will remove.

> Also for other returns in this function: is it safe to return 0 here instead of
> "default" position (if default makes sense at all)?
Yes, it's safe to return 0 - this is handled by the Geolocation object. There's no concept of a default position.

> > +void GeolocationPositionCache::writeToDB(Geoposition* position)
> 
> Can't the parameter be a const?
Will fix

[Steve Block]

Created attachment 48896 [details]
Patch

[Ariya Hidayat]

> > > +static const char* databaseName = "/CachedPosition.db";
> > 
> > Is there a standard name for this? Or can we just pick a sensible one?
> I don't know of a standard name, I think this seems reasonable. Looking at the
> database API code, the precedent is to use a hardcoded database name and a
> platform-configurable path, as here.

Sounds sensible. BTW, what about CachedGeoposition as the name (to signify the "Geo").

> Yes, it's safe to return 0 - this is handled by the Geolocation object. There's
> no concept of a default position.

Acknowledged.

[Steve Block]

> Sounds sensible. BTW, what about CachedGeoposition as the name (to signify the
> "Geo").
Sure, that makes sense. I can fix that when landing if you have no other comments.

[Steve Block]

ping!

[Steve Block]

Landed manually as http://trac.webkit.org/changeset/55142

Closing bug as resolved fixed.