[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 #6 from Steve Block <steveblock at google.com>  2010-11-22 08:16:51 PST ---
(From update of attachment 74242)
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

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