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

[bugzilla-daemon at webkit.org]

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





--- Comment #4 from Steve Block <steveblock at google.com>  2010-12-16 08:27:44 PST ---
(From update of attachment 76221)
View in context: https://bugs.webkit.org/attachment.cgi?id=76221&action=review

>> WebCore/page/GeolocationPositionCache.cpp:44
>> +    static GeolocationPositionCache* instance = 0;
> 
> I believe DEFINE_STATIC_LOCAL is preferred.

Will do

>> 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.

I think we leak plenty of small static objects like this. Personally I think that it's worth it to keep the cached position in memory.

>> WebCore/page/GeolocationPositionCache.cpp:87
>> +    return m_cachedPosition.get();
> 
> The calling code handles null, right?

Correct

>> WebCore/page/GeolocationPositionCache.cpp:90
>> +void GeolocationPositionCache::readFromDatabase()
> 
> This will block the main thread, right?  That seems bad.  Can it be avoided?

Yes, this will be done in Bug 50825, which is the motivation for this refactoring.

>> WebCore/page/GeolocationPositionCache.h:38
>> +// Maintains a cached position for Geolocation.
> 
> Not sure this is necessary.

Sure

>> WebCore/page/GeolocationPositionCache.h:61
>> +class GeolocationPositionCacheWrapper {
> 
> This should be in its own file or a sub class

Will do

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

OK

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