http://lists.w3.org/Archives/Public/public-geolocation/2009Mar/0068.html In the current implementation, the timer starts right at the first request, and may fire before the user has granted permission. <rdar://problem/6672275>
Created attachment 29422 [details] implements the fix
Created attachment 29423 [details] updated patch to compile this compiles. I brought the change from iPhone, which is slightly different.
Comment on attachment 29423 [details] updated patch to compile r-. Some concerns below. > + * page/Geolocation.cpp: Add a RefPtr<PositionOptions> so it can be used > + later when the timer is started. Change PositionOptions* to PassRefPtr<PositionOptions> > + where needed. Please remove the tabs. > -void Geolocation::getCurrentPosition(PassRefPtr<PositionCallback> successCallback, PassRefPtr<PositionErrorCallback> errorCallback, PositionOptions* options) > +void Geolocation::getCurrentPosition(PassRefPtr<PositionCallback> successCallback, PassRefPtr<PositionErrorCallback> errorCallback, PassRefPtr<PositionOptions> options) > { > RefPtr<GeoNotifier> notifier = GeoNotifier::create(successCallback, errorCallback, options); > > - if (!m_service->startUpdating(options)) { > + if (!m_service->startUpdating(options.get())) { I am not sure it is safe to use the PRP options like this after passing it to GeoNotifier::create. Instead, you should use notifier->m_options as is one for notifier->m_errorCallback. > if (notifier->m_errorCallback) { > +int Geolocation::watchPosition(PassRefPtr<PositionCallback> successCallback, PassRefPtr<PositionErrorCallback> errorCallback, PassRefPtr<PositionOptions> options) > { > RefPtr<GeoNotifier> notifier = GeoNotifier::create(successCallback, errorCallback, options); > > - if (!m_service->startUpdating(options)) { > + if (!m_service->startUpdating(options.get())) { Same here. > +void Geolocation::startTimer(Vector<RefPtr<GeoNotifier> >& notifiers) Can this function be a static function instead of a member function? > + > +void Geolocation::startTimersForOneShots() > +{ > + Vector<RefPtr<GeoNotifier> > copy; > + copyToVector(m_oneShots, copy); If starting the timer cannot alter m_oneShot, I don't think there is any reason to copy the set into a vector. > +void Geolocation::startTimersForWatchers() > +{ > + Vector<RefPtr<GeoNotifier> > copy; > + copyValuesToVector(m_watchers, copy); Same comment.
(In reply to comment #3) > (From update of attachment 29423 [details] [review]) > r-. Some concerns below. > > + > > +void Geolocation::startTimersForOneShots() > > +{ > > + Vector<RefPtr<GeoNotifier> > copy; > > + copyToVector(m_oneShots, copy); > > If starting the timer cannot alter m_oneShot, I don't think there is any reason > to copy the set into a vector. > > > +void Geolocation::startTimersForWatchers() > > +{ > > + Vector<RefPtr<GeoNotifier> > copy; > > + copyValuesToVector(m_watchers, copy); > > Same comment. > Each of these are not Vectors, and this allows me to have a common startTimers call between the two. I'm addressing your other comments, and will have a new patch shortly.
Created attachment 29425 [details] Addresses Sam's review comments. It still gets the Vector for the oneShots and watchers when it starts the timers. This is so there is a common method between the two. One is a HashMap, the other a HashSet.
Comment on attachment 29425 [details] Addresses Sam's review comments. > > + static void startTimer(Vector<RefPtr<GeoNotifier> >&); This doesn't need to be in the header. You can just declare it static in the cpp file. Otherwise, r=me.
startTimer uses GeoNotifier, which is private to Geolocation. If I remove the declaration from the class, I get errors when using GeoNotifier.
bolsinga:WebKit bolsinga$ svn commit Sending WebCore/ChangeLog Sending WebCore/bindings/js/JSGeolocationCustom.cpp Sending WebCore/page/Geolocation.cpp Sending WebCore/page/Geolocation.h Transmitting file data .... Committed revision 42445.