Skip to content

Conversation

@marcoscaceres
Copy link
Member

@marcoscaceres marcoscaceres commented Apr 9, 2024

Closes #145

The following tasks have been completed:

Implementation commitment:


Preview | Diff

@reillyeon
Copy link
Member

I filed a Chromium issue to track this. Please file issues for WebKit and Gecko.

@marcoscaceres
Copy link
Member Author

Filed Gecko and WebKit bugs:
https://bugzilla.mozilla.org/show_bug.cgi?id=1890706

@saschanaz, would you be the right person to comment on this?

@marcoscaceres
Copy link
Member Author

marcoscaceres commented Apr 10, 2024

Sent a PR for WebKit WebKit/WebKit#27063

@marcoscaceres marcoscaceres changed the title Expose .toJSON() on GeolocationCoordinates Addition: expose .toJSON() on GeolocationCoordinates Apr 10, 2024
@saschanaz
Copy link
Member

👍 from Mozilla.

@marcoscaceres marcoscaceres changed the title Addition: expose .toJSON() on GeolocationCoordinates Addition: expose .toJSON() on GeolocationCoordinates + GeolocationPosition Apr 10, 2024
webkit-commit-queue pushed a commit to marcoscaceres/WebKit that referenced this pull request Apr 11, 2024
https://bugs.webkit.org/show_bug.cgi?id=272434
rdar://126183686

Reviewed by Ryosuke Niwa.

Implements and tests the toJSON() method for the GeolocationCoordinates interface, as proposed:
w3c/geolocation#147

* LayoutTests/fast/dom/Geolocation/coordinates-interface-toJSON-expected.txt: Added.
* LayoutTests/fast/dom/Geolocation/coordinates-interface-toJSON.html: Added.
* Source/WebCore/Modules/geolocation/GeolocationCoordinates.idl:

Canonical link: https://commits.webkit.org/277347@main
@marcoscaceres
Copy link
Member Author

@reillyeon, if all looks good, could you give it a ✅ 🙏

Patches ready to land on the WebKit side.

@reillyeon
Copy link
Member

I'm writing up the "Intent to Prototype and Ship" email for the Blink launch process and I just want to make sure I understand the motivation for this change. Are tests planning to use JSON.stringify() or is it that much easier to compare objects when they are "plain old JavaScript objects" than when they are instances of a WebIDL interface?

I noted when I was writing tests for this change in Chromium that the output of JSON.stringify() is not well defined (due to property order) so it doesn't seem appropriate for tests.

@marcoscaceres
Copy link
Member Author

marcoscaceres commented Apr 12, 2024

I'm writing up the "Intent to Prototype and Ship" email for the Blink launch process and I just want to make sure I understand the motivation for this change.

The primary motivation was to make it easier for developers to (re)use and serialize these objects.

await position = new Promise(r => navigator.geolocation.getCurrentPosition(r));
await fetch('https://example.com/api/positions', {
  method: 'POST', 
  headers: {
    'Content-Type': 'application/json'
  },
  body: JSON.stringify(position, null, 2)
});

And then it just happens that we can potentially reuse .coords.toJSON() for something like #146.

Are tests planning to use JSON.stringify() or is it that much easier to compare objects when they are "plain old JavaScript objects" than when they are instances of a WebIDL interface?

It's simpler to compare object's expected values than JSON.stringify() for the reasons you discovered.

I noted when I was writing tests for this change in Chromium that the output of JSON.stringify() is not well defined (due to property order) so it doesn't seem appropriate for tests.

Yeah, I concluded the same thing. You can see how I tested in WebKit:
https://github.com/WebKit/WebKit/pull/27063/files#diff-7531c0af803badf851f88a70c6702ee38c2936b1a42342e0430a3605befaf5b7

But the only interesting part is (overlook the fact that they are on the global object, that's just a WebKit testing framework thing):

          // We use "actual" for future proofing on purpose, in case more properties get added to the interface.
          for (const key in window.actual) {
            shouldBe(`window.actual.${key}`, `window.expected.${key}`);
          }

webkit-commit-queue pushed a commit to marcoscaceres/WebKit that referenced this pull request Apr 12, 2024
https://bugs.webkit.org/show_bug.cgi?id=272501
rdar://problem/126247408

Reviewed by Ryosuke Niwa.

Implements .toJSON() method on GeolocationPosition, as per spec change:
w3c/geolocation#147

* LayoutTests/fast/dom/Geolocation/position-interface-toJSON-expected.txt: Added.
* LayoutTests/fast/dom/Geolocation/position-interface-toJSON.html: Added.
* Source/WebCore/Modules/geolocation/GeolocationPosition.idl:

Canonical link: https://commits.webkit.org/277413@main
@saschanaz
Copy link
Member

Do we have context about why GeolocationPosition is not a dictionary? 🤔

@marcoscaceres
Copy link
Member Author

Do we have context about why GeolocationPosition is not a dictionary?

Just because of legacy, @saschanaz (given the API is like 10+ years old). If we were doing the API today, it would definitely just have been a dictionary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for converting Geolocation Position+Coordinates to JSON (object)

4 participants