[Webkit-unassigned] [Bug 40002] Need Geolocation LayoutTest to test case where permission has neither been granted nor denied

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=40002


Alexey Proskuryakov <ap at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #58033|review?                     |review-
               Flag|                            |




--- Comment #5 from Alexey Proskuryakov <ap at webkit.org>  2010-06-07 08:56:38 PST ---
(From update of attachment 58033)
+        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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.