Bug 25756

Created attachment 30283 [details] Add GEOLOCATION guards around Geolocation code

I think Sam should review this. Geolocation is already off for various platforms; I'm not sure why this patch is needed.

(In reply to comment #2) > I think Sam should review this. Geolocation is already off for various > platforms; I'm not sure why this patch is needed. > This patch makes turning off Geolocation support more effective (by guarding files exclusive to Geolocation). This patch is not meant to fix turning off Geolocation, since it is not broken as Darin mentioned.

Comment on attachment 30283 [details] Add GEOLOCATION guards around Geolocation code This looks fine. I would expect dead-code stripping compilers to take care of this sort of thing though.

Landed in r44093. This worked on Qt and Gtk+ with geolocation enabled.

I'm backing out this change. This broke all of the Mac builds: http://build.webkit.org/builders/Tiger%20Intel%20Release/builds/1322 http://build.webkit.org/builders/Leopard%20Intel%20Release%20%28Build%29/builds/1555 http://build.webkit.org/builders/Leopard%20Intel%20Debug%20%28Build%29/builds/1548 Also the Geolocation implementation was designed such that if you don't want Geolocation, you should provide an empty implementation and not compile these source files. Please update the patch to do the above for your port rather than adding ENABLE(GEOLOCATION) macro guards everywhere. (It's possible that some ENABLE(GEOLOCATION) guards will need to be added to get the desired result, but not to entire files.) $ git svn dcommit Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/bindings/js/JSCustomPositionCallback.cpp M WebCore/bindings/js/JSCustomPositionCallback.h M WebCore/bindings/js/JSCustomPositionErrorCallback.cpp M WebCore/bindings/js/JSCustomPositionErrorCallback.h M WebCore/bindings/js/JSGeolocationCustom.cpp M WebCore/page/Geolocation.cpp M WebCore/page/Geolocation.idl M WebCore/page/Geoposition.cpp M WebCore/page/Geoposition.h M WebCore/page/Geoposition.idl M WebCore/page/Navigator.cpp M WebCore/page/PositionError.idl M WebCore/platform/GeolocationService.cpp Committed r44094 http://trac.webkit.org/changeset/44094

Comment on attachment 30283 [details] Add GEOLOCATION guards around Geolocation code r- per Comment #6

It was explicitly added without #defines so that the code would not bit rot. Antti suggested it, and I liked it. I think this shouldn't be fixed.

Per Maciej's feedback in <https://lists.webkit.org/pipermail/webkit-dev/2009-May/007850.html>, he prefers adding #ifdefs to files. If this patch is landed again, these additional changes need to be made: - The Geolocation-related export in WebCore/WebCore.base.exp needs to be put into its own file (WebCore/WebCore.Geolocation.exp). - WebCore/DerivedSources.make needs to be updated to append WebCore.Geolocation.exp to WEBCORE_EXPORT_DEPENDENCIES (see examples for WebCore.NPAPI.exp or WebCore.JNI.exp). - A few files in WebKit/mac will need #if ENABLE(GEOLOCATION)/#endif guards added.

Created attachment 50821 [details] second try Size of the geolocation code has increased recently and the geolocation code now also assumes sqlite3. The top level GEOLOCATION flag has been already introduced since this patch was first filed.

Created attachment 50822 [details] right patch this time

Created attachment 51798 [details] re-baseline previous patch

Comment on attachment 51798 [details] re-baseline previous patch > +2010-03-26 Laszlo Gombos <laszlo.1.gombos@nokia.com> > + > + Reviewed by NOBODY (OOPS!). > + > + Guard Geolocation files with ENABLE_GEOLOCATION > + https://bugs.webkit.org/show_bug.cgi?id=25756 > + > + The intent is to guard the Geolocation implementation files > + and minimize the impact on This seems like an incomplete sentence? > Index: WebCore/page/Geolocation.cpp > =================================================================== > --- WebCore/page/Geolocation.cpp (revision 56641) > +++ WebCore/page/Geolocation.cpp (working copy) > @@ -28,6 +28,8 @@ > #include "config.h" > #include "Geolocation.h" > > +#if ENABLE(GEOLOCATION) > + > #include "Chrome.h" > #include "Frame.h" > #include "Page.h" > @@ -641,3 +643,19 @@ void Geolocation::stopUpdating() > } > > } // namespace WebCore > + > +#else > + > +namespace WebCore { > + > +void Geolocation::disconnectFrame() {} > + > +Geolocation::Geolocation(Frame*) {} > + > +Geolocation::~Geolocation() {} > + > +void Geolocation::setIsAllowed(bool) {} > + > +} > + > +#endif // ENABLE(GEOLOCATION) I think the preferred place for the stub implementation is in the header file (Geolocation.h) so that the whole thing can be inlined and optimized out by the compiler. r- to move the stub implementation into the header and fix the ChangeLog entry. Otherwise this looks great!

Created attachment 51953 [details] fix ChangeLog Thanks for the review, David. I fixed the ChangeLog. I considered your suggestion of moving stub implementation is in the header file; however I think that would mean that those symbols could not not get exported. I was trying to minimize the impact on clients, so that a stub symbol is still exported for most symbols. For example Geolocation::setIsAllowed() is still listed in WebCore.base.exp. Moving those symbols to the header would mean moving some more symbols over to WebCore.Geolocation.exp and making more changes under WebKit/mac. I'm happy to make those changes if that is the direction we want to take this patch. For now, I just resubmitted the patch with only changes in the ChangeLog.

(In reply to comment #14) > Created an attachment (id=51953) [details] > fix ChangeLog > > Thanks for the review, David. I fixed the ChangeLog. > > I considered your suggestion of moving stub implementation is in the header > file; however I think that would mean that those symbols could not not get > exported. I was trying to minimize the impact on clients, so that a stub symbol > is still exported for most symbols. For example Geolocation::setIsAllowed() is > still listed in WebCore.base.exp. > > Moving those symbols to the header would mean moving some more symbols over to > WebCore.Geolocation.exp and making more changes under WebKit/mac. I'm happy to > make those changes if that is the direction we want to take this patch. > > For now, I just resubmitted the patch with only changes in the ChangeLog. So there are two options here--move the exported symbols to WebCore.Geolocation.exp and add more ENABLE(GEOLOCATION) macros to WebKit, or leave it as is. If you want to save more space, I would move more symbols to the export file, but I'll review the current patch as-is.

Comment on attachment 51953 [details] fix ChangeLog r=me, but consider adding more symbols to WebCore.Geolocation.exp.

Comment on attachment 51953 [details] fix ChangeLog Thanks for the review. I'll mark the patch for landing as it is and will introduce more optimizations in a subsequent patch.

Comment on attachment 51953 [details] fix ChangeLog Clearing flags on attachment: 51953 Committed r56781: <http://trac.webkit.org/changeset/56781>

All reviewed patches have been landed. Closing bug.

Revision r44093 cherry-picked into qtwebkit-2.0 with commit 741b45f9897eaf6c8313a3db6b241315767045d8

Revision r56781 cherry-picked into qtwebkit-2.0 with commit 3d5b2276f815adb90f1edc312f4c623ade8befe0