Bug 65289 – Remove GeolocationPositionCache

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

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(-)

[WebKit Review Bot]

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 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)]

(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 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]

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]

> 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)]

(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)]

Created attachment 102223 [details]
Remove GeolocationPositionCache

[Steve Block]

LGTM. Thanks for CC'ing me.

[David Kilzer (:ddkilzer)]

(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]

> 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)]

(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)]

Committed r91936: <http://trac.webkit.org/changeset/91936>