Bug 47586 - Get CLIENT_BASED_GEOLOCATION building
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
Depends on:
Blocks: 45752
  Show dependency treegraph
Reported: 2010-10-13 06:00 PDT by John Knottenbelt
Modified: 2010-10-13 11:02 PDT (History)
4 users (show)

See Also:

Patch (13.08 KB, patch)
2010-10-13 06:22 PDT, John Knottenbelt
no flags Details | Formatted Diff | Diff
Patch (10.84 KB, patch)
2010-10-13 07:36 PDT, John Knottenbelt
no flags Details | Formatted Diff | Diff
Patch (12.89 KB, patch)
2010-10-13 09:12 PDT, John Knottenbelt
no flags Details | Formatted Diff | Diff
Patch (11.60 KB, patch)
2010-10-13 09:30 PDT, John Knottenbelt
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Knottenbelt 2010-10-13 06:00:53 PDT
In preparation for introducing client-based geolocation classes, get the chromium port building and running with CLIENT_BASED_GEOLOCATION
Comment 1 John Knottenbelt 2010-10-13 06:22:33 PDT
Created attachment 70604 [details]
Comment 2 Steve Block 2010-10-13 06:40:42 PDT
Comment on attachment 70604 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=70604&action=review

> WebKitTools/DumpRenderTree/chromium/LayoutTestController.cpp:1501

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
Comment 3 John Knottenbelt 2010-10-13 07:36:00 PDT
Created attachment 70610 [details]
Comment 4 John Knottenbelt 2010-10-13 07:37:01 PDT
You are right, I do not need the guards in LayoutTestController.cpp, well spotted.
Comment 5 Steve Block 2010-10-13 07:40:56 PDT
Comment on attachment 70610 [details]

LGTM, but best to wait for a review from chromium.
Comment 6 Marcus Bulach 2010-10-13 08:18:30 PDT
(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:
#error "This file should not be compiled when 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?
Comment 7 John Knottenbelt 2010-10-13 09:12:47 PDT
Created attachment 70613 [details]
Comment 8 Steve Block 2010-10-13 09:22:56 PDT
Comment on attachment 70613 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=70613&action=review

> WebCore/platform/gtk/GeolocationServiceGtk.cpp:25

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.
Comment 9 John Knottenbelt 2010-10-13 09:30:36 PDT
Created attachment 70616 [details]
Comment 10 WebKit Commit Bot 2010-10-13 11:02:40 PDT
Comment on attachment 70616 [details]

Clearing flags on attachment: 70616

Committed r69673: <http://trac.webkit.org/changeset/69673>
Comment 11 WebKit Commit Bot 2010-10-13 11:02:45 PDT
All reviewed patches have been landed.  Closing bug.