Bug 49438 – Add LayoutTests to test high accuracy mode in Geolocation

• Status: UNCONFIRMED
• Component: Tools / Tests

[John Knottenbelt]

We need additional LayoutTests to test that setEnableHighAccuracy is being invoked correctly on the GeolocationClient.

[John Knottenbelt]

Created attachment 76085 [details]
Patch

Draft patch to get feedback on general approach.

[Steve Block]

Comment on attachment 76085 [details]
Patch

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

I'm not sure how useful it is to add this test. The test code is rather subtle, yet all we're really testing is that a boolean flag is successfully passed to the client/service. Would it be better to add a layoutTestController.getGeolocationClientState() method which returns {idle,lowAccuracy,highAccuracy} so we can test this directly? That would also allow us to more easily write more interesting tests for the multiplexing of multiple requests to a single client in GeolocationController.

> WebKit/chromium/public/WebGeolocationClientMock.h:52
> +    WEBKIT_API void setPositions(double lowAccuracyLatitude, double lowAccuracyLongitude, double lowAccuracyAccuracy, double highAccuracyLatitude, double highAccuracyLongitude, double highAccuracyAccuracy);

I think you should do this in the non-client-based mock too.

> WebKitTools/DumpRenderTree/LayoutTestController.h:98
> +    void setMockGeolocationPositions(double lowAccuracyLatitude, double lowAccuracyLongitude, double lowAccuracyAccuracy,

This name might be a little confusing. Maybe it's better to make this setMockGeolocationPositionHighAccuracy(lat, lng, acc) - the position is used preferentially to the regular mock position when in high accuracy mode and when available.

[John Knottenbelt]

Created attachment 76647 [details]
Patch

[John Knottenbelt]

This new patch takes the alternative approach of adding a geolocationClientState method to the layoutTestController. Comments and suggestions very welcome!

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

[WebKit Review Bot]

Attachment 76647 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7173028

[Build Bot]

Attachment 76647 [details] did not build on win:
Build output: http://queues.webkit.org/results/7154037

[Steve Block]

Comment on attachment 76647 [details]
Patch

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

> LayoutTests/fast/dom/Geolocation/script-tests/accuracy-requests.js:20
> +    layoutTestController.setGeolocationPermission(true);

Inconsistent indentation. Should be 4 spaces I think.

> LayoutTests/fast/dom/Geolocation/script-tests/accuracy-requests.js:29
> +var clientState;

Are these used?

> LayoutTests/fast/dom/Geolocation/script-tests/accuracy-requests.js:44
> +      layoutTestController.setMockGeolocationPosition(mockLatitude+1, mockLongitude+1, mockAccuracy+1);

Spaces around operators

> LayoutTests/fast/dom/Geolocation/script-tests/accuracy-requests.js:47
> +  }

No braces on one-line control statements

> WebCore/platform/mock/GeolocationServiceMock.cpp:81
> +    for (GeolocationServiceSet::const_iterator iter = s_instances->begin(); iter != end; ++iter) {

This is wrong I think. The LTC method should return the state of the Geolocation service/client for the current window. In the case of the client-based impl, we have one client per page, so we query the client corresponding to our page. For the non-client-based impl, we have one service per winfo, so we should query that.

> WebCore/platform/mock/GeolocationServiceMock.h:46
> +        Low,

I think it's better to be explicit - LowAccuracy

> WebCore/platform/mock/GeolocationServiceMock.h:58
> +    static ClientState geolocationClientState();

This is a confusing name, as this is non-client based!

> WebKit/chromium/ChangeLog:9
> +        * public/WebGeolocationServiceMock.h:
> +        * src/AssertMatchingEnums.cpp:

??

> WebKit/chromium/public/WebGeolocationServiceMock.h:47
> +    WEBKIT_API static WebString geolocationClientState();

If we wait for the Chromium non-client-based impl to be deleted, this will go away, right?

> WebKit/chromium/src/AssertMatchingEnums.cpp:395
> +

Intentional?

> WebKitTools/DumpRenderTree/LayoutTestController.h:269
> +    JSRetainPtr<JSStringRef> geolocationClientState() const;

Is it better to use a name that's agnostic to client-based vs non-client-based? WDYT?

> WebKitTools/DumpRenderTree/chromium/LayoutTestController.cpp:1525
> +    WebString clientState = m_shell->webViewHost()->geolocationClientMock()->geolocationClientState();

I don't see the change to GeolocationClientMock?

> WebKitTools/DumpRenderTree/mac/LayoutTestControllerMac.mm:370
> +JSRetainPtr<JSStringRef> LayoutTestController::geolocationClientState() const

I think you'll need to do this for other ports too, no?