Bug 34084 - Geolocation service does not cache position information between browser sessions
: Geolocation service does not cache position information between browser sessions
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebCore Misc.
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks: 30676
  Show dependency treegraph
 
Reported: 2010-01-25 06:24 PST by Steve Block
Modified: 2010-02-23 04:08 PST (History)
5 users (show)

See Also:


Attachments
Patch (16.23 KB, patch)
2010-02-02 04:52 PST, Steve Block
no flags Details | Formatted Diff | Diff
Patch (18.60 KB, patch)
2010-02-17 05:09 PST, Steve Block
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Steve Block 2010-01-25 06:24:46 PST
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
Comment 1 Steve Block 2010-02-02 03:25:32 PST
This problem is not specific to Android
Comment 2 Steve Block 2010-02-02 04:52:19 PST
Created attachment 47923 [details]
Patch
Comment 3 Steve Block 2010-02-02 08:57:27 PST
See Bug 30676 for the patch to implement the maximumAge property, which makes use of the position cache added in this bug.
Comment 4 Steve Block 2010-02-11 08:30:26 PST
ping?
Comment 5 Kenneth Rohde Christiansen 2010-02-11 09:51:30 PST
Comment on attachment 47923 [details]
Patch

Have you thought about some way of testing this?
Comment 6 Steve Block 2010-02-11 11:00:56 PST
> 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 7 Ariya Hidayat 2010-02-11 14:59:57 PST
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?
Comment 8 Steve Block 2010-02-17 05:06:20 PST
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
Comment 9 Steve Block 2010-02-17 05:09:13 PST
Created attachment 48896 [details]
Patch
Comment 10 Ariya Hidayat 2010-02-17 11:55:47 PST
> > > +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.
Comment 11 Steve Block 2010-02-17 15:24:56 PST
> 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.
Comment 12 Steve Block 2010-02-22 13:03:43 PST
ping!
Comment 13 Steve Block 2010-02-23 04:08:37 PST
Landed manually as http://trac.webkit.org/changeset/55142

Closing bug as resolved fixed.