Bug 28264 - Add Mock Geolocation service for use in LayoutTests
: Add Mock Geolocation service for use in LayoutTests
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
: 27255 27256 27944
  Show dependency treegraph
 
Reported: 2009-08-13 10:03 PST by
Modified: 2009-09-07 23:14 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-08-13 10:03:50 PST
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 From 2009-08-13 10:05:41 PST -------
Created an attachment (id=34754) [details]
Patch 1 for bug 28264

Adds a mock Geolocation service.
------- Comment #2 From 2009-08-19 15:54:14 PST -------
Created an attachment (id=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 From 2009-09-01 16:00:56 PST -------
(From update of attachment 35155 [details])
+    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 From 2009-09-02 03:33:57 PST -------
> 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 From 2009-09-04 12:57:46 PST -------
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 From 2009-09-04 12:59:20 PST -------
*** Bug 21717 has been marked as a duplicate of this bug. ***
------- Comment #7 From 2009-09-07 03:25:24 PST -------
Created an attachment (id=39137) [details]
Patch 3 for bug 28264

(From update of attachment 35155 [details] [details])
> +    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 From 2009-09-07 09:00:49 PST -------
(From update of attachment 39137 [details])
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 From 2009-09-07 09:39:41 PST -------
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 From 2009-09-07 14:27:21 PST -------
Created an attachment (id=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 From 2009-09-07 15:07:23 PST -------
(From update of attachment 39162 [details])
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 From 2009-09-07 15:55:21 PST -------
Created an attachment (id=39166) [details]
Patch 5 for bug 28264

WebCore.pro fixed
------- Comment #13 From 2009-09-07 16:00:31 PST -------
(From update of attachment 39166 [details])
Great!  This is ready to land once the buildbots are functional again.
------- Comment #14 From 2009-09-07 18:03:44 PST -------
(From update of attachment 39166 [details])
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 From 2009-09-07 18:16:58 PST -------
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 From 2009-09-07 22:38:05 PST -------
I've resolved the conflicts locally.  Running pre-submit scripts now.
------- Comment #17 From 2009-09-07 23:14:05 PST -------
Committed r48144: <http://trac.webkit.org/changeset/48144>