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
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
: 30676
  Show dependency treegraph
 
Reported: 2010-01-25 06:24 PST by
Modified: 2010-02-23 04:08 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2010-02-02 03:25:32 PST -------
This problem is not specific to Android
------- Comment #2 From 2010-02-02 04:52:19 PST -------
Created an attachment (id=47923) [details]
Patch
------- Comment #3 From 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 From 2010-02-11 08:30:26 PST -------
ping?
------- Comment #5 From 2010-02-11 09:51:30 PST -------
(From update of attachment 47923 [details])
Have you thought about some way of testing this?
------- Comment #6 From 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 From 2010-02-11 14:59:57 PST -------
(From update of attachment 47923 [details])
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 From 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 From 2010-02-17 05:09:13 PST -------
Created an attachment (id=48896) [details]
Patch
------- Comment #10 From 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 From 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 From 2010-02-22 13:03:43 PST -------
ping!
------- Comment #13 From 2010-02-23 04:08:37 PST -------
Landed manually as http://trac.webkit.org/changeset/55142

Closing bug as resolved fixed.