[webkit-reviews] review granted: [Bug 21475] Provide support for the Geolocation API : [Attachment 24463] updated

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Greg Bolsinga <bolsinga at apple.com>'s
request for review:
Bug 21475: Provide support for the Geolocation API
https://bugs.webkit.org/show_bug.cgi?id=21475

Attachment 24463: updated
https://bugs.webkit.org/attachment.cgi?id=24463&action=edit

------- Additional Comments from Sam Weinig <sam at webkit.org>
r=me.  There are a bunch of comments below, but I think we have gone back and
forth enough to land this patch.  I would encourage you to make all the
changes, but you can use your discretion.

> +    // should this be on the Frame?
No, and this comment should go away.

> +void Geolocation::sendErrorToOneShots(PositionError* error)
> +{
> +    const GeoNotifierSet copy = m_oneShots;

I think it is more efficient to copy this to a Vector and iterate the vector. 
There is a function call copyToVector(const HashSet<>, Vector<>) that you can
use.

> +void Geolocation::sendErrorToWatchers(PositionError* error)
> +{
> +    const GeoNotifierMap copy = m_watchers;

Same here.  Since you only need the hash values, you can use
copyValuesToVector().

> +void Geolocation::sendPositionToOneShots(Geoposition* position)
> +{
> +    const GeoNotifierSet copy = m_oneShots;

And here.

> +
> +void Geolocation::sendPositionToWatchers(Geoposition* position)
> +{
> +    const GeoNotifierMap copy = m_watchers;

Once again

> +#include "GeolocationService.h"
> +#include "PositionCallback.h"
> +#include "PositionErrorCallback.h"
> +#include "PositionOptions.h"
> +#include "Timer.h"
> +
> +#include <wtf/Platform.h>
> +#include <wtf/HashMap.h>
> +#include <wtf/HashSet.h>
> +#include <wtf/OwnPtr.h>
> +#include <wtf/PassRefPtr.h>
> +#include <wtf/RefCounted.h>
> +#include <wtf/RefPtr.h>
These don't have to be paragraphed.

> +
> +    interface [
> +	   GenerateConstructor
> +    ] Geolocation {
If this is generating a constructor, you need to update DOMWindow.idl (with the
requisite #ifdef) to add the constructor there too.

> +    interface [
> +	   GenerateConstructor
> +    ] Geoposition {
Same deal with the constructor.


> +
> +	   [DontEnum] DOMString toString();
We usually #ifdef these only for JS.

>  
>	   bool onLine() const;
> +	   Geolocation* geolocation() const;
> +	   // This is used for GC marking.
> +	   Geolocation* optionalGeolocation() const { return
m_geolocation.get(); }
>      private:
Need a newline before private.

> +class PositionError : public RefCounted<PositionError> {
> +public:
> +    enum ErrorCode {
> +	   PERMISSION_ERROR = 1,
> +	   LOCATION_PROVIDER_ERROR,
> +	   POSITION_NOT_FOUND_ERROR,
> +	   TIMEOUT_ERROR,
> +	   UNKNOWN_ERROR
> +    };

Perhaps we should give feedback to the WG that the error codes should have
constants?

> +
> +    class PositionErrorCallback : public RefCounted<PositionErrorCallback> {

> +    public:
> +	   virtual ~PositionErrorCallback() { }
> +	   virtual void handleEvent(PositionError* position) = 0;
No need for the parameter name here.  

> +
> +    bool enableHighAccuracy() const { return m_highAccuracy; }
> +    void setEnableHighAccuracy(bool enable) { m_highAccuracy = enable; }
> +    long timeout() const { return m_timeout; }
> +    void setTimeout(long t) { m_timeout = t; }
Should be ints.

Are the set methods ever used?

> +    
> +    bool m_highAccuracy;
> +    long m_timeout;
Should be an int.

> +
> +class GeolocationService : public Noncopyable {
> +public:
> +    static GeolocationService* create(GeolocationServiceClient*);
> +    virtual ~GeolocationService() {}
> +    
> +    virtual bool startUpdating(PositionOptions*) = 0;
> +    virtual void stopUpdating() = 0;
> +    
> +    virtual Geoposition* lastPosition() const = 0;
> +    virtual PositionError* lastError() const = 0;

Since we are not doing the FakeService anymore, I don't think these need to be
virtual at all.