Bug 47586 – Get CLIENT_BASED_GEOLOCATION building

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[John Knottenbelt]

In preparation for introducing client-based geolocation classes, get the chromium port building and running with CLIENT_BASED_GEOLOCATION

[John Knottenbelt]

Created attachment 70604 [details]
Patch

[Steve Block]

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

[John Knottenbelt]

Created attachment 70610 [details]
Patch

[John Knottenbelt]

You are right, I do not need the guards in LayoutTestController.cpp, well spotted.

[Steve Block]

Comment on attachment 70610 [details]
Patch

LGTM, but best to wait for a review from chromium.

[Marcus Bulach]

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

[John Knottenbelt]

Created attachment 70613 [details]
Patch

[Steve Block]

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.

[John Knottenbelt]

Created attachment 70616 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 70616 [details]
Patch

Clearing flags on attachment: 70616

Committed r69673: <http://trac.webkit.org/changeset/69673>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.