Bug 32499 – Add client based Geolocation provider

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

We should add a way to string geolocation information up through the client infrastructure.

[Sam Weinig]

Created attachment 44768 [details]
Patch

[WebKit Review Bot]

Attachment 44768 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/page/Geolocation.h:56:  Missing space after ,  [whitespace/comma] [3]
WebCore/page/Geolocation.cpp:62:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/page/GeolocationError.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/page/GeolocationPosition.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 4

Comment on attachment 44768 [details]
Patch

CLIENT_BASED_GEOLOCATION is not a platform feature, so it’s not appropriate to guard it with USE(). Please use ENABLE(CLIENT_BASED_GEOLOCATION) (or maybe CLIENT_GEOLOCATION_PROVIDER).

> +        Add client based Geolocation provider

Not sure Geolocation needs to be capitalized here.

> +static PassRefPtr<Geoposition> createGeopositionFromGeolocationPosition(GeolocationPosition* position)

You can call this createGeoposition().

> +static PassRefPtr<PositionError> createPositionErrorFromGeolocationError(GeolocationError* error)

You can call this createGeoposition().

> +void Geolocation::positionChanged()

I think this method should take a PassRefPtr<Geoposition> and set m_currentPosition to it. Both call sites know the Geoposition.

> +void Geolocation::positionChanged(GeolocationPosition* position)

I’d call this setPosition().

> +void Geolocation::errorOccurred(GeolocationError* error)

And I’d call this setError().

> +    RefPtr<Geoposition> m_lastPosition;

This variable isn’t necessary.

> +    HashSet<RefPtr<Geolocation> >::const_iterator it = m_observers.begin();
> +    HashSet<RefPtr<Geolocation> >::const_iterator end = m_observers.end();
> +    for (; it != end; ++it)

Normally the loop variable is defined and initialized in the for statement.

> +    enum ErrorCode {
> +        PermissionDenied,
> +        PositionUnavailable
> +    };

It’s strange to call these “codes” but I don’t have a better suggestion.

>  #if ENABLE(NOTIFICATIONS)
>      class NotificationPresenter;
>  #endif
> +#if ENABLE(CLIENT_BASED_GEOLOCATION)
> +    class NotificationPresenter;
> +#endif

Please combine these into a single #if ENABLE(NOTIFICATIONS) || ENABLE(CLIENT_BASED_GEOLOCATION).

> +#if USE(CLIENT_BASED_GEOLOCATION)
> +        GeolocationController* geolocationController() const { return m_geolocationController.get(); }
> +#endif

Can this return a GeolocationController&? Seems like it should never be 0. Which makes me wonder why it’s referenced by pointer and allocated dynamically instead of being a member of Page. However, this is consistent with other clients so I’m not asking that you change it.

(In reply to comment #3)
> > +static PassRefPtr<PositionError> createPositionErrorFromGeolocationError(GeolocationError* error)
> 
> You can call this createGeoposition().

Sorry, I meant createPositionError().

[Sam Weinig]

> >  #if ENABLE(NOTIFICATIONS)
> >      class NotificationPresenter;
> >  #endif
> > +#if ENABLE(CLIENT_BASED_GEOLOCATION)
> > +    class NotificationPresenter;
> > +#endif

This is not necessary, it was left in by mistake.  I have remove it.

[Sam Weinig]

Created attachment 44771 [details]
Patch 2

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44771 [details] without any errors.

[Sam Weinig]

Landed in r52103.

[Steve Block]

Eric, thanks for adding me to the bug.

Sam, I have a few questions about the patch ...
- Can you explain the motivation behind adding an alternative to the current Geolocation service? What are the advantages of the client-based provider? Do you intend that we support both types in the future, or will the existing Geolocation service be deprecated?
- Why do you introduce new position and error types? Why can't the existing Geoposition and PositionError types be used with the client provider? I see you've excluded the TIMEOUT error code from the new error type as it's not needed by the provider. Why have you left in the PERMISSION_DENIED code? This too is handled separately by the Geolocation class.
- What's the reason for making Geolocation::suspend/resume() private? These methods are used as public methods on Android, where we suspend the Geolocation service when the browser tab is in the background. Also, it looks like the implementation of these methods is missing.

Finally, could you CC me on all future Geolocation changes please? I'm working on Geolocation in Android and have sent a number of patches recently.

Thanks,
Steve

[Sam Weinig]

(In reply to comment #9)
> Eric, thanks for adding me to the bug.
> 
> Sam, I have a few questions about the patch ...
> - Can you explain the motivation behind adding an alternative to the current
> Geolocation service? What are the advantages of the client-based provider?

The intent of the client based provider is to allow an embedding layer more control of the geolocation implementation without egregious layering violations.

> Do you intend that we support both types in the future, or will the existing
> Geolocation service be deprecated?

It will only be deprecated if the new way works better for others.

> - Why do you introduce new position and error types? Why can't the existing
> Geoposition and PositionError types be used with the client provider? I see
> you've excluded the TIMEOUT error code from the new error type as it's not
> needed by the provider. Why have you left in the PERMISSION_DENIED code? This
> too is handled separately by the Geolocation class.

I did this because it is an existing layering violation for the platform layer to know about these DOM types.  In general, I don't think it is a good idea to conflate data coming from the platform and DOM types due to the caching behavior of JS objects.

> - What's the reason for making Geolocation::suspend/resume() private? These
> methods are used as public methods on Android, where we suspend the Geolocation
> service when the browser tab is in the background. Also, it looks like the
> implementation of these methods is missing.

I made them private to find out if anyone was using it, and since it compiled I assumed no one was.  I meant to remove them.  I will add them back if you are using them.

> Finally, could you CC me on all future Geolocation changes please? I'm working
> on Geolocation in Android and have sent a number of patches recently.
> 

Sure.

[Steve Block]

> I did this because it is an existing layering violation for the platform layer
> to know about these DOM types.  In general, I don't think it is a good idea to
> conflate data coming from the platform and DOM types due to the caching
> behavior of JS objects.
I see.

> I made them private to find out if anyone was using it, and since it compiled I
> assumed no one was.  I meant to remove them.
We're upstreaming as fast as we can but I'm afraid the relevant Android files aren't yet there.

> I will add them back if you are using them.
Thanks