In preparation for introducing client-based geolocation classes, get the chromium port building and running with CLIENT_BASED_GEOLOCATION
Created attachment 70604 [details] Patch
Comment on attachment 70604 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=70604&action=review > WebKitTools/DumpRenderTree/chromium/LayoutTestController.cpp:1501 > +#if !ENABLE(CLIENT_BASED_GEOLOCATION) Do you need these guards now that you've stubbed out the client-based impl? If you do, I think the following style is better ... #if client-based // FIXME #else foo(); #endif
Created attachment 70610 [details] Patch
You are right, I do not need the guards in LayoutTestController.cpp, well spotted.
Comment on attachment 70610 [details] Patch LGTM, but best to wait for a review from chromium.
(In reply to comment #5) > (From update of attachment 70610 [details]) > LGTM, but best to wait for a review from chromium. looks fine by me as well, just two minor suggestions: View in context: https://bugs.webkit.org/attachment.cgi?id=70610&action=review > WebCore/WebCore.gyp/WebCore.gyp:1207 > + ['exclude', '/GeolocationService.*$'], I'm not too familiar with gyp, but would this exclude platform/chromium/GeolocationServiceChromium.*$ ? you may also want to add something like: #if ENABLE(CLIENT_BASED_GEOLOCATION) #error "This file should not be compiled when ENABLE(CLIENT_BASED_GEOLOCATION)" #endif // ENABLE(CLIENT_BASED_GEOLOCATION) to those files, so that it makes a clearer documentation (on top of removing the entry points..) View in context: https://bugs.webkit.org/attachment.cgi?id=70610&action=review > WebKit/chromium/src/WebGeolocationServiceMock.cpp:46 > +// FIXME: Implement mock bindings for client-based geolocation and remove WebGeolocationServiceMockClientBasedImpl?
Created attachment 70613 [details] Patch
Comment on attachment 70613 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=70613&action=review > WebCore/platform/gtk/GeolocationServiceGtk.cpp:25 > +#endif // ENABLE(CLIENT_BASED_GEOLOCATION) Sorry to be a pain, but I don't think we should be making these changes to non-Chromium files. It's making assumptions about the build systems of other platforms. Is there a need for a guard in these files? Will building this file when !client-based cause a compile error? If not, let's leave it alone. If so, we should add guards around the code like elsewhere.
Created attachment 70616 [details] Patch
Comment on attachment 70616 [details] Patch Clearing flags on attachment: 70616 Committed r69673: <http://trac.webkit.org/changeset/69673>
All reviewed patches have been landed. Closing bug.