Bug 25756 - Explicit guards for ENABLE_GEOLOCATION
Summary: Explicit guards for ENABLE_GEOLOCATION
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 35784
  Show dependency treegraph
 
Reported: 2009-05-13 13:25 PDT by Laszlo Gombos
Modified: 2010-03-30 07:08 PDT (History)
10 users (show)

See Also:


Attachments
Add GEOLOCATION guards around Geolocation code (8.23 KB, patch)
2009-05-13 13:29 PDT, Laszlo Gombos
ddkilzer: review-
Details | Formatted Diff | Diff
second try (8.23 KB, patch)
2010-03-16 12:18 PDT, Laszlo Gombos
no flags Details | Formatted Diff | Diff
right patch this time (11.63 KB, patch)
2010-03-16 12:20 PDT, Laszlo Gombos
no flags Details | Formatted Diff | Diff
re-baseline previous patch (11.71 KB, patch)
2010-03-26 17:00 PDT, Laszlo Gombos
ddkilzer: review-
Details | Formatted Diff | Diff
fix ChangeLog (11.74 KB, patch)
2010-03-29 12:58 PDT, Laszlo Gombos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Laszlo Gombos 2009-05-13 13:25:05 PDT
I have found entire source files where the ENABLE_GEOLOCATION
macro guard is appropriate. Inserting these guards helps reduce the overall
library size when the geolocation api is disabled. Please see attached patch.
Comment 1 Laszlo Gombos 2009-05-13 13:29:22 PDT
Created attachment 30283 [details]
Add GEOLOCATION guards around Geolocation code
Comment 2 Darin Adler 2009-05-13 13:42:22 PDT
I think Sam should review this. Geolocation is already off for various platforms; I'm not sure why this patch is needed.
Comment 3 Laszlo Gombos 2009-05-15 10:12:19 PDT
(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 4 Eric Seidel (no email) 2009-05-19 22:47:23 PDT
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.
Comment 5 Holger Freyther 2009-05-23 08:28:58 PDT
Landed in r44093. This worked on Qt and Gtk+ with geolocation enabled.
Comment 6 David Kilzer (:ddkilzer) 2009-05-23 09:15:43 PDT
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 7 David Kilzer (:ddkilzer) 2009-05-23 09:16:11 PDT
Comment on attachment 30283 [details]
Add GEOLOCATION guards around Geolocation code

r- per Comment #6
Comment 8 Greg Bolsinga 2009-05-23 13:15:38 PDT
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.
Comment 9 David Kilzer (:ddkilzer) 2009-05-24 05:23:46 PDT
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.
Comment 10 Laszlo Gombos 2010-03-16 12:18:29 PDT
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.
Comment 11 Laszlo Gombos 2010-03-16 12:20:46 PDT
Created attachment 50822 [details]
right patch this time
Comment 12 Laszlo Gombos 2010-03-26 17:00:01 PDT
Created attachment 51798 [details]
re-baseline previous patch
Comment 13 David Kilzer (:ddkilzer) 2010-03-28 20:46:21 PDT
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!
Comment 14 Laszlo Gombos 2010-03-29 12:58:40 PDT
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.
Comment 15 David Kilzer (:ddkilzer) 2010-03-29 21:09:19 PDT
(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 16 David Kilzer (:ddkilzer) 2010-03-29 21:11:56 PDT
Comment on attachment 51953 [details]
fix ChangeLog

r=me, but consider adding more symbols to WebCore.Geolocation.exp.
Comment 17 Laszlo Gombos 2010-03-30 05:20:13 PDT
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 18 WebKit Commit Bot 2010-03-30 06:27:41 PDT
Comment on attachment 51953 [details]
fix ChangeLog

Clearing flags on attachment: 51953

Committed r56781: <http://trac.webkit.org/changeset/56781>
Comment 19 WebKit Commit Bot 2010-03-30 06:27:47 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Simon Hausmann 2010-03-30 07:01:17 PDT
Revision r44093 cherry-picked into qtwebkit-2.0 with commit 741b45f9897eaf6c8313a3db6b241315767045d8
Comment 21 Simon Hausmann 2010-03-30 07:08:15 PDT
Revision r56781 cherry-picked into qtwebkit-2.0 with commit 3d5b2276f815adb90f1edc312f4c623ade8befe0