[webkit-reviews] review denied: [Bug 25149] Geolocation should start timer after permission is granted : [Attachment 29423] updated patch to compile

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Greg Bolsinga <bolsinga at apple.com>'s
request for review:
Bug 25149: Geolocation should start timer after permission is granted
https://bugs.webkit.org/show_bug.cgi?id=25149

Attachment 29423: updated patch to compile
https://bugs.webkit.org/attachment.cgi?id=29423&action=review

------- Additional Comments from Sam Weinig <sam at webkit.org>
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.