Bug 28264 - Add Mock Geolocation service for use in LayoutTests
Summary: Add Mock Geolocation service for use in LayoutTests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 21717 (view as bug list)
Depends on:
Blocks: 27255 27256 27944
  Show dependency treegraph
 
Reported: 2009-08-13 10:03 PDT by Steve Block
Modified: 2009-09-07 23:14 PDT (History)
6 users (show)

See Also:


Attachments
Patch 1 for bug 28264 (23.80 KB, patch)
2009-08-13 10:05 PDT, Steve Block
no flags Details | Formatted Diff | Diff
Patch 2 for bug 28264 (50.52 KB, patch)
2009-08-19 15:54 PDT, Steve Block
abarth: review-
Details | Formatted Diff | Diff
Patch 3 for bug 28264 (51.26 KB, patch)
2009-09-07 03:25 PDT, Steve Block
abarth: review-
Details | Formatted Diff | Diff
Patch 4 for bug 28264 (62.93 KB, patch)
2009-09-07 14:27 PDT, Steve Block
abarth: review+
abarth: commit-queue-
Details | Formatted Diff | Diff
Patch 5 for bug 28264 (62.86 KB, patch)
2009-09-07 15:55 PDT, Steve Block
abarth: review+
abarth: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Steve Block 2009-08-13 10:03:50 PDT
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.
Comment 1 Steve Block 2009-08-13 10:05:41 PDT
Created attachment 34754 [details]
Patch 1 for bug 28264

Adds a mock Geolocation service.
Comment 2 Steve Block 2009-08-19 15:54:14 PDT
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?
Comment 3 Adam Barth 2009-09-01 16:00:56 PDT
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.
Comment 4 Steve Block 2009-09-02 03:33:57 PDT
> 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?
Comment 5 Adam Barth 2009-09-04 12:57:46 PDT
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.
Comment 6 Adam Barth 2009-09-04 12:59:20 PDT
*** Bug 21717 has been marked as a duplicate of this bug. ***
Comment 7 Steve Block 2009-09-07 03:25:24 PDT
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.
Comment 8 Adam Barth 2009-09-07 09:00:49 PDT
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.
Comment 9 Adam Barth 2009-09-07 09:39:41 PDT
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.
Comment 10 Steve Block 2009-09-07 14:27:21 PDT
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.
Comment 11 Adam Barth 2009-09-07 15:07:23 PDT
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.
Comment 12 Steve Block 2009-09-07 15:55:21 PDT
Created attachment 39166 [details]
Patch 5 for bug 28264

WebCore.pro fixed
Comment 13 Adam Barth 2009-09-07 16:00:31 PDT
Comment on attachment 39166 [details]
Patch 5 for bug 28264

Great!  This is ready to land once the buildbots are functional again.
Comment 14 Adam Barth 2009-09-07 18:03:44 PDT
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.
Comment 15 Adam Barth 2009-09-07 18:16:58 PDT
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
Comment 16 Adam Barth 2009-09-07 22:38:05 PDT
I've resolved the conflicts locally.  Running pre-submit scripts now.
Comment 17 Adam Barth 2009-09-07 23:14:05 PDT
Committed r48144: <http://trac.webkit.org/changeset/48144>