Bug 178148

Summary: Modernize Geolocation code
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebCore Misc.Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, berto, buildbot, cgarcia, commit-queue, gustavo, mcatanzaro, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 178173    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

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>