Bug 40002 – Need Geolocation LayoutTest to test case where permission has neither been granted nor denied

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Steve Block]

We need a Geolocation LayoutTest to test the case where a position is available but the embedder has not yet either granted or denied permission. In this case, no callback should be made.

[Steve Block]

Created attachment 57553 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 57553 [details]
Patch

I guess I'm missing something, but I don't see how this test can work. All LayoutTestController::setGeolocationPermission() does is set a member variable, how does this result in async callback?

[Steve Block]

(In reply to comment #2)
> (From update of attachment 57553 [details])
> I guess I'm missing something, but I don't see how this test can work.
Sorry, I was testing with the Android LayoutTestController, which isn't upstream, and forgot to update the common LayoutTestController. Updating patch coming.

[Steve Block]

Created attachment 58033 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 58033 [details]
Patch

+        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.

[Steve Block]

Thanks for the review Alexey.

> This calls out from DRT code, so UIDelegate may be released in the meanwhile.
> I'd use a local variable for the listener.
I'm not sure I understand. When would the UI delegate be released while script is still running?

> +    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?
This change is the "minor fix". Previously, the MockGeolocationProvider didn't trigger a position update to the WebView when it registered - only when a new mock position was set. It relied on the fact that the WebView was registered synchronously along with the call to set the mock position. So by the time the MockGeolocationProvider's timer fired, the Webview was registered and it would be sent the position. Now that the permission is granted asynchronously, the WebView is also added asynchronously (Geolocation does not call startUpdating() until permission is granted). So there's a race condition and the timer may fire before the WebView is registered. This change makes sure that the WebView gets the position when it registers.

I think the above change is independent of whether the allow/deny callback is completely asynchronous. Is there a particular need for it to be completely asynchronous?

[Alexey Proskuryakov]

> > This calls out from DRT code, so UIDelegate may be released in the meanwhile.
> > I'd use a local variable for the listener.
> I'm not sure I understand. When would the UI delegate be released while script is still running?

I haven't looked into the details, maybe WebKit can navigate away from the current page during this callback. It's always best not to rely on behavior of delegate calls made into different projects.

> I think the above change is independent of whether the allow/deny callback is completely 
> asynchronous. Is there a particular need for it to be completely asynchronous?

Nothing particular I can think of, just matching "real life" code path better.

> This change is the "minor fix".

OK. It would be nice to say a few words about this in ChangeLog.

[Steve Block]

Created attachment 62699 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 62699 [details]
Patch

+    [[[mainFrame webView] UIDelegate] didSetGeolocationPermission];

I'd still prefer this to be called didSetMockGeolocationPermission (same for a few other names, of course) . It's hard to understand the various permissions here, so being specific helps.

+    HashSet<id<WebGeolocationPolicyListener> > m_pendingGeolocationPermissionListeners;

This code is fine in the short run, but HashSet isn't a great data structure to keep Objective-C objects in. Specifically, we'll get problems if we try to run DumpRenderTree in garbage collected mode.  NSMutableSet is the easiest way to make the code more clean.

r=me, but please consider addressing these comments.

[Darin Adler]

(In reply to comment #9)
> NSMutableSet is the easiest way to make the code more clean.

Or perhaps NSMapTable.

[Steve Block]

Created attachment 63340 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 63340 [details]
Patch

+#import <Foundation/NSSet.h>

This isn't needed, please don't add this line.

+    if (m_pendingGeolocationPermissionListeners != nil && !m_timer)

We don't compare to null, so this can be just (m_pendingGeolocationPermissionListeners && !m_timer)

+        if (!m_pendingGeolocationPermissionListeners)
+            m_pendingGeolocationPermissionListeners = [[NSMutableSet set] retain];

I see that m_pendingGeolocationPermissionListeners is released in timerFired, but shouldn't it be also released in dealloc method - in case the timer doesn't get a chance to fire?

+    NSEnumerator* enumerator = [m_pendingGeolocationPermissionListeners objectEnumerator];

Should we be iterating a copy? It seems that a client might add listeners when handling allow/deny.

r=me

[Steve Block]

> This isn't needed, please don't add this line.
Will fix

> We don't compare to null, so this can be just (m_pendingGeolocationPermissionListeners && !m_timer)
Will fix

> I see that m_pendingGeolocationPermissionListeners is released in timerFired, but shouldn't it be also released in dealloc method - in case the timer doesn't get a chance to fire?
Yes, will fix

> Should we be iterating a copy? It seems that a client might add listeners when handling allow/deny.
No, because listeners are only added before permission has been set. But when the timer fires, the permission must have already been set, as in the assertion above

[Steve Block]

Created attachment 63353 [details]
Patch

[Steve Block]

Comment on attachment 63353 [details]
Patch

New patch to check build on EWS bots.

[Steve Block]

Comment on attachment 63353 [details]
Patch

Minor changes only since Alexey's r+

[Steve Block]

Comment on attachment 63353 [details]
Patch

Giving up on commit queue, which seems to be having problems. Will land manually.

[Steve Block]

Landed manually as http://trac.webkit.org/changeset/64639

Closing as resolved fixed.

[WebKit Review Bot]

http://trac.webkit.org/changeset/64639 might have broken Qt Linux Release

[Steve Block]

> http://trac.webkit.org/changeset/64639 might have broken Qt Linux Release
Added new tests to Qt skipped list with http://trac.webkit.org/changeset/64640

[Simon Hausmann]

<cherry-pick-for-backport: r64639>

[Simon Hausmann]

<cherry-pick-for-backport: r64640>

[Simon Hausmann]

Revision r64639 cherry-picked into qtwebkit-2.1 with commit 741b9a8007571c1a8907f4de91f7dd9f6b3c345e
Revision r64640 cherry-picked into qtwebkit-2.1 with commit 51fa56069c38a406e86170dd764d6ce90d6d1a5a
found f81dde5b7304f9389b876c60ba5843774e3e7a02