[Webkit-unassigned] [Bug 49438] Add LayoutTests to test high accuracy mode in Geolocation

[bugzilla-daemon at webkit.org]

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





--- Comment #7 from Steve Block <steveblock at google.com>  2011-01-06 07:23:05 PST ---
(From update of attachment 76647)
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?

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