Bug 178173 - [Geolocation] Expose Coordinates.floorLevel
Summary: [Geolocation] Expose Coordinates.floorLevel
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, WebExposed
Depends on: 178148
Blocks:
  Show dependency treegraph
 
Reported: 2017-10-11 10:40 PDT by Chris Dumez
Modified: 2018-02-26 13:11 PST (History)
8 users (show)

See Also:


Attachments
WIP Patch (2.71 KB, patch)
2017-10-11 10:47 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (18.59 KB, patch)
2017-10-11 11:09 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (24.31 KB, patch)
2017-10-11 11:18 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (27.06 KB, patch)
2017-10-11 12:46 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (32.96 KB, patch)
2017-10-11 12:54 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-elcapitan (1.20 MB, application/zip)
2017-10-11 13:57 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2017-10-11 10:40:46 PDT
Expose Coordinates.floorLevel via Geolocation API.
Comment 1 Chris Dumez 2017-10-11 10:41:02 PDT
<rdar://problem/34918936>
Comment 2 Chris Dumez 2017-10-11 10:47:03 PDT
Created attachment 323428 [details]
WIP Patch
Comment 3 Chris Dumez 2017-10-11 11:09:17 PDT
Created attachment 323430 [details]
WIP Patch
Comment 4 Chris Dumez 2017-10-11 11:18:40 PDT
Created attachment 323432 [details]
WIP Patch
Comment 5 Ryosuke Niwa 2017-10-11 11:55:05 PDT
Comment on attachment 323432 [details]
WIP Patch

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

> Source/WebCore/Modules/geolocation/Coordinates.idl:38
> +    readonly attribute unrestricted double? floorLevel;

Please hide this behind a runtime flag because we don't have floor level information on macOS (and other platforms like Windows).
Comment 6 Chris Dumez 2017-10-11 12:32:43 PDT
(In reply to Ryosuke Niwa from comment #5)
> Comment on attachment 323432 [details]
> WIP Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=323432&action=review
> 
> > Source/WebCore/Modules/geolocation/Coordinates.idl:38
> > +    readonly attribute unrestricted double? floorLevel;
> 
> Please hide this behind a runtime flag because we don't have floor level
> information on macOS (and other platforms like Windows).

I'll add a Runtime flag but note that a lot of the coordinates attributes (heading / speed, ...) are optional and only provided on some specific platforms. This is the reason why those attributes are nullable, they are null if the platform does not support providing them.
Comment 7 Chris Dumez 2017-10-11 12:46:52 PDT
Created attachment 323448 [details]
WIP Patch
Comment 8 Chris Dumez 2017-10-11 12:54:56 PDT
Created attachment 323449 [details]
Patch
Comment 9 Build Bot 2017-10-11 13:57:28 PDT
Comment on attachment 323449 [details]
Patch

Attachment 323449 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4827509

New failing tests:
imported/w3c/web-platform-tests/XMLHttpRequest/open-url-worker-origin.htm
Comment 10 Build Bot 2017-10-11 13:57:29 PDT
Created attachment 323460 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 11 Chris Dumez 2017-10-11 14:47:28 PDT
(In reply to Build Bot from comment #9)
> Comment on attachment 323449 [details]
> Patch
> 
> Attachment 323449 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.webkit.org/results/4827509
> 
> New failing tests:
> imported/w3c/web-platform-tests/XMLHttpRequest/open-url-worker-origin.htm

This failure is unrelated.
Comment 12 Chris Dumez 2017-10-11 15:22:02 PDT
Comment on attachment 323449 [details]
Patch

Clearing flags on attachment: 323449

Committed r223211: <https://trac.webkit.org/changeset/223211>
Comment 13 Chris Dumez 2017-10-11 15:22:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Chris Peterson 2018-02-26 11:08:30 PST
In addition to setting floorLevel, this patch's GeolocationPosition class for iOS [1] initializes altitude, altitudeAccuracy, heading, and speed but the macOS GelocationPosition class [2] does not.

Is not supporting altitude, altitudeAccuracy, heading, and speed on macOS an intentional design decision or an oversight?

[1] https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/Modules/geolocation/GeolocationPosition.h

[2] https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/Modules/geolocation/ios/GeolocationPositionIOS.mm
Comment 15 Chris Dumez 2018-02-26 12:28:53 PST
(In reply to Chris Peterson from comment #14)
> In addition to setting floorLevel, this patch's GeolocationPosition class
> for iOS [1] initializes altitude, altitudeAccuracy, heading, and speed but
> the macOS GelocationPosition class [2] does not.
> 
> Is not supporting altitude, altitudeAccuracy, heading, and speed on macOS an
> intentional design decision or an oversight?
> 
> [1]
> https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/Modules/
> geolocation/GeolocationPosition.h
> 
> [2]
> https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/Modules/
> geolocation/ios/GeolocationPositionIOS.mm

I do not believe this is accurate, the following C API is available to the client application to provide the location on Mac:
WK_EXPORT WKGeolocationPositionRef WKGeolocationPositionCreate(double timestamp, double latitude, double longitude, double accuracy);
WK_EXPORT WKGeolocationPositionRef WKGeolocationPositionCreate_b(double timestamp, double latitude, double longitude, double accuracy, bool providesAltitude, double altitude, bool providesAltitudeAccuracy, double altitudeAccuracy, bool providesHeading, double heading, bool providesSpeed, double speed);
WK_EXPORT WKGeolocationPositionRef WKGeolocationPositionCreate_c(double timestamp, double latitude, double longitude, double accuracy, bool providesAltitude, double altitude, bool providesAltitudeAccuracy, double altitudeAccuracy, bool providesHeading, double heading, bool providesSpeed, double speed, bool providesFloorLevel, double floorLevel);

Notice that the last one can be used to provide heading / speed and altitude. The implementation looks like so:
WKGeolocationPositionRef WKGeolocationPositionCreate_c(double timestamp, double latitude, double longitude, double accuracy, bool providesAltitude, double altitude, bool providesAltitudeAccuracy, double altitudeAccuracy, bool providesHeading, double heading, bool providesSpeed, double speed, bool providesFloorLevel, double floorLevel)
{
    WebCore::GeolocationPosition corePosition { timestamp, latitude, longitude, accuracy };
    if (providesAltitude)
        corePosition.altitude = altitude;
    if (providesAltitudeAccuracy)
        corePosition.altitudeAccuracy = altitudeAccuracy;
    if (providesHeading)
        corePosition.heading = heading;
    if (providesSpeed)
        corePosition.speed = speed;
    if (providesFloorLevel)
        corePosition.floorLevel = floorLevel;

    auto position = WebGeolocationPosition::create(WTFMove(corePosition));
    return toAPI(&position.leakRef());
}

I think Safari may only provide timestamp / latitude / longitude / accuracy though. Is this your issue? If so, I would suggest filing a bug via https://developer.apple.com/bug-reporting/ since this would be a Safari issue, not a WebKit issue.
Comment 16 Chris Peterson 2018-02-26 13:11:43 PST
(In reply to Chris Dumez from comment #15)
> I think Safari may only provide timestamp / latitude / longitude / accuracy
> though. Is this your issue? If so, I would suggest filing a bug via
> https://developer.apple.com/bug-reporting/ since this would be a Safari
> issue, not a WebKit issue.

Yes, that's the issue that lead me here. I'll report a Safari issue. Thanks!