Bug 120136 – [WK2] Add didChangeEnableHighAccuracy callback to WK2 geolocation API

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: WebKit2

[Hugo Parente Lima]

This GeolocationProvider callback is needed if the application want to use e.g. a low power method of acquiring geopositon when the web author don't need high accuracy.

This API was added to Nix, I'm upstreaming it just in case you guys have interest and/or feel this is useful.

[Hugo Parente Lima]

Created attachment 209304 [details]
Patch

[Early Warning System Bot]

Comment on attachment 209304 [details]
Patch

Attachment 209304 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1516860

[Build Bot]

Comment on attachment 209304 [details]
Patch

Attachment 209304 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1528200

[Build Bot]

Comment on attachment 209304 [details]
Patch

Attachment 209304 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1516862

[Hugo Parente Lima]

I can fix the Qt-wk2 and maybe the mac build errors if some wk2-owner show interest about land this patch, if there's no interest anyone feel free to mark the bug as WONTFIX.

Thanks.

[Benjamin Poulain]

(In reply to comment #5)
> I can fix the Qt-wk2 and maybe the mac build errors if some wk2-owner show interest about land this patch, if there's no interest anyone feel free to mark the bug as WONTFIX.

I am interested in this, this seems great.

The WebKit1 code for EnableHighAccuracy is quite wrong. I need to go through the code again to remember the problems and see if this patch avoids them.

Please give me a couple of days before I check this, maybe the weekend. If I did not review by Monday, feel free to nag me on IRC.
Do not land if another reviewer r+.

[Hugo Parente Lima]

(In reply to comment #6)
> (In reply to comment #5)
> > I can fix the Qt-wk2 and maybe the mac build errors if some wk2-owner show interest about land this patch, if there's no interest anyone feel free to mark the bug as WONTFIX.
> 
> I am interested in this, this seems great.
> 
> The WebKit1 code for EnableHighAccuracy is quite wrong. I need to go through the code again to remember the problems and see if this patch avoids them.

One problem I noticed is that setEnableHighAccuracy is called before startUpdating(), so the first update will not consider the highaccuracy setting.
 
> Please give me a couple of days before I check this, maybe the weekend. If I did not review by Monday, feel free to nag me on IRC.
> Do not land if another reviewer r+.

No problem, I'm not in a hurry :-)

[Hugo Parente Lima]

(In reply to comment #7)
> (In reply to comment #6)
> One problem I noticed is that setEnableHighAccuracy is called before startUpdating(), so the first update will not consider the highaccuracy setting.

Oops, I was wrong, they are called in the right order, sorry for the noise.

[Benjamin Poulain]

Comment on attachment 209304 [details]
Patch

I finally spent the time to read this. The patch is not correct.

WebGeolocationClient is per page. WebGeolocationManager is a WebProcess concept. WebGeolocationManagerProxy is WebContext concept.
You cannot just mix everything. When the message go to a more general concept, you need to keep track of the details of the smaller scope.

For example:
- Take a process Alpha with the page A and B. Page A ask for HighAccuracy=true, page B ask for HighAccuracy=false. With your patch, the last one wins. What should happen is HighAccuracy should be kept to true because one of the two live Geolocation object is in that mode.
- Take a UIProcess Beta with two web processes A and B. Same reasoning as above.
etc.

[Hugo Parente Lima]

(In reply to comment #9)
> (From update of attachment 209304 [details])
> I finally spent the time to read this. The patch is not correct.
> 
> WebGeolocationClient is per page. WebGeolocationManager is a WebProcess concept. WebGeolocationManagerProxy is WebContext concept.
> You cannot just mix everything. When the message go to a more general concept, you need to keep track of the details of the smaller scope.
> 
> For example:
> - Take a process Alpha with the page A and B. Page A ask for HighAccuracy=true, page B ask for HighAccuracy=false. With your patch, the last one wins. What should happen is HighAccuracy should be kept to true because one of the two live Geolocation object is in that mode.
> - Take a UIProcess Beta with two web processes A and B. Same reasoning as above.
> etc.

Got it, with the current patch the application still can solve this issue, but I think is better to solve it somehow on WebKit2 UIProcess level.

[Benjamin Poulain]

(In reply to comment #10)
> Got it, with the current patch the application still can solve this issue, but I think is better to solve it somehow on WebKit2 UIProcess level.

I am not sure to follow you here. How can the application solve the issues?

[Hugo Parente Lima]

(In reply to comment #11)
> (In reply to comment #10)
> > Got it, with the current patch the application still can solve this issue, but I think is better to solve it somehow on WebKit2 UIProcess level.
> 
> I am not sure to follow you here. How can the application solve the issues?

It knows about all start/stop geolocations requests and about all enable/disable high accuracy from all pages, so it have all the information to not turn off the high accuracy when it can't because there are some page using high accuracy. But I think the application shouldn't worry about this logic.

[Benjamin Poulain]

(In reply to comment #12)
> It knows about all start/stop geolocations requests and about all enable/disable high accuracy from all pages, so it have all the information to not turn off the high accuracy when it can't because there are some page using high accuracy. But I think the application shouldn't worry about this logic.

That is simply not possible. The informations about the Geolocation objects never leaves the WebProcess.

The best you can do is never go back from high accuracy until you stop updating geolocation. That would terribly bad for battery on mobile devices.

[Hugo Parente Lima]

(In reply to comment #13)
> That is simply not possible. The informations about the Geolocation objects never leaves the WebProcess.

Doesn't Webcore call stopUpdate when a Geolocation object requesting positions is destroyed?

Better to have this conversation on IRC btw.
 
> The best you can do is never go back from high accuracy until you stop updating geolocation. That would terribly bad for battery on mobile devices.

[Benjamin Poulain]

I will add this on https://bugs.webkit.org/show_bug.cgi?id=123786.

*** This bug has been marked as a duplicate of bug 123786 ***