[webkit-reviews] review denied: [Bug 21475] Provide support for the Geolocation API : [Attachment 24425] Add missed changes to other build systems

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied 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 24425: Add missed changes to other build systems
https://bugs.webkit.org/attachment.cgi?id=24425&action=edit

------- Additional Comments from Sam Weinig <sam at webkit.org>
+	 * ChangeLog:
This should not be in here.

+#include "config.h"
+#include "JSCustomPositionCallback.h"
+#include "Console.h"
MIssing newline between JSCustomPositionCallback.h and Console.h.

+#include <kjs/JSObject.h>
+#include <kjs/protect.h>
+#include "PositionCallback.h"
+#include <wtf/Forward.h>
This should be sorted.

+#include "config.h"
+#include "JSGeolocation.h"
+#include "DOMWindow.h"
Missing newline.

+JSC::JSValue* toJS(JSC::ExecState* exec, Geolocation* object)
The JSC:: are not needed.

+	 return getDOMObjectWrapper<JSGeolocation>(exec, object);
+    else
+	 return jsUndefined();
The else not needed.

+    int w = m_impl->watchPosition(positionCallback.release(),
positionErrorCallback.release(), positionOptions.get());
The w variable name is a little vague.	Perhaps watchID?

+#include "config.h"
+#include "Geolocation.h"
+#include "PositionError.h"
Missing newline.

+#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>
+
+#include "GeolocationService.h"
+#include "PositionCallback.h"
+#include "PositionErrorCallback.h"
+#include "PositionOptions.h"
+#include "Timer.h"
We usually put the wtf headers last.

+void Geolocation::disconnectFrame()
+{
+    m_frame = 0;
+}
This should probably stop all the watchers and oneShots.

+    RefPtr<PositionError> error = new PositionError(4, "Timed out");
+    m_errorCallback->handleEvent(error.get());
This is leaking the PostionError.  You should use the create idiom for this. 
The handleEvent should probably also take a PassRefPtr.

+    RefPtr<GeoNotifier> notifier = new GeoNotifier(successCallback,
errorCallback, options);
This also leaks

+	     RefPtr<PositionError> error = new PositionError(1, "Unable to
Start");
Here too.

+    RefPtr<GeoNotifier> notifier = m_watchers.get(watchId);
+    if (notifier)
+	 m_watchers.remove(watchId);
There is no reason to call get here.

+    GeoNotifierSet::const_iterator oneShotsEnd = m_oneShots.end();
+    for (GeoNotifierSet::const_iterator it = m_oneShots.begin(); it !=
oneShotsEnd; ++it) {
+	 RefPtr<GeoNotifier> notifier = *it;
+	 
+	 if (notifier->m_errorCallback)
+	     notifier->m_errorCallback->handleEvent(error);
+    }
+    m_oneShots.clear();
+    
+    GeoNotifierMap::const_iterator watchersEnd = m_watchers.end();
+    for (GeoNotifierMap::const_iterator it = m_watchers.begin(); it !=
watchersEnd; ++it) {
+	 RefPtr<GeoNotifier> notifier = (*it).second;
+	 
+	 if (notifier->m_errorCallback)
+	     notifier->m_errorCallback->handleEvent(error);
+    }
These both need to copy the HashSet/HashMap to avoid it being mutated while
iterating it.  Why isn't the watchers map cleared?

+    GeoNotifierSet::const_iterator oneShotsEnd = m_oneShots.end();
+    for (GeoNotifierSet::const_iterator it = m_oneShots.begin(); it !=
oneShotsEnd; ++it) {
Need to copy it here too.

+	     RefPtr<PositionError> error = new PositionError(5, "JavaScript
Error");
This leaks.  Also, the error should probably be a bit more generic and say
something like "An exception was thrown" so it is not JS specific.

+    GeoNotifierMap::const_iterator watchersEnd = m_watchers.end();
+    for (GeoNotifierMap::const_iterator it = m_watchers.begin(); it !=
watchersEnd; ++it) {
Same issues for this too.  You can probably use the copying to a Vector to get
the best efficiency.

+#include <wtf/Platform.h>
Not necessary here.

+#if ENABLE_GEOLOCATION
+	 readonly attribute Geolocation geolocation;
+#else
+	 readonly attribute [DontEnum] Geolocation geolocation;
+#endif
I don't think this will do it.	For instance, I think
navigator.hasProperty("geolocation") or "geolocation" in navigator will still
return true.

+    int code() const { return m_code; }
+    void setCode(int code) { m_code = code; }
+    const String& message() const { return m_message; }
+    void setMessage(const String& message) { m_message = message; }
setCode and setMessage are not needed as the properties are readonly.

+#include "config.h"
+
+#include "GeolocationService.h"
Newline in the wrong place.

+GeolocationService::GeolocationService(GeolocationServiceClient* client)
+    : m_geolocationServiceClient(client)
+{
+}
It would probably be better to assert that you have a client and remove the
null checks.

+class GeolocationServiceClient
+{
Brace should go on previous line.

For GeolocationService, I think a better design would be to have
implementations provide the methods instead of requiring them to subclass by
using pure virtual methods.

+#include "config.h"
+#include "GeolocationServiceFake.h"
+#include "WebCoreGeolocation.h"
Newline.


+    m_lastPosition = new
Geoposition(37.33264,-122.03099,0.0,1.0,0.0,0.0,0.0,1000.0);
Where is this?	Can we make this configurable like
WebCoreShouldUseFakeGeolocation()?

+    GeolocationService::positionChanged();
No prefix needed here.

Do the PositionOptions work if you pass in a vanilla js object?  

Looking great.	r- due to the leaks.