Bug 178148 - Modernize Geolocation code
Summary: Modernize Geolocation code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 178173
  Show dependency tree / graph
 
Reported: 2017-10-10 15:59 PDT by Chris Dumez
Modified: 2017-10-11 10:43 PDT (History)
9 users (show)

See Also:

Attachments
Patch (63.04 KB, patch)
2017-10-10 20:08 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (63.43 KB, patch)
2017-10-10 20:18 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (63.49 KB, patch)
2017-10-10 20:32 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (64.43 KB, patch)
2017-10-10 20:49 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (64.67 KB, patch)
2017-10-10 20:56 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (64.68 KB, patch)
2017-10-10 21:00 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (64.70 KB, patch)
2017-10-10 21:08 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (65.14 KB, patch)
2017-10-10 22:39 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (65.16 KB, patch)
2017-10-11 08:44 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (65.17 KB, patch)
2017-10-11 08:47 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (65.41 KB, patch)
2017-10-11 09:37 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Show Obsolete (10) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2017-10-10 15:59:31 PDT 
Modernize Geolocation code.
Comment 1 Chris Dumez 2017-10-10 20:08:43 PDT 
Created attachment 323374 [details]
Patch
Comment 2 Build Bot 2017-10-10 20:11:20 PDT 
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 3 Chris Dumez 2017-10-10 20:18:47 PDT 
Created attachment 323376 [details]
Patch
Comment 4 Chris Dumez 2017-10-10 20:32:44 PDT 
Created attachment 323378 [details]
Patch
Comment 5 Chris Dumez 2017-10-10 20:49:45 PDT 
Created attachment 323380 [details]
Patch
Comment 6 Chris Dumez 2017-10-10 20:56:54 PDT 
Created attachment 323381 [details]
Patch
Comment 7 Chris Dumez 2017-10-10 21:00:07 PDT 
Created attachment 323382 [details]
Patch
Comment 8 Chris Dumez 2017-10-10 21:08:40 PDT 
Created attachment 323383 [details]
Patch
Comment 9 Ryosuke Niwa 2017-10-10 22:23:09 PDT 
Comment on attachment 323383 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323383&action=review

r=me

> Source/WebCore/Modules/geolocation/Coordinates.h:48
> +    double latitude() const { return m_position.latitude; }
> +    double longitude() const { return m_position.longitude; }

We should probably assert that these values aren't NaN/-1.

> Source/WebCore/Modules/geolocation/GeolocationPosition.h:56
> +    double latitude { -1 };

it's worth mentioning that these values are mandatory and cannot be omitted.
We should also add asserts where it's used to make sure we never see -1.
Also, we should probably use NaN instead of -1 per IRC discussion
since latitude & longitude can take a negative value.
Comment 10 Chris Dumez 2017-10-10 22:39:14 PDT 
Created attachment 323388 [details]
Patch
Comment 11 Chris Dumez 2017-10-11 08:44:00 PDT 
Created attachment 323418 [details]
Patch
Comment 12 Chris Dumez 2017-10-11 08:47:42 PDT 
Created attachment 323419 [details]
Patch
Comment 13 Chris Dumez 2017-10-11 09:37:10 PDT 
Created attachment 323422 [details]
Patch
Comment 14 Chris Dumez 2017-10-11 10:42:04 PDT 
Comment on attachment 323422 [details]
Patch

Clearing flags on attachment: 323422

Committed r223192: <https://trac.webkit.org/changeset/223192>
Comment 15 Chris Dumez 2017-10-11 10:42:05 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2017-10-11 10:43:09 PDT 
<rdar://problem/34936394>