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
This problem is not specific to Android
Created attachment 47923 [details] Patch
See Bug 30676 for the patch to implement the maximumAge property, which makes use of the position cache added in this bug.
ping?
Comment on attachment 47923 [details] Patch Have you thought about some way of testing this?
> 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.
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?
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
Created attachment 48896 [details] Patch
> > > +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.
> 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.
ping!
Landed manually as http://trac.webkit.org/changeset/55142 Closing bug as resolved fixed.