Bug 50826

Created attachment 76221 [details] Patch

LGTM

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

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

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

Created attachment 76773 [details] Patch

Comment on attachment 76773 [details] Patch r=me

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 <yurys@chromium.org> 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

Created attachment 76805 [details] Patch

Comment on attachment 76805 [details] Patch rebased

Comment on attachment 76805 [details] Patch Clearing flags on attachment: 76805 Committed r74226: <http://trac.webkit.org/changeset/74226>

All reviewed patches have been landed. Closing bug.

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

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

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 }

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

> 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