[Webkit-unassigned] [Bug 38323] Prevents setLastPosition() before startUpdating() being called.

[bugzilla-daemon at webkit.org]

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





--- Comment #9 from Marcus Bulach <bulach at chromium.org>  2010-05-04 08:19:11 PST ---
Hi Steve,

thanks again for your review! comments inline:

(In reply to comment #6)
> (From update of attachment 54801 [details])
> > +        This prevents a page requesting permission when it has just accessed navigator.geolocation (without calling navigator.geolocation.getGeolocationPosition).
> Did you mean navigator.geolocation.getCurrentPosition/watchPosition?
> 
> > +    m_startUpdatingRequested = true;
> Don't you also need to set this to false in stopUpdating() to prevent
> superfluous permission requests after all location requests have stopped?

thanks for pointing this out: there was actually another issue with
stopUpdating detaching the bridge.
fixed now by using a counter around start / stop updating.

> If a call to navigator.getCurrentPosition/watchPosition is successful, then
> permission must have been granted by the time stopUpdating() is called, so
> there's no problem. However, if the service returns an error, or JavaScript
> cancels the location request before the service returns anything, then there
> will be no location requests in progress and no permission request will have
> been made. With the current patch, m_startUpdatingRequested will still be true
> so subsequent calls to setLastPosition() due to activity in other frames will
> wrongly trigger a permission request.

yep, I guess the new patch addresses the javascript cancelling the request.
as for the service throwing error condition: if i understood the flow correctly
it will eventually call stopUpdating, right? so the counter will be balanced?

> 
> Or does the browser process already have logic to prevent this?

nope, the browser doesn't know about this.
speaking of which, I added a browser test on the other side:
http://codereview.chromium.org/1950001/show

it fails without this patch, and obviously passes with.. :)

> 
> Maybe it's better to modify the browser process to only broadcast location
> information to bridges that have called startUpdating() but not stopUpdating(),
> as you suggest in the Chromium bug?

I think adding this logic up in the browser process would further complicate
things. the issue was that the bridge was being attached too early, so I hope
this patch now fixes it (by attaching it later), and also the other issue you
raised about start / stop. 
from the browser point of view, a bridge now is only attached when it has a
pending request.

would you please take another look?

thanks!

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