Bug 28264 – Add Mock Geolocation service for use in LayoutTests

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Steve Block]

To allow proper testing of the Geolocation API, we need a mock implementation of the Geolocation service which provides predictable behavior of the API. The mock Geolocation service will be integrated with DumpRenderTree so that it can be controlled from LayoutTests.

This was discussed in https://bugs.webkit.org/show_bug.cgi?id=27716, which added the first Geolocation LayoutTests.

[Steve Block]

Created attachment 34754 [details]
Patch 1 for bug 28264

Adds a mock Geolocation service.

[Steve Block]

Created attachment 35155 [details]
Patch 2 for bug 28264

This new patch also adds the hooks for DumpRenderTree on Mac, and the first layout test to make use of the mock service.

Could somebody please take a look?

[Adam Barth]

Comment on attachment 35155 [details]
Patch 2 for bug 28264

+    static FactoryFunction* s_factoryFunction;

Usually we just declare these statically in the cpp file instead of declaring them in the h file.

+++ WebCore/platform/MockGeolocationService.cpp	(revision 0)

Does WebCore have other Mock classes?  This should probably live in WebKitTools/DumpRenderTree

What's the benefit of holding all that data in the Mock class as statics versus just holding them as members of the instance?

+ GeolocationService::FactoryFunction* GeolocationService::s_factoryFunction = &GeolocationServiceMac::create;

I don't think this a very common pattern in WebCore...  I there must be a better way to create this Mock service.

+ #import <WebKit/WebMockGeolocation.h>

This probably need to be a private API header.  Otherwise, the header will need to go through MacOS API review.

I'm not really sure what the best approach is here.  We should consult with some WebCore/platform experts on the best way to test against mock platform features.

[Steve Block]

> Does WebCore have other Mock classes?  This should probably live in
> WebKitTools/DumpRenderTree
I don't see any other mocks in WebCore. Is there a pattern for mocks in DumpRenderTree that I can follow?

> What's the benefit of holding all that data in the Mock class as statics versus
> just holding them as members of the instance?
The GeolocationService instance is created when the page's Geolocation instance is created. This is created lazily in Navogator::Geolocation() when navigator.geolocation is first accessed. So if we want to use MockGeolocationService, we have to configure WebCore to do so before navigator.geolocation is accessed. So GeolocationService::useMock method must be static.

The advantage of making the lastPosition and lastError members of MockGeolocationService static, is that it allows us to easily configure MockGeolocationService when GeolocationService::useMock is called.

Alternatively, LayoutTestController.setMockGeolocationPosition could call GeolocationService::useMock, then create the page's Geolocation object (if it didn't already exist), then configure its MockGeolocationService. This would require extra plumbing to allow the LayoutTestController to get the Navigator object for the current frame, and modifications to Geolocation to allow the LayoutTestController to get the GeolocationService it creates.

This is possible, but seemed like a less neat solution, given the changes required.

> I'm not really sure what the best approach is here.  We should consult with
> some WebCore/platform experts on the best way to test against mock platform
> features.
OK, I'll ask on #webkit. Do you have any suggestions as to who to approach?

[Adam Barth]

Ok.  It looks like no one on webkit-dev is objecting to creating a mock version of the service for testing.  Here's what I think we should do:

1) Make a new "platform" directory named "mock" or "testing" or some such.

2) Make a GeolocationServiceMock or GeolocationServiceTesting class in that folder.

3) The mac WebKit APIs seem fine, but we need to make these Private to avoid impacting the OS X webkit APIs.  You should be able to find examples of this.

[Adam Barth]

*** Bug 21717 has been marked as a duplicate of this bug. ***

[Steve Block]

Comment on attachment 35155 [details]
Patch 2 for bug 28264

> +    static FactoryFunction* s_factoryFunction;
> 
> Usually we just declare these statically in the cpp file instead of declaring
> them in the h file.
This static member needs to be accessed outside of GeolocationService.cpp. See GeolocationServiceMac.mm etc. It's the equivalent of the former GeolocationService::create method.

Do you want the static members of GeolocationServiceMock to be moved to non-member static variables in the cpp file too?

> + #import <WebKit/WebMockGeolocation.h>
> 
> This probably need to be a private API header.  Otherwise, the header will need
> to go through MacOS API review.
I've renamed this WebGeolocationMockPrivate and set the Xcode role to 'Private'. Is there anything more to it than this?
 
> 1) Make a new "platform" directory named "mock" or "testing" or some such.
> 
> 2) Make a GeolocationServiceMock or GeolocationServiceTesting class in that
> folder.
Done

> *** Bug 21717 has been marked as a duplicate of this bug. ***FWIW, I'm not sure that Greg intended Bug 21717 to be equivalent to this one. I think his intent was to create a mock Geolocation service for testing on platforms where the platform _doesn't_ have an implementation. Personally, I don't think that's necessary, so am happy to see the bug closed.

[Adam Barth]

Comment on attachment 39137 [details]
Patch 3 for bug 28264

This looks great.  Two minor issues:

+++ WebCore/WebCore.base.exp

Please keep this file alphabetical.

The project files don't seem to know about the mock directory.  I suspect this will cause build breaks.

[Adam Barth]

A couple more points:

1) How does this patch link on non-mac platforms?  You don't seem to have implemented the new layoutTestController methods on the other platforms.

2) This test probably fails on non-mac platforms.  You probably want to add it to the Skipped files to avoid turning the buildbot red.

[Steve Block]

Created attachment 39162 [details]
Patch 4 for bug 28264

> +++ WebCore/WebCore.base.exp
> 
> Please keep this file alphabetical.
Fixed

> The project files don't seem to know about the mock directory.  I suspect this
> will cause build breaks.
Fixed. Updated WebCore.pro, GNUmakefile.am and WebCoreCommon.vsprops.

> 1) How does this patch link on non-mac platforms?  You don't seem to have
> implemented the new layoutTestController methods on the other platforms.
I've added stub implementations for non-mac platforms (gtk, win and wx). Presumably I'm not required
to write the implementations for all platforms?

> 2) This test probably fails on non-mac platforms.  You probably want to add it
> to the Skipped files to avoid turning the buildbot red.
All Geolocation tests are currently skipped for all platforms other than gtk. I've
added the new test to the skipped list for gtk given the lack of implementation of
the LayoutTestcontroller methods.

I haven't been able to test on Windows as the build is currently broken, but will do so tomorrow.

[Adam Barth]

Comment on attachment 39162 [details]
Patch 4 for bug 28264

WebCore.pro is missing the "mock" directory for the header.  Other than that, this looks good to go.  Thanks for your patience.

I'm marking as commit-queue- because of the WebCore.pro typo.  Someone can either land this manually, or you can upload a new version with the typo fixed.

The buildbots seem to be a bit underwater at the moment, so I don't want to attempt to land this right now.

[Steve Block]

Created attachment 39166 [details]
Patch 5 for bug 28264

WebCore.pro fixed

[Adam Barth]

Comment on attachment 39166 [details]
Patch 5 for bug 28264

Great!  This is ready to land once the buildbots are functional again.

[Adam Barth]

Comment on attachment 39166 [details]
Patch 5 for bug 28264

Rejecting patch 39166 from commit-queue.  This patch will require manual commit.

Patch https://bugs.webkit.org/attachment.cgi?id=39166 from bug 28264 failed to download and apply.

[Adam Barth]

patching file WebCore/WebCore.vcproj/WebCore.vcproj
Hunk #1 FAILED at 17068.
Hunk #2 FAILED at 21759.
2 out of 2 hunks FAILED -- saving rejects to file WebCore/WebCore.vcproj/WebCore.vcproj.rej

[Adam Barth]

I've resolved the conflicts locally.  Running pre-submit scripts now.

[Adam Barth]

Committed r48144: <http://trac.webkit.org/changeset/48144>