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(-)
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.
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.
(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.)
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.
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.
> 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.
(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?
Created attachment 102223 [details] Remove GeolocationPositionCache
LGTM. Thanks for CC'ing me.
(In reply to comment #9) > LGTM. Thanks for CC'ing me. Did you see the second patch? You're okay with removing GeolocationPositionCache?
> 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.
(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!
Committed r91936: <http://trac.webkit.org/changeset/91936>