[webkit-reviews] review granted: [Bug 65289] Disable GeolocationPositionCache on Apple ports : [Attachment 102191] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted David Kilzer (ddkilzer)
<ddkilzer at webkit.org>'s request for review:
Bug 65289: Disable GeolocationPositionCache on Apple ports
https://bugs.webkit.org/show_bug.cgi?id=65289

Attachment 102191: Patch
https://bugs.webkit.org/attachment.cgi?id=102191&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.