Bug 49735

Created attachment 74382 [details] Patch

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.

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?

Created attachment 74558 [details] Patch

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

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.

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::

Created attachment 74675 [details] Patch

Comment on attachment 74675 [details] Patch Thanks again for the review. I've made the changes you suggested.

Comment on attachment 74675 [details] Patch Clearing flags on attachment: 74675 Committed r72624: <http://trac.webkit.org/changeset/72624>

All reviewed patches have been landed. Closing bug.