[Webkit-unassigned] [Bug 49258] Implement Mocks for Client-based Geolocation

[bugzilla-daemon at webkit.org]

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





--- Comment #2 from Steve Block <steveblock at google.com>  2010-11-11 04:22:24 PST ---
(From update of attachment 73484)
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

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