Bug 48518 – Convert correctly between GeolocationPosition and Geoposition timestamp formats

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[John Knottenbelt]

WebCore/Page/Geolocation.cpp creates Geopositions from GeolocationPositions, but incorrect converts the timestamp. The GeolocationPosition timestamp is a floating point number in seconds (a double), and the Geoposition timestamp is a DOMTimestamp which is an integer number of milliseconds (unsigned long long). The fix is to convert seconds to milliseconds by multiplying by 1000.

[John Knottenbelt]

Created attachment 72180 [details]
Patch

[Andreas Kling]

Can you make a layout test for this?

[Adam Barth]

Comment on attachment 72180 [details]
Patch

We need a test.  Also, we should have a helper function that does this conversion so we can give it a name.

[Steve Block]

> We need a test.  Also, we should have a helper function that does this
> conversion so we can give it a name.
Makes sense, but why does GeolocationPosition use anything other than DOMTimeStamp (as used by Geoposition) in the first place? Is it to simplify converting to a WebKit type? I'm surprised there aren't other cases where we convert DOMTimeStamp to a WebKit type?

[John Knottenbelt]

Created attachment 73063 [details]
Patch

[John Knottenbelt]

It's worth noting that this bug is specific to ENABLE(CLIENT_BASED_GEOLOCATION). 

The fast/dom/Geolocation/maximum-age.html test fails without the fix, but I have included a new layout test which tests the timestamp conversion much more obviously.

Adam Barth suggested using converter functions. I've added a new bug 49066 with a proposal for that.

[John Knottenbelt]

Created attachment 73240 [details]
Patch

[Build Bot]

Attachment 73240 [details] did not build on win:
Build output: http://queues.webkit.org/results/5501031

[John Knottenbelt]

Created attachment 73358 [details]
Patch

[Jeremy Orlow]

Comment on attachment 73358 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 73358 [details]
Patch

Rejecting patch 73358 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2
Last 500 characters of output:
ast/dom/DOMException ....
fast/dom/DOMImplementation .....
fast/dom/Document ................
fast/dom/Document/CaretRangeFromPoint ....
fast/dom/Element .................................
fast/dom/EntityReference .
fast/dom/Geolocation ................................
fast/dom/Geolocation/timestamp.html -> failed

Exiting early after 1 failures. 7085 tests run.
148.68s total testing time

7084 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
4 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/5977083

[John Knottenbelt]

Created attachment 73995 [details]
Patch

[John Knottenbelt]

Comment on attachment 73995 [details]
Patch

The DOMTimeStamp bindings differs between Chromium and Mac WebKit. See bug https://bugs.webkit.org/show_bug.cgi?id=49589 . Change the test so that it does not depend on the actual representation of DOMTimeStamp.

Also, adds a new forwarding header to mac DumpRenderTree in order to get a proper timestamp for the event (comparable to Date.getTime).

[WebKit Commit Bot]

Comment on attachment 73995 [details]
Patch

Clearing flags on attachment: 73995

Committed r72211: <http://trac.webkit.org/changeset/72211>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/72211 might have broken GTK Linux 64-bit Debug
The following tests are not passing:
fast/dom/Geolocation/timestamp.html

[Ryosuke Niwa]

This change seems to be responsible for Gtk Linux test brokerage.  fast/dom/Geolocation/timestep.html is passing in http://build.webkit.org/results/GTK%20Linux%2032-bit%20Release/r72207%20(7748)/results.html but failing in http://build.webkit.org/results/GTK%20Linux%2032-bit%20Release/r72212%20(7749)/results.html