Bug 50826 – GeolocationPositionCache needs refactoring

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Steve Block]

Currently, Geolocation position cache uses numerous static members. Refactoring to use a single instance of the class will make the code cleaner and make it simpler to move database access to a background thread in Bug 50825.

[Steve Block]

Created attachment 76221 [details]
Patch

[Andrei Popescu]

LGTM

[Jeremy Orlow]

Comment on attachment 76221 [details]
Patch

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

[Steve Block]

Comment on attachment 76221 [details]
Patch

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

[Jeremy Orlow]

k.  give me a new patch and i'll LGTM it

[Steve Block]

Created attachment 76773 [details]
Patch

[Jeremy Orlow]

Comment on attachment 76773 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 76773 [details]
Patch

Rejecting attachment 76773 [details] from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'apply-attachment', '--non-interactive', 76773]" exit_code: 2
Last 500 characters of output:
ailed to merge in the changes.
Patch failed at 0001 2010-12-16  Yury Semikhatsky  <[e-mail]>

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at WebKitTools/Scripts/update-webkit line 132.

Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2

Full output: http://queues.webkit.org/results/7179078

[Steve Block]

Created attachment 76805 [details]
Patch

[Steve Block]

Comment on attachment 76805 [details]
Patch

rebased

[WebKit Commit Bot]

Comment on attachment 76805 [details]
Patch

Clearing flags on attachment: 76805

Committed r74226: <http://trac.webkit.org/changeset/74226>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ariya Hidayat]

The name of the reviewer in the commit http://trac.webkit.org/changeset/74226 is wrong.

[Antonio Gomes]

All Qt bots (windows, minimal, arm) but linux 32bits do not seem to build after this patch:

(...)
/Qt-4.7.1/lib -L/usr/X11R6/lib -lQtNetwork -lQtCore -lpthread  
obj/release/Geolocation.o: In function `WebCore::Geolocation::~Geolocation()':
Geolocation.cpp:(.text._ZN7WebCore11GeolocationD1Ev+0x30): undefined reference to `WebCore::GeolocationPositionCache::removeUser()'
Geolocation.cpp:(.text._ZN7WebCore11GeolocationD1Ev+0x11c): undefined reference to `WebCore::GeolocationPositionCache::removeUser()'
collect2: ld returned 1 exit status
make[1]: *** [../lib/libQtWebKit.so.4.9.0] Error 1
make[1]: Leaving directory `/home/webkitbuildbot/slaves/release32bitMinimal/buildslave/qt-linux-release-minimal/build/WebKitBuild/Release/WebCore'
make: *** [sub-WebCore-make_default-ordered] Error 2

[Jake]

To get this to build locally on windows/Qt I added these #ifdefs in Geolocation.h:


    class PositionCacheWrapper {
    public:
        PositionCacheWrapper()
            : m_cache
#if ENABLE(GEOLOCATION)
			(GeolocationPositionCache::instance())
#else
			(0)
#endif
        {
#if ENABLE(GEOLOCATION)
            m_cache->addUser();
#endif
        }
        ~PositionCacheWrapper()
        {
#if ENABLE(GEOLOCATION)
			m_cache->removeUser();
#endif
        }

[Carlos Garcia Campos]

It fails to build gtk port without geolocation support too, one #ifdef was enough for me to fix it:

+#if ENABLE(GEOLOCATION)
     PositionCacheWrapper m_positionCache;
+#endif

[Steve Block]

> The name of the reviewer in the commit http://trac.webkit.org/changeset/74226
> is wrong.
Aargh. I knew I wouldn't be around to watch the build, so I wanted to commit via the commit-queue to make sure that I landed exactly the patch that had passed the try-bots. I hoped that the commit queue would be smart enough to set jorlow as reviewer. Would it have helped if I'd set him as reviewer in my patch, or would the commit-queue have overridden it anyway?

> All Qt bots (windows, minimal, arm) but linux 32bits do not seem to build after
> this patch:
Apologies. Should be fixed with http://trac.webkit.org/changeset/74240