RESOLVED FIXED 65289
Remove GeolocationPositionCache
https://bugs.webkit.org/show_bug.cgi?id=65289
Summary Remove GeolocationPositionCache
David Kilzer (:ddkilzer)
Reported Wednesday, July 27, 2011 11:13:31 PM UTC
Created attachment 102191 [details] Patch Reviewed by NOBODY (OOPS!). * page/Geolocation.h: Update copyright. Include what we use. (WebCore::Geolocation::PositionCacheWrapper): Add separate implementation of PositionCacheWrapper that does not use GeolocationPositionCache. Although GeolocationPositionCache effectively does nothing until setDatabasePath() is called, there's no need to compile the code if it's not being used. * page/GeolocationPositionCache.cpp: Update macro guard. --- 3 files changed, 37 insertions(+), 4 deletions(-)
Attachments
Patch (3.78 KB, patch)
2011-07-27 15:13 PDT, David Kilzer (:ddkilzer)
darin: review+
Remove GeolocationPositionCache (29.24 KB, patch)
2011-07-27 21:29 PDT, David Kilzer (:ddkilzer)
abarth: review+
WebKit Review Bot
Comment 1 Wednesday, July 27, 2011 11:16:24 PM UTC
Attachment 102191 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Darin Adler
Comment 2 Wednesday, July 27, 2011 11:25:01 PM UTC
Comment on attachment 102191 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102191&action=review > Source/WebCore/page/Geolocation.h:42 > +#include <wtf/HashMap.h> > +#include <wtf/HashSet.h> > +#include <wtf/PassRefPtr.h> > +#include <wtf/RefCounted.h> > +#include <wtf/RefPtr.h> > +#include <wtf/Vector.h> This seems like too many new includes. Why do we need to include all of these? I can see why we’d need to Geoposition.h and maybe RefPtr.h, but not all those others.
David Kilzer (:ddkilzer)
Comment 3 Wednesday, July 27, 2011 11:32:53 PM UTC
(In reply to comment #2) > (From update of attachment 102191 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=102191&action=review > > > Source/WebCore/page/Geolocation.h:42 > > +#include <wtf/HashMap.h> > > +#include <wtf/HashSet.h> > > +#include <wtf/PassRefPtr.h> > > +#include <wtf/RefCounted.h> > > +#include <wtf/RefPtr.h> > > +#include <wtf/Vector.h> > > This seems like too many new includes. Why do we need to include all of these? I can see why we’d need to Geoposition.h and maybe RefPtr.h, but not all those others. Each of those types is used in definition of the Geolocation object. In an experimental patch (where I commented out most of GeolocationPositionCache.h), I had to go back and add these headers because they were no longer being included for us. Hence, I'm "including what we use" in case other headers change later. (I can remove these headers, but there are implicit dependencies on those same headers being included elsewhere.) Maybe we should include these common wtf headers in config.h? (That would be a separate bug, though.)
Darin Adler
Comment 4 Thursday, July 28, 2011 12:57:01 AM UTC
Comment on attachment 102191 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102191&action=review >>> Source/WebCore/page/Geolocation.h:42 >>> +#include <wtf/Vector.h> >> >> This seems like too many new includes. Why do we need to include all of these? I can see why we’d need to Geoposition.h and maybe RefPtr.h, but not all those others. > > Each of those types is used in definition of the Geolocation object. > > In an experimental patch (where I commented out most of GeolocationPositionCache.h), I had to go back and add these headers because they were no longer being included for us. > > Hence, I'm "including what we use" in case other headers change later. (I can remove these headers, but there are implicit dependencies on those same headers being included elsewhere.) > > Maybe we should include these common wtf headers in config.h? (That would be a separate bug, though.) Thanks. Good explanation. We do not do the “including what we use in case other headers change later” technique elsewhere in the project, so I suggest not doing it here either.
Sam Weinig
Comment 5 Thursday, July 28, 2011 2:35:45 AM UTC
Given that there are no Android build files anymore, can we just rip out the parts that do write to disk? I don't think it is a good feature for WebCore to have.
Adam Barth
Comment 6 Thursday, July 28, 2011 3:10:29 AM UTC
> Given that there are no Android build files anymore, can we just rip out the parts that do write to disk? I don't think it is a good feature for WebCore to have. WebCore is writing to disk? I agree that sounds like a misfeature.
David Kilzer (:ddkilzer)
Comment 7 Thursday, July 28, 2011 4:27:47 AM UTC
(In reply to comment #6) > > Given that there are no Android build files anymore, can we just rip out the parts that do write to disk? I don't think it is a good feature for WebCore to have. > > WebCore is writing to disk? I agree that sounds like a misfeature. To be crystal clear, you're talking about removing GeolocationPositionCache, correct?
David Kilzer (:ddkilzer)
Comment 8 Thursday, July 28, 2011 5:29:35 AM UTC
Created attachment 102223 [details] Remove GeolocationPositionCache
Steve Block
Comment 9 Thursday, July 28, 2011 11:17:54 AM UTC
LGTM. Thanks for CC'ing me.
David Kilzer (:ddkilzer)
Comment 10 Thursday, July 28, 2011 3:55:27 PM UTC
(In reply to comment #9) > LGTM. Thanks for CC'ing me. Did you see the second patch? You're okay with removing GeolocationPositionCache?
Steve Block
Comment 11 Thursday, July 28, 2011 4:08:01 PM UTC
> Did you see the second patch? You're okay with removing GeolocationPositionCache? Yes, sure. I think it has value, but there obviously problems with it and I don't have time to address them now. I might look at reinstating it in the future.
David Kilzer (:ddkilzer)
Comment 12 Thursday, July 28, 2011 5:58:16 PM UTC
(In reply to comment #11) > > Did you see the second patch? You're okay with removing GeolocationPositionCache? > Yes, sure. I think it has value, but there obviously problems with it and I don't have time to address them now. I might look at reinstating it in the future. Okay, thanks!
David Kilzer (:ddkilzer)
Comment 13 Thursday, July 28, 2011 5:58:54 PM UTC
Note You need to log in before you can comment on or make changes to this bug.