[webkit-reviews] review denied: [Bug 40002] Need Geolocation LayoutTest to test case where permission has neither been granted nor denied : [Attachment 58033] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Steve Block
<steveblock at google.com>'s request for review:
Bug 40002: Need Geolocation LayoutTest to test case where permission has
neither been granted nor denied
https://bugs.webkit.org/show_bug.cgi?id=40002

Attachment 58033: Patch
https://bugs.webkit.org/attachment.cgi?id=58033&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
+	 assert(m_pendingGeolocationPermissionListener == nil);

Should be "ASSERT(!m_pendingGeolocationPermissionListener)".

+    if (m_pendingGeolocationPermissionListener == nil)

Should be "if (!m_pendingGeolocationPermissionListener)".

+    if (allow)
+	 [m_pendingGeolocationPermissionListener allow];
+    else
+	 [m_pendingGeolocationPermissionListener deny];
+
+    [m_pendingGeolocationPermissionListener release];
+    m_pendingGeolocationPermissionListener = nil;

This calls out from DRT code, so UIDelegate may be released in the meanwhile.
I'd use a local variable for the listener.

+    if (!_timer)
+	 _timer = [NSTimer scheduledTimerWithTimeInterval:0 target:self
selector:@selector(timerFired) userInfo:0 repeats:NO];

I don't understand this change. Ideally, the allow/deny callback should be
completely asynchronous (and not dispatched from setGeolocationPermission()
directly), but this change doesn't give us that. Is this what you refer to as
"minor fix" in the ChangeLog?

+- (void)setGeolocationPermission:(bool)allow

It's somewhat confusing to have a function with such a generic name in UI
delegate. Something like "setMockGeolocationOErmission" can help avoid the
confusion.

+    // FIXME: Implement for Geolocation layout tests.

Does the new test need to be skipped on Gtk?

I think that this could use another review round, especially if you decide to
implement true asynchronicity.