Created attachment 73484 [details] Patch

Comment on attachment 73484 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=73484&action=review The non-client-based mock and the Mac client-based mock both share the mock position or error between all WebViews, but this mock does not. Is this intentional? Presumably you intend this sharing to be implemented by each port, if required by the LayoutTests? Also, we should update Mac (currently the only port using client-based Geolocation I think) to use this mock. That can be a separate patch if you like. do you have a bug? > WebCore/Android.mk:582 > + platform/mock/GeolocationClientMock.cpp \ Adding this here is fine, but for future reference, on Android we only add things to the makefiles when they're required. > WebCore/platform/mock/GeolocationClientMock.cpp:34 > +#include "GeolocationClientMock.h" I think the style is to keep config.h and the main header together, with the guard after them both > WebCore/platform/mock/GeolocationClientMock.cpp:55 > + ASSERT(!controller || !m_controller); Can you explain why you test for !controller? When would this ever be called with a null controller? > WebCore/platform/mock/GeolocationClientMock.cpp:64 > + makeGeolocationCallback(); I don't think it's better to update the controller synchronously here. This means that a JS call to LayoutTestController.setMockGeolocationPosition() will result in a synchronous call back to JS. This make LayoutTests hard to debug because of the re-entrancy. I think it's best to make the callback asynchronously, as the existing Mac client-based mock does. > WebCore/platform/mock/GeolocationClientMock.cpp:77 > + m_controller = 0; Can you explain why this is required? It looks like the controller will always stop the client before it calls this method. So there's no way we could ever try to make a callback after the controller and Geolocation objects have gone away. If you want to be sure, you could leave this in, but change line 112 to an assert? > WebCore/platform/mock/GeolocationClientMock.cpp:96 > + // FIXME: We need to add some tests regarding "high accuracy" mode. Do you have a bug for this? Maybe add a link? > WebCore/platform/mock/GeolocationClientMock.cpp:110 > +void GeolocationClientMock::makeGeolocationCallback() I think 'Geolocation' is superfluous - maybe 'makeCallback' or 'updateController'? > WebCore/platform/mock/GeolocationClientMock.h:65 > + virtual GeolocationPosition *lastPosition(); '*' should go on the left. > WebCore/platform/mock/GeolocationClientMock.h:78 > + No need for all these blank lines

Created attachment 74242 [details] Patch

I still think that having mock objects in WebCore is a strategically wrong approach.

(In reply to comment #4) > I still think that having mock objects in WebCore is a strategically wrong approach. For those who are not familiar with the thread on webkit-dev, here is a link http://www.mail-archive.com/webkit-dev@lists.webkit.org/msg12162.html After all embedders have switched to using client-based geolocation ( https://bugs.webkit.org/show_bug.cgi?id=40373 ), we will be able to remove WebCore/platform/mock/GeolocationServiceMock.cpp altogther, so the net result will be to replace GeolocationServiceMock with GeolocationClientMock. As mentioned in the thread, I do think it is a good idea for the mocks to be compiled out of WebCore in production code. I have created bug https://bugs.webkit.org/show_bug.cgi?id=49806 and will raise this topic for discussion on webkit-dev when I come to look at it.

Comment on attachment 74242 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=74242&action=review > WebCore/platform/mock/GeolocationClientMock.cpp:34 > +#if ENABLE(CLIENT_BASED_GEOLOCATION) Usually have a blank line around guards for the whole file > WebCore/platform/mock/GeolocationClientMock.cpp:75 > + // DumpRenderTree calls this before the running the next test. I'm not sure this comment is needed - and it should probably be in the header anyway. > WebCore/platform/mock/GeolocationClientMock.cpp:110 > +void GeolocationClientMock::setTimer() How about something more descriptive, like 'asynchronouslyUpdateController()'? > WebCore/platform/mock/GeolocationClientMock.cpp:112 > + if (m_controller && m_isActive && !m_timer.isActive()) In updateController(), you assert m_controller. Why not the same here? > WebCore/platform/mock/GeolocationClientMock.cpp:129 > + if (m_lastError.get()) We should never have both an error and a position, so you could use an 'else if' to make that clear. > WebCore/platform/mock/GeolocationClientMock.h:73 > + GeolocationController *m_controller; '*' on left

Created attachment 74562 [details] Patch

Comment on attachment 74562 [details] Patch Thanks for the review, I have made the suggested changes.

Comment on attachment 74562 [details] Patch Clearing flags on attachment: 74562 Committed r72581: <http://trac.webkit.org/changeset/72581>

All reviewed patches have been landed. Closing bug.

*** Bug 43925 has been marked as a duplicate of this bug. ***