[webkit-reviews] review denied: [Bug 28264] Add Mock Geolocation service for use in LayoutTests : [Attachment 35155] Patch 2 for bug 28264

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied steveblock at google.com's request for
review:
Bug 28264: Add Mock Geolocation service for use in LayoutTests
https://bugs.webkit.org/show_bug.cgi?id=28264

Attachment 35155: Patch 2 for bug 28264
https://bugs.webkit.org/attachment.cgi?id=35155&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
+    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.