[webkit-reviews] review requested: [Bug 28264] Add Mock Geolocation service for use in LayoutTests : [Attachment 39137] Patch 3 for bug 28264

[bugzilla-daemon at webkit.org]

steveblock at google.com has asked  for review:
Bug 28264: Add Mock Geolocation service for use in LayoutTests
https://bugs.webkit.org/show_bug.cgi?id=28264

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

------- Additional Comments from steveblock at google.com
(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.
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.