[Webkit-unassigned] [Bug 49735] [Chromium] Introduce wrapper types for WebCore::GeolocationError, WebCore::GeolocationPosition, WebCore::GeolocationController

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=49735





--- Comment #3 from John Knottenbelt <jknotten at chromium.org>  2010-11-19 10:17:40 PST ---
(From update of attachment 74382)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.