Bug 49438 - Add LayoutTests to test high accuracy mode in Geolocation
Summary: Add LayoutTests to test high accuracy mode in Geolocation
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
Depends on: 49258
  Show dependency treegraph
Reported: 2010-11-12 03:51 PST by John Knottenbelt
Modified: 2011-02-15 04:41 PST (History)
6 users (show)

See Also:

Patch (21.72 KB, patch)
2010-12-09 10:17 PST, John Knottenbelt
no flags Details | Formatted Diff | Diff
Patch (20.44 KB, patch)
2010-12-15 08:10 PST, John Knottenbelt
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Knottenbelt 2010-11-12 03:51:39 PST
We need additional LayoutTests to test that setEnableHighAccuracy is being invoked correctly on the GeolocationClient.
Comment 1 John Knottenbelt 2010-12-09 10:17:03 PST
Created attachment 76085 [details]

Draft patch to get feedback on general approach.
Comment 2 Steve Block 2010-12-10 06:02:04 PST
Comment on attachment 76085 [details]

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.
Comment 3 John Knottenbelt 2010-12-15 08:10:52 PST
Created attachment 76647 [details]
Comment 4 John Knottenbelt 2010-12-15 08:12:40 PST
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
Comment 5 WebKit Review Bot 2010-12-15 09:29:31 PST
Attachment 76647 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7173028
Comment 6 Build Bot 2010-12-15 12:56:54 PST
Attachment 76647 [details] did not build on win:
Build output: http://queues.webkit.org/results/7154037
Comment 7 Steve Block 2011-01-06 07:23:05 PST
Comment on attachment 76647 [details]

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


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