Adds supports for layout tests using GeolocationServiceMock. Allows injection of GeolocationServiceMock factory.
Created attachment 56011 [details] Patch
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!
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'?
Created attachment 56071 [details] Patch
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.
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?
Created attachment 56363 [details] Patch
(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?
Comment on attachment 56363 [details] Patch Clearing flags on attachment: 56363 Committed r59772: <http://trac.webkit.org/changeset/59772>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/59772 might have broken Qt Windows 32-bit Release