Bug 49735 – [Chromium] Introduce wrapper types for WebCore::GeolocationError, WebCore::GeolocationPosition

• Component: WebKit API
• Status: RESOLVED FIXED

[John Knottenbelt]

Introduce WebKit public API for dealing with Geolocation errors, positions and controller interface.

[John Knottenbelt]

Created attachment 74382 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 74382 [details]
Patch

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

> WebKit/chromium/public/WebGeolocationController.h:59
> +    WebCore::GeolocationController* m_controller;

what prevents the GeolocationController object from being destroyed before the WebGeolocationController?
how do you avoid memory errors in that case?

this seems error prone.

> WebKit/chromium/public/WebGeolocationError.h:38
> +class WebGeolocationError {

GeolocationError is reference counted, so it seems like you should implement
WebGeolocationError as a simple wrapper (like WebNode).

Use WebPrivatePtr<WebCore::GeolocationError> as your sole member variable.

> WebKit/chromium/public/WebGeolocationPosition.h:88
> +    void copyFrom(const WebCore::GeolocationPosition&);

GeolocationPosition is reference counted, so it seems like you should implement
WebGeolocationPosition as a simple wrapper (like WebNode).

Use WebPrivatePtr<WebCore::GeolocationPosition> as your sole member variable.

[John Knottenbelt]

Comment on attachment 74382 [details]
Patch

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

>> WebKit/chromium/public/WebGeolocationController.h:59
>> +    WebCore::GeolocationController* m_controller;
> 
> what prevents the GeolocationController object from being destroyed before the WebGeolocationController?
> how do you avoid memory errors in that case?
> 
> this seems error prone.

Thanks for reviewing so quickly.

The destructor of WebCore::GeolocationController calls WebCore::GeolocationClient::geolocationDestroyed() on it's client. This client interface will be implemented by a class called GeolocationClientProxy which will in turn call it's client's geolocationDestroyed() method - WebKit::WebGeolocationClient::geolocationDestroyed(). Implementers of the client should set their WebGeolocationController member to 0 on receiving geolocationDestroyed(). 

I agree that this seems error prone. I have been following a similar pattern to the DeviceOrientation (WebDeviceOrientationController / DeviceOrientationProxy) which uses a similar scheme.

I will include the above mentioned classes in a future patch to this bug, as it seems that they are necessary to understand this issue.

In the meantime, I will think more about it and see if I can come up with anything simpler.

>> WebKit/chromium/public/WebGeolocationError.h:38
>> +class WebGeolocationError {
> 
> GeolocationError is reference counted, so it seems like you should implement
> WebGeolocationError as a simple wrapper (like WebNode).
> 
> Use WebPrivatePtr<WebCore::GeolocationError> as your sole member variable.

Does the same comment you made about the DeviceMotionData class in https://bugs.webkit.org/show_bug.cgi?id=47078#c11 apply?

"Looking at the way this class is used, I'm not sure why you didn't just
do the same thing as you did for WebDeviceOrientation.  It seems like it
does not need to be a wrapper around the WebCore type.  It can just be
a simple class with member variables for the various fields."

However, it seems that there is already a Geoposition struct (src/chrome/common/Geoposition.h) being marshalled over the IPC for the non-client based Geolocation. So it seems perhaps a simple wrapper (as you suggest) will be better after all?

[John Knottenbelt]

Created attachment 74558 [details]
Patch

[John Knottenbelt]

This patch changes the WebGeolocationError and WebGeolocationPosition classes into simple WebCore wrappers as Darin suggested. 

The WebGeolocationController class has been removed from this patch and will be presented in a forthcoming patch where it can be seen in its proper context.

(In reply to comment #4)
> Created an attachment (id=74558) [details]
> Patch

[John Knottenbelt]

WebGeolocationController has moved to https://bugs.webkit.org/show_bug.cgi?id=46895

(In reply to comment #5)
> This patch changes the WebGeolocationError and WebGeolocationPosition classes into simple WebCore wrappers as Darin suggested. 
> 
> The WebGeolocationController class has been removed from this patch and will be presented in a forthcoming patch where it can be seen in its proper context.

[Darin Fisher (:fishd, Google)]

Comment on attachment 74558 [details]
Patch

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

> WebKit/chromium/public/WebGeolocationError.h:43
> +    enum ErrorCode {

Sorry, I should have pointed this out in the previous review, but in the
WebKit API enum names should be formatted like this:

  enum Foo {
      FooA,
      FooB
  };

In this case, I would use:

  enum Error {
      ErrorPermissionDenied,
      ErrorPositionUnavailable
  };

I'd recommend asserting with a compile-time assert that these enum values
match the corresponding WebCore types by adding to AssertMatchingEnums.cpp.
That way, in the implementation code, you can just static_cast between
the WebKit and WebCore enum types.

> WebKit/chromium/public/WebGeolocationPosition.h:41
> +    WebGeolocationPosition() {};

nit: no trailing ";"

> WebKit/chromium/public/WebGeolocationPosition.h:58
> +

nit: delete extra new line, leave only one between #endif and "private:" label.

> WebKit/chromium/src/WebGeolocationError.cpp:51
> +WebGeolocationError::WebGeolocationError(WTF::PassRefPtr<WebCore::GeolocationError> error)

nit: no need for WTF:: or WebCore:: prefixes in this file.

> WebKit/chromium/src/WebGeolocationError.cpp:56
> +WebGeolocationError& WebGeolocationError::operator=(WTF::PassRefPtr<WebCore::GeolocationError> error)

ditto

> WebKit/chromium/src/WebGeolocationError.cpp:62
> +WebGeolocationError::operator WTF::PassRefPtr<WebCore::GeolocationError>() const

ditto

> WebKit/chromium/src/WebGeolocationPosition.cpp:50
> +WebGeolocationPosition& WebGeolocationPosition::operator=(WTF::PassRefPtr<WebCore::GeolocationPosition> position)

same nit about dropping WTF:: and WebCore::

[John Knottenbelt]

Created attachment 74675 [details]
Patch

[John Knottenbelt]

Comment on attachment 74675 [details]
Patch

Thanks again for the review. I've made the changes you suggested.

[WebKit Commit Bot]

Comment on attachment 74675 [details]
Patch

Clearing flags on attachment: 74675

Committed r72624: <http://trac.webkit.org/changeset/72624>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.