[webkit-reviews] review granted: [Bug 103540] Use GeolocationController's last geoposition as cached position. : [Attachment 176520] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted John Knottenbelt
<jknotten at chromium.org>'s request for review:
Bug 103540: Use GeolocationController's last geoposition as cached position.
https://bugs.webkit.org/show_bug.cgi?id=103540

Attachment 176520: Patch
https://bugs.webkit.org/attachment.cgi?id=176520&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=176520&action=review


The patch looks good, the test seems well designed for what we want to cover.

One extra concern is privacy with the "leaking" of information through the
page.

For example, by having a frame with Google Maps, you can find if the user has
authorized Google Maps to access location.
I think this is not blocking because finding the same information through
timing attacks is just as easy. Moreover, that is a pretty poor leak if the
authorization client is implemented properly.

> Source/WebCore/ChangeLog:9
> +	   GeolocationClient for multiple frames' Geolocation instances.  This

Two space after the period.

>
LayoutTests/fast/dom/Geolocation/resources/cached-position-while-watching-inner
.html:6
> +	       'success':  true,

Double space after the colon.

>
LayoutTests/fast/dom/Geolocation/resources/cached-position-while-watching-inner
.html:8
> +	       'message': 'Received cached position lat: ' +
position.coords.latitude + ', long: ' +
> +			  position.coords.longitude

Coding style: position.coords.longitude should be on the same line, or the "+"
should be on the second line.

>
LayoutTests/fast/dom/Geolocation/script-tests/cached-position-while-watching.js
:2
> +description('Tests that a cached position can be obtained in one frame after
another frame has created a geolocation watch.');
> +

Maybe inline "cached-position-while-watching.js" in
"cached-position-while-watching.html"? It is just my personal preference, your
call.

>
LayoutTests/fast/dom/Geolocation/script-tests/cached-position-while-watching.js
:17
> +navigator.geolocation.watchPosition(
> +    function() {

You could use getCurrentPosition(). That would avoid isFirstPosition and make
the timing more interesting for the test.