WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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+
Details
Formatted Diff
Diff
Remove GeolocationPositionCache
(29.24 KB, patch)
2011-07-27 21:29 PDT
,
David Kilzer (:ddkilzer)
abarth
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
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
Committed
r91936
: <
http://trac.webkit.org/changeset/91936
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug