[webkit-reviews] review canceled: [Bug 34084] Geolocation service does not cache position information between browser sessions : [Attachment 47923] Patch

[bugzilla-daemon at webkit.org]

Ariya Hidayat <ariya.hidayat at gmail.com> has canceled Steve Block
<steveblock at google.com>'s request for review:
Bug 34084: Geolocation service does not cache position information between
browser sessions
https://bugs.webkit.org/show_bug.cgi?id=34084

Attachment 47923: Patch
https://bugs.webkit.org/attachment.cgi?id=47923&action=review

------- Additional Comments from Ariya Hidayat <ariya.hidayat at gmail.com>
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?