[webkit-reviews] review denied: [Bug 21475] Provide support for the Geolocation API : [Attachment 24267] updated 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 24267: updated other build systems
https://bugs.webkit.org/attachment.cgi?id=24267&action=edit

------- Additional Comments from Sam Weinig <sam at webkit.org>
In addition to the comments we discussed on IRC about abstracting the platform
differences behind an abstraction in WebCore/platform and adding tests, here
are some comments.

I don't think you are using the latest license.  We have a two clause version
that correctly identifies Apple as Apple, Inc. in all places.  You can find an
example of it in ScrollbarTheme.h.

+    if (exec->hadException()) {
+	 m_frame->domWindow()->console()->reportCurrentException(exec);
+	 
+	 raisedException = true;
+    }
Extra newline.

+    virtual void handleEvent(Geoposition*, bool& raisedException);
+private:
+    JSCustomPositionCallback(JSC::JSObject* callback, Frame*);
There should be a newline before private:

+    virtual void handleEvent(PositionError*);
+private:
+    JSCustomPositionErrorCallback(JSC::JSObject* callback, Frame*);
Same here.

+    if (Frame* frame =
toJSDOMWindow(exec->dynamicGlobalObject())->impl()->frame())
+	 positionCallback = JSCustomPositionCallback::create(object, frame);
This should probably use the lexicalGlobalObject.  I don't think any of the
uses of the dynamic global object are correct in the patch and all should
probably be replaced with use of the lexical one.

+Geolocation::Geolocation(Frame* frame)
+    : m_frame(frame)
+{
+
+}
Extra newline.

+    {
+    GeoNotifierSet::const_iterator end = m_oneShots.end();
The { seems only scope the for loops differently so they can use the same
variable names.  I think a better strategy would be to make the names more
verbose (oneShotsEnd, and watchesEnd).

+	 void timerFired(Timer<GeoNotifier>* timer);
No need to include parameter name.

+	 RefPtr<PositionCallback>	 m_successCallback;
+	 RefPtr<PositionErrorCallback>	 m_errorCallback;
+	 Timer<GeoNotifier>		 m_timer;
We usually don't line up the variable names like this. 

+    static PassRefPtr<Geolocation> create(Frame* frame);
No need to include parameter name.

+    void clearWatch(long watchId);
I think this should be an int.	long in IDL translates to int in cpp.

+    void handleNewPosition(PassRefPtr<Geoposition> newPosition);
+    void handleError(PositionError* error);

+    Geolocation(Frame* frame);

+    virtual bool startUpdating(PositionOptions* options) = 0;
No need to include parameter names,

+    typedef HashSet< RefPtr<GeoNotifier> > GeoNotifierSet;
Extra space before RefPtr.

+    typedef HashMap<long, RefPtr<GeoNotifier> > GeoNotifierMap;
Again, probably int instead of long.

+#include <wtf/Platform.h>
+#include <wtf/RefCounted.h>
+#include "PlatformString.h"
These should be sorted.

+	 , m_timestamp(timestamp) 
+    {}
+    
The braces should go on separate lines.

+    unsigned long long timestamp() const { return m_timestamp; }
We typedef DOMTypeStamp in Event.h.  You can use that for this.

+	 DOMString toString();
Though I did not look at the spec, I suspect this should be [DontEnum].

+#if PLATFORM(IPHONE)
+    if (!m_geolocation)
+	 m_geolocation = Geolocation::create(m_frame);
+    return m_geolocation.get();
+#else
+    return NULL;
+#endif
Clearly, +#if PLATFORM(IPHONE) is wrong here.  I think we should introduce
ENABLE(GEOLOCATION).  Also, the NULL should just be 0.	Just returning 0 here
is really not enough because the geolocation property is still enumerable and
will still return true for hasProperty and the corresponding 'geolocation' in
window.navigator feature checks.  I agree with Antti (he said this on irc) that
there are good reasons to want to compile this feature to ensure it doesn't die
of code rot, but I think it needs to not be detectable.

It looks like the Geolocation is not getting marked by its parent.  I assumed
the JSPluginArray and JSMimeTypeArray are having the same problem, but we
should follow the paradigm set in DOMWindow and add optionalGeolocation methods
and CustomMark to JSNavigator.

+	 virtual void handleEvent(Geoposition* position, bool& raisedException)
= 0;
position parameter name is not needed.

+#include <wtf/Platform.h>
+#include <wtf/RefCounted.h>
+#include "PlatformString.h"
Please sort.

+	 , m_message(message)
+    {}
+    
Braces should go on separate lines.

+    long code() const { return m_code; }
+    void setCode(long code) { m_code = code; }

+    long m_code;
Again, long == int.