Bug 31554

Summary: [Android] Android is missing the implementation of the GeolocationService iface.
Product: WebKit Reporter: Andrei Popescu <andreip>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: android-webkit-unforking, benm
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Other   
Attachments:
Description Flags
Android implementation of the GeolocationService implementation
dglazkov: review-
Android implementation of the GeolocationService implementation v2
none
Android implementation of the GeolocationService implementation v2 dglazkov: review+, benm: commit-queue-

Description Andrei Popescu 2009-11-16 08:06:13 PST
Each port of WebKit that wants to support Geolocation must implement the GeolocationService interface. So Android needs one too.
Comment 1 Andrei Popescu 2009-11-16 08:16:16 PST
Created attachment 43308 [details]
Android implementation of the GeolocationService implementation

The corresponding code in the Android source tree can be found at

http://android.git.kernel.org/?p=platform/external/webkit.git;a=tree;f=WebCore/platform/android;h=b66c3277d6de15bd67218256deeafa72925b557d;hb=refs/heads/eclair
Comment 2 Dimitri Glazkov (Google) 2009-11-16 09:15:04 PST
Comment on attachment 43308 [details]
Android implementation of the GeolocationService implementation

> +// GeolocationServiceBridge is the bridge to the Java implementation. It manages
> +// the lifetime of the Java object. It is an implementation detail of
> +// GeolocationServiceAndroid.
> +class GeolocationServiceBridge {

Can you split this out into a separate file?

> +    static PassRefPtr<Geoposition> convertLocationToGeoposition(JNIEnv *env, const jobject &location);

toGeoposition is shorter and more inline with the WebKit convention.

> +static const char* kJavaGeolocationServiceClass = "android/webkit/GeolocationService";

prefix k* is not necessary, just use standard camelCase.

> +enum kJavaGeolocationServiceClassMethods {
> +    GEOLOCATION_SERVICE_METHOD_INIT = 0,

GeolocationServiceMethodInit <-- casing here and elsewhere

> +    GEOLOCATION_SERVICE_METHOD_START,
> +    GEOLOCATION_SERVICE_METHOD_STOP,
> +    GEOLOCATION_SERVICE_METHOD_SET_ENABLE_GPS,
> +    GEOLOCATION_SERVICE_METHOD_COUNT,
> +};
Comment 3 Andrei Popescu 2009-11-16 10:48:53 PST
Created attachment 43314 [details]
Android implementation of the GeolocationService implementation v2
Comment 4 Andrei Popescu 2009-11-16 10:52:37 PST
Created attachment 43315 [details]
Android implementation of the GeolocationService implementation v2

The previous patch was missing new lines at the end of the new files.
Comment 5 Andrei Popescu 2009-11-16 10:54:59 PST
Thanks Dimitri,

(In reply to comment #2)
> (From update of attachment 43308 [details])
> > +// GeolocationServiceBridge is the bridge to the Java implementation. It manages
> > +// the lifetime of the Java object. It is an implementation detail of
> > +// GeolocationServiceAndroid.
> > +class GeolocationServiceBridge {
> 
> Can you split this out into a separate file?
> 

Sure, done.

> > +    static PassRefPtr<Geoposition> convertLocationToGeoposition(JNIEnv *env, const jobject &location);
> 
> toGeoposition is shorter and more inline with the WebKit convention.
>

Indeed. Changed.
 
> > +static const char* kJavaGeolocationServiceClass = "android/webkit/GeolocationService";
> 
> prefix k* is not necessary, just use standard camelCase.
> 

Done.

> > +enum kJavaGeolocationServiceClassMethods {
> > +    GEOLOCATION_SERVICE_METHOD_INIT = 0,
> 
> GeolocationServiceMethodInit <-- casing here and elsewhere
>

Fixed.
 
Andrei
Comment 6 Dimitri Glazkov (Google) 2009-11-16 11:09:48 PST
Comment on attachment 43315 [details]
Android implementation of the GeolocationService implementation v2

looks good.
Comment 7 Ben Murdoch 2009-11-17 04:28:20 PST
Comment on attachment 43315 [details]
Android implementation of the GeolocationService implementation v2

Will land this manually as the commit queue is currently disabled.
Comment 8 Ben Murdoch 2009-11-17 05:13:32 PST
Landed as r51071.