Bug 49258 - Implement Mocks for Client-based Geolocation
Summary: Implement Mocks for Client-based Geolocation
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
: 43925 (view as bug list)
Depends on: 49259
Blocks: 49438 49734 46895
  Show dependency treegraph
Reported: 2010-11-09 09:45 PST by John Knottenbelt
Modified: 2010-11-25 05:18 PST (History)
4 users (show)

See Also:

Patch (17.11 KB, patch)
2010-11-10 04:04 PST, John Knottenbelt
no flags Details | Formatted Diff | Diff
Patch (17.32 KB, patch)
2010-11-18 08:46 PST, John Knottenbelt
no flags Details | Formatted Diff | Diff
Patch (17.36 KB, patch)
2010-11-22 09:20 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-09 09:45:23 PST
Provide a mock implementation for WebCore::GeolocationClient
Comment 1 John Knottenbelt 2010-11-10 04:04:35 PST
Created attachment 73484 [details]
Comment 2 Steve Block 2010-11-11 04:22:24 PST
Comment on attachment 73484 [details]

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
Comment 3 John Knottenbelt 2010-11-18 08:46:26 PST
Created attachment 74242 [details]
Comment 4 Alexey Proskuryakov 2010-11-18 11:02:58 PST
I still think that having mock objects in WebCore is a strategically wrong approach.
Comment 5 John Knottenbelt 2010-11-19 08:03:05 PST
(In reply to comment #4)
> I still think that having mock objects in WebCore is a strategically wrong approach.

For those who are not familiar with the thread on webkit-dev, here is a link

After all embedders have switched to using client-based geolocation ( https://bugs.webkit.org/show_bug.cgi?id=40373 ), we will be able to remove WebCore/platform/mock/GeolocationServiceMock.cpp altogther, so the net result will be to replace GeolocationServiceMock with GeolocationClientMock. 

As mentioned in the thread, I do think it is a good idea for the mocks to be compiled out of WebCore in production code. I have created bug https://bugs.webkit.org/show_bug.cgi?id=49806 and will raise this topic for discussion on webkit-dev when I come to look at it.
Comment 6 Steve Block 2010-11-22 08:16:51 PST
Comment on attachment 74242 [details]

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

> WebCore/platform/mock/GeolocationClientMock.cpp:34

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
Comment 7 John Knottenbelt 2010-11-22 09:20:47 PST
Created attachment 74562 [details]
Comment 8 John Knottenbelt 2010-11-22 09:22:05 PST
Comment on attachment 74562 [details]

Thanks for the review, I have made the suggested changes.
Comment 9 WebKit Commit Bot 2010-11-22 20:43:36 PST
Comment on attachment 74562 [details]

Clearing flags on attachment: 74562

Committed r72581: <http://trac.webkit.org/changeset/72581>
Comment 10 WebKit Commit Bot 2010-11-22 20:43:42 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Steve Block 2010-11-25 05:18:19 PST
*** Bug 43925 has been marked as a duplicate of this bug. ***