Bug 48518 - Convert correctly between GeolocationPosition and Geoposition timestamp formats
: Convert correctly between GeolocationPosition and Geoposition timestamp formats
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
: 49066
: 39908 40373 45752
  Show dependency treegraph
 
Reported: 2010-10-28 06:22 PST by
Modified: 2010-11-17 11:59 PST (History)


Attachments
Patch (1.65 KB, patch)
2010-10-28 06:32 PST, John Knottenbelt
no flags Review Patch | Details | Formatted Diff | Diff
Patch (5.03 KB, patch)
2010-11-05 07:08 PST, John Knottenbelt
no flags Review Patch | Details | Formatted Diff | Diff
Patch (8.94 KB, patch)
2010-11-08 08:18 PST, John Knottenbelt
no flags Review Patch | Details | Formatted Diff | Diff
Patch (5.06 KB, patch)
2010-11-09 04:46 PST, John Knottenbelt
no flags Review Patch | Details | Formatted Diff | Diff
Patch (6.69 KB, patch)
2010-11-16 08:07 PST, John Knottenbelt
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-10-28 06:22:30 PST
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.
------- Comment #1 From 2010-10-28 06:32:41 PST -------
Created an attachment (id=72180) [details]
Patch
------- Comment #2 From 2010-10-31 13:44:53 PST -------
Can you make a layout test for this?
------- Comment #3 From 2010-10-31 18:10:52 PST -------
(From update of attachment 72180 [details])
We need a test.  Also, we should have a helper function that does this conversion so we can give it a name.
------- Comment #4 From 2010-11-05 06:47:55 PST -------
> 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?
------- Comment #5 From 2010-11-05 07:08:58 PST -------
Created an attachment (id=73063) [details]
Patch
------- Comment #6 From 2010-11-05 07:12:25 PST -------
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.
------- Comment #7 From 2010-11-08 08:18:49 PST -------
Created an attachment (id=73240) [details]
Patch
------- Comment #8 From 2010-11-08 08:50:53 PST -------
Attachment 73240 [details] did not build on win:
Build output: http://queues.webkit.org/results/5501031
------- Comment #9 From 2010-11-09 04:46:00 PST -------
Created an attachment (id=73358) [details]
Patch
------- Comment #10 From 2010-11-15 09:07:20 PST -------
(From update of attachment 73358 [details])
r=me
------- Comment #11 From 2010-11-15 09:34:03 PST -------
(From update of attachment 73358 [details])
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
------- Comment #12 From 2010-11-16 08:07:44 PST -------
Created an attachment (id=73995) [details]
Patch
------- Comment #13 From 2010-11-16 08:11:20 PST -------
(From update of attachment 73995 [details])
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).
------- Comment #14 From 2010-11-17 09:08:51 PST -------
(From update of attachment 73995 [details])
Clearing flags on attachment: 73995

Committed r72211: <http://trac.webkit.org/changeset/72211>
------- Comment #15 From 2010-11-17 09:08:58 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #16 From 2010-11-17 10:00:20 PST -------
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
------- Comment #17 From 2010-11-17 11:59:29 PST -------
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