[Webkit-unassigned] [Bug 50826] GeolocationPositionCache needs refactoring

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=50826





--- Comment #3 from Jeremy Orlow <jorlow at chromium.org>  2010-12-16 07:12:50 PST ---
(From update of attachment 76221)
View in context: https://bugs.webkit.org/attachment.cgi?id=76221&action=review

overall, things are looking better

> WebCore/page/GeolocationPositionCache.cpp:44
> +    static GeolocationPositionCache* instance = 0;

I believe DEFINE_STATIC_LOCAL is preferred.

> WebCore/page/GeolocationPositionCache.cpp:46
> +        instance = new GeolocationPositionCache();

What's the webkit policy on leaking things like this?  I know we do it in some places, but I imagine it's kind of frowned upon.

I wonder if this class should instead be ref counted, be saved to some static member variable as a pointer, and then have it's destructor zero out the pointer in addition to writing to disk.  The downside is that if you then go back to a site that wants to use the cached value, it'll be a bit slower.  The upside is that it'd be cleaner in that it won't leak or require manual (and thus error prone) ref counting.

> WebCore/page/GeolocationPositionCache.cpp:87
> +    return m_cachedPosition.get();

The calling code handles null, right?

> WebCore/page/GeolocationPositionCache.cpp:90
> +void GeolocationPositionCache::readFromDatabase()

This will block the main thread, right?  That seems bad.  Can it be avoided?

> WebCore/page/GeolocationPositionCache.h:38
> +// Maintains a cached position for Geolocation.

Not sure this is necessary.

> WebCore/page/GeolocationPositionCache.h:61
> +class GeolocationPositionCacheWrapper {

This should be in its own file or a sub class

> WebCore/page/GeolocationPositionCache.h:66
> +        ASSERT(m_cache);

no need

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.