[Webkit-unassigned] [Bug 25149] Geolocation should start timer after permission is granted

[bugzilla-daemon at webkit.org]

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


sam at webkit.org changed:

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




------- Comment #3 from sam at webkit.org  2009-04-12 14:22 PDT -------
(From update of attachment 29423)
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.


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