w3c / geolocation

W3C Geolocation API
https://www.w3.org/TR/geolocation/
81 stars 56 forks source link

Remove [NoInterfaceObject] #20

Closed anssiko closed 5 years ago

anssiko commented 5 years ago

Rename exposed interfaces

Fix #19

anssiko commented 5 years ago

PTAL @reillyeon

anssiko commented 5 years ago

@honry can help update https://github.com/web-platform-tests/wpt/tree/master/geolocation-API accordingly? TL;DR: this patch exposes some interfaces so maybe idlharness.js helps.

Honry commented 5 years ago

@foolip's bot will take care of every IDL updates. (i.e. the bot will automatically generate a PR with updated idlharness.js tests to reflect to every IDL updates in all specs.)

anssiko commented 5 years ago

Props to @autofoolip for taking care of idlharness.js tests automatically.

IIUC the bot uses IDLs extracted by reffy (here https://github.com/web-platform-tests/wpt/blob/master/interfaces/geolocation-API.idl) that extracts them by crawling the TR version of the spec (in this case https://www.w3.org/TR/geolocation-API). What follows is that the IDL tests will be updated automatically only when a new spec version is published on TR.

My interpretation is we're still compliant with the test as you commit policy we follow in this group. We just need to publish a TR version after any substantial change to the spec IDL to trigger the IDL test update.

@Honry, please correct me if I'm wrong.

Honry commented 5 years ago

All refreshed IDLs used by the bot are maintained in https://github.com/tidoust/reffy-reports/tree/master/w3c/idl

Looks like it crawls the ED version of the spec, for example, the bot updated the https://github.com/web-platform-tests/wpt/pull/15125 while the IDL change is not published to TR yet.

anssiko commented 5 years ago

@Honry, thanks for the clarification. Using IDL from EDs as the source sounds like a better solution. No issues then.

marcoscaceres commented 5 years ago

Oh wait, did https://github.com/w3c/geolocation-api/pull/23 undo this? If yes, yay 😃

marcoscaceres commented 5 years ago

Anyway, please please please file bugs on Gecko with changes you are making, otherwise we are all going to get out of sync!

reillyeon commented 5 years ago

23 partially undid the rename. https://chromium-review.googlesource.com/c/chromium/src/+/1471230 is the change to implement this in Blink which I never merged due to some CQ flakiness. I should get on that.

It still renames,

  Coordinates   -> GeolocationCoordinates
  Position      -> GeolocationPosition
  PositionError -> GeolocationPositionError
marcoscaceres commented 5 years ago

Ok, that’s not too bad... but if we can avoid the renaming entirely 🥰

reillyeon commented 5 years ago

Wouldn't you agree that the names "coordinates" and "position" are likely to be confused with rendering concepts from other parts of the platform?

marcoscaceres commented 5 years ago

Wouldn't you agree that the names "coordinates" and "position" are likely to be confused with rendering concepts from other parts of the platform?

From the engines perspective, no - because they are already implemented specifically for Geolocation.

From an authoring perspective, maybe: They don't have constructors, so they can't be directly used anyway. A new set of coordinates from any new spec could just be named 2DCoordinates and 3DPosition or whatever, without us needing to rename this one (i.e., this name is already squatted).

reillyeon commented 5 years ago

If you are firmly opposed to renaming I agree that the names are already squatted and future APIs can work around it. From my perspective this is our last shot before renaming becomes impossible so it seems like a good opportunity to pick something less likely to cause confusion.

anssiko commented 5 years ago

@marcoscaceres said:

As @annevk mentioned, it’s probably not worth doing this at this point.

Per my reading of https://github.com/w3c/geolocation-api/issues/19#issue-399256646 @annevk is not opposed to the idea of rename, just wanted the group to "carefully consider with what name to expose these interfaces and then please drop this.", where "drop this" refers to [NoInterfaceObject]. I feel the group did carefully consider the names as documented in #19 and #23.

To be clear, the following are the interfaces that dropped [NoInterfaceObject] and the renames to address #19:

Coordinates -> GeolocationCoordinates
Position -> GeolocationPosition
PositionError -> GeolocationPositionError

This seems like a good opportunity for implementers to roll both the (mostly) mechanical rename and the removal of [NoInterfaceObject] into the same patch as in https://chromium-review.googlesource.com/c/chromium/src/+/1471230

Yours, Anssi the co-chair

marcoscaceres commented 5 years ago

I think we need a third opinion here from the WebKit folks - otherwise this exercise is rather pointless. I'm willing to make the change in Gecko if they are also willing to make the change in WebKit. It's a bunch of refactoring work, so I'd personally prefer to spend time on other things. However, if others think this preemptive change is good for the platform, I'm ok to invest time to do the refactoring.

anssiko commented 5 years ago

Looping in @cdumez for WebKit feedback. Here's the Blink patch of this spec change: https://chromium-review.googlesource.com/c/chromium/src/+/1471230

marcoscaceres commented 5 years ago

While we are here, can I kindly request that we add: https://github.com/w3c/screen-orientation/blob/gh-pages/.github/PULL_REQUEST_TEMPLATE.md

That way, we make sure nothing gets merged into the spec unless we have agreement from at least 2 browser vendors, browser bugs are filed, and we have tests?

cdumez commented 5 years ago

This looks reasonable. Where is the WebKit bug so that I can work on this?

marcoscaceres commented 5 years ago

Curse you, @cdumez 😋 guess I’ll be refactoring too.

anssiko commented 5 years ago

@cdumez, the WebKit bug is at https://webkit.org/b/200885 Thanks!

@marcoscaceres, you want me to open one for Gecko? You'll know Mozillians who should be cc'd in the bug, but I'm happy to create one if so preferred.

marcoscaceres commented 5 years ago

That would be great! Please cc me and we will go from there.

anssiko commented 5 years ago

@marcoscaceres, the Gecko bug is at https://bugzil.la/1575144 Thanks!

marcoscaceres commented 5 years ago

Awesome, thanks!

anssiko commented 5 years ago

The WebKit bug https://webkit.org/b/200885 is now fixed. That was fast, thanks @cdumez!

marcoscaceres commented 4 years ago

Landed in Gecko https://bugzilla.mozilla.org/show_bug.cgi?id=1575144