Bug 40002

Created attachment 57553 [details] Patch

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?

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

Created attachment 58033 [details] Patch

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.

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?

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

Created attachment 62699 [details] Patch

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.

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

Created attachment 63340 [details] Patch

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

> 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

Created attachment 63353 [details] Patch

Comment on attachment 63353 [details] Patch New patch to check build on EWS bots.

Comment on attachment 63353 [details] Patch Minor changes only since Alexey's r+

Comment on attachment 63353 [details] Patch Giving up on commit queue, which seems to be having problems. Will land manually.

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

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

> 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

<cherry-pick-for-backport: r64639>

<cherry-pick-for-backport: r64640>

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