Bug 39908

See also: bug 40148.

reentrant-error.html and reentrant-success.html are now fixed with the fix for Bug 40148. maximum-age.html still fails in client-based Geolocation. This seems not to be due to the client-based approach itself, but due to the different code path used to obtain permission.

Created attachment 74002 [details] Patch

Comment on attachment 74002 [details] Patch Depends on https://bugs.webkit.org/show_bug.cgi?id=49597 for GeoNotifierVector typedef.

Comment on attachment 74002 [details] Patch Looks good, although hard to follow the logic behind it. Maybe we could move the pending cache-using requests out of the vector copy and back into the set before calling SendError(). (But after clearing the set). We could do this for both oneshots and watchers (to avoid a cache-using watcher from going into limbo if a fatal error occurred before its cache-pump timer fires) This way we'd keep the policy logic in one place, rather than half in SendError and half in the copy-back

Created attachment 74101 [details] Patch

Comment on attachment 74101 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=74101&action=review > WebCore/page/Geolocation.cpp:582 > + copyToSet(oneShotsCached, m_oneShots); It's not obvious when the watchersCached set is not re-instated here too. Specifically, if the error was fatal we won't have sent them an error above (as we removed them from the watchersCopy vector) but they won't get a cached position either (as we don't reinstate here). Perhaps it's the intermix of isFatal and isDenied that is confusing me. Would it be clearer as: if (!isFatal) { removeCachedNotifiers(oneShotsCopy, oneShotsCached); removeCachedNotifiers(watchersCopy, watchersCached); } else m_watchers.clear(); m_oneshots.clear(); Also would it be worth adding a comment on hasListeners that we need to test that before re-instating pending one-shots?

Created attachment 74119 [details] Patch

Comment on attachment 74119 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=74119&action=review Steve is probably the best reviewer, but unfortunately he's on holiday. Here are a few comments to get you started though. > LayoutTests/ChangeLog:4 > + include title and bug url at top...in other CLs too > WebCore/page/Geolocation.cpp:530 > + RefPtr<GeoNotifier> notifier = *it; just store a pointer. No need to ref it. > WebCore/page/Geolocation.cpp:543 > + RefPtr<GeoNotifier> notifier = *it; same > WebCore/page/Geolocation.cpp:544 > + dest.add(notifier); wrong indent

Comment on attachment 74119 [details] Patch LGTM. I'd be inclined to comment why we ignore watchersCached, or at least name it ignoreWatchersCached, or maybe even pass a NULL pointer into removeCachedNotifiers to avoid copying out the unneeded refs. I don't know which fits best with webkit style though?

(In reply to comment #10) > (From update of attachment 74119 [details]) > LGTM. I'd be inclined to comment why we ignore watchersCached, or at least name it ignoreWatchersCached, or maybe even pass a NULL pointer into removeCachedNotifiers to avoid copying out the unneeded refs. I don't know which fits best with webkit style though? some comments: View in context: https://bugs.webkit.org/attachment.cgi?id=74119&action=review > WebCore/page/Geolocation.cpp:525 > +void Geolocation::removeCachedNotifiers(GeoNotifierVector& notifiers, GeoNotifierVector& cached) hmm, I was a bit confused with this name and the fact that it has two params, my original thought was that it was removing "cached" from "notifiers".. not sure if it'd be any clearer, but perhaps extractNotifiersWithCachedPosition? also, both this and copyToSet could be static helper functions rather than regular methods. > WebCore/page/Geolocation.cpp:561 > + GeoNotifierVector oneShotsCached; as above, perhaps oneShotsWithCachedPosition

Comment on attachment 74119 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=74119&action=review >> WebCore/page/Geolocation.cpp:525 >> +void Geolocation::removeCachedNotifiers(GeoNotifierVector& notifiers, GeoNotifierVector& cached) > > hmm, I was a bit confused with this name and the fact that it has two params, my original thought was that it was removing "cached" from "notifiers".. > not sure if it'd be any clearer, but perhaps extractNotifiersWithCachedPosition? > > also, both this and copyToSet could be static helper functions rather than regular methods. Agree with method rename. GeoNotifier and GeoNotifierVector are private in Geolocation, which makes it hard to bring these methods out as non-member functions. However, they can be static member functions. >> WebCore/page/Geolocation.cpp:561 >> + GeoNotifierVector oneShotsCached; > > as above, perhaps oneShotsWithCachedPosition Agree.

Created attachment 74240 [details] Patch

(In reply to comment #13) > Created an attachment (id=74240) [details] > Patch LGTM, thanks john!

Comment on attachment 74240 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=74240&action=review > WebCore/ChangeLog:19 > +2010-11-16 John Knottenbelt <jknotten@chromium.org> Why the duplication? > WebCore/page/Geolocation.cpp:534 > + } else { no {}

Created attachment 74255 [details] Patch

Comment on attachment 74255 [details] Patch r=me

Thanks for tackling this!

Comment on attachment 74255 [details] Patch Clearing flags on attachment: 74255 Committed r72396: <http://trac.webkit.org/changeset/72396>

All reviewed patches have been landed. Closing bug.