Bug 39081 – [chromium] Adds supports for layout tests using GeolocationServiceMock

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Marcus Bulach]

Adds supports for layout tests using GeolocationServiceMock.
Allows injection of GeolocationServiceMock factory.

[Marcus Bulach]

Created attachment 56011 [details]
Patch

[Marcus Bulach]

hi steve, sorry to keep bothering you, but would you mind taking a look at this patch? take your time, this is low priority.. ;) 
thanks!

[Steve Block]

Comment on attachment 56011 [details]
Patch

Can you explain why this is needed? As far as I can see, GeolocationServiceChromiumMock is an intermediate that sits between the existing GeolocationServiceMock and the Geolocation object. Does this class exist solely to add the extra calls to GeolocationServiceChromium::startUpdating()/stopUpdating()?

It looks like GeolocationServiceChromiumMock is mostly boilerplate - could it be reduced by having it inherit from GeolocationServiceMock, rather than owning an instance?

> diff --git a/WebCore/platform/GeolocationService.cpp b/WebCore/platform/GeolocationService.cpp
> +void GeolocationService::useMockFactory(FactoryFunction f)
> +{
> +    s_mockFactoryFunction = f;
> +    useMock();
>  }
So we need to call useMock() here, as it's called from setPosition and setError? Would a better name for this function then be 'setCustomMock'?

[Marcus Bulach]

Created attachment 56071 [details]
Patch

[Marcus Bulach]

thanks for the quick reply, Steve!
replies inline:

(In reply to comment #3)
> (From update of attachment 56011 [details])
> Can you explain why this is needed? As far as I can see, GeolocationServiceChromiumMock is an intermediate that sits between the existing GeolocationServiceMock and the Geolocation object. Does this class exist solely to add the extra calls to GeolocationServiceChromium::startUpdating()/stopUpdating()?
> 
> It looks like GeolocationServiceChromiumMock is mostly boilerplate - could it be reduced by having it inherit from GeolocationServiceMock, rather than owning an instance?

sorry, I tried to keep the minimalistic comment style here, but obviously it needs some clarifications.. :) let me know what's the best way to add (ChangeLog? inline comments?)
anyways: there are extensive comments on the other side:
http://codereview.chromium.org/2094003/diff/5001/6001

basically: there are a few layers up in the embedder that require a GeolocationServiceChromium (for getBridgeId() / attachBridgeIfNeeded).

hence, the simplest way seemed to extend that expected object/interface, and delegate the calls to GeolocationServiceMock as appropriate..

so yes, it's just an intermediary proxy that sits between Geolocation and the embedder. it's all boiler plate code to adapt what the embedder expects to what the mock provides, and to inject in a Geolocation object.

> 
> > diff --git a/WebCore/platform/GeolocationService.cpp b/WebCore/platform/GeolocationService.cpp
> > +void GeolocationService::useMockFactory(FactoryFunction f)
> > +{
> > +    s_mockFactoryFunction = f;
> > +    useMock();
> >  }
> So we need to call useMock() here, as it's called from setPosition and setError? Would a better name for this function then be 'setCustomMock'?

renamed to setCustomMockFactory, and moved the "useMock()" call from here to higher up.

[Steve Block]

Comment on attachment 56071 [details]
Patch

> so yes, it's just an intermediary proxy that sits between Geolocation and the embedder. it's all boiler plate code to adapt
>  what the embedder expects to what the mock provides, and to inject in a Geolocation object.
OK, got it, thanks

WebKit/chromium/src/WebGeolocationServiceMock.cpp:124
 +  void WebGeolocationServiceMock::useGeolocationServiceMock()
Do you need this method? The existing GeolocationServiceMock simply calls GeolocationService::useMock() from setPosition() and setError(). Could we do similarly and call setCustomMockFactory() from setMockGeolocationPosition() and setMockGeolocationError()? Is the additional call to GeolocationService::useMock() required?

[Marcus Bulach]

Created attachment 56363 [details]
Patch

[Marcus Bulach]

(In reply to comment #6)
> (From update of attachment 56071 [details])
> > so yes, it's just an intermediary proxy that sits between Geolocation and the embedder. it's all boiler plate code to adapt
> >  what the embedder expects to what the mock provides, and to inject in a Geolocation object.
> OK, got it, thanks
> 
> WebKit/chromium/src/WebGeolocationServiceMock.cpp:124
>  +  void WebGeolocationServiceMock::useGeolocationServiceMock()
> Do you need this method? The existing GeolocationServiceMock simply calls GeolocationService::useMock() from setPosition() and setError(). Could we do similarly and call setCustomMockFactory() from setMockGeolocationPosition() and setMockGeolocationError()? Is the additional call to GeolocationService::useMock() required?

hehe, yeah, tricky trade off between less API surface versus number of calls..

anyways: followed your suggestions, removed the method, and added calls to setCustomMockFactory() on both setMockGeolocationPosition/Error.

another look please?

[WebKit Commit Bot]

Comment on attachment 56363 [details]
Patch

Clearing flags on attachment: 56363

Committed r59772: <http://trac.webkit.org/changeset/59772>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/59772 might have broken Qt Windows 32-bit Release