Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update status of this document #61

Merged
merged 1 commit into from
Jun 17, 2024
Merged

Update status of this document #61

merged 1 commit into from
Jun 17, 2024

Conversation

anssiko
Copy link
Member

@anssiko anssiko commented Jun 3, 2024

@anssiko anssiko requested a review from reillyeon June 3, 2024 17:47
Copy link
Member

@reillyeon reillyeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this change resolves #60. If your goal is to recast this proposal as way of satisfying background geolocation use cases then I think a more significant update to the abstract is necessary and GeolocationSensor interface should be removed until a more concrete proposal for how an API satisfying these use cases (such as @mkruisselbrink's proposal from 2014) is presented.

Review and feedback is sought after in particular for the new [[#use-cases-background-continuous]]
and [[#use-cases-background-geofence]] use case categories. All feedback, including
contributions for new informative use cases and other considerations,
is welcome via <a href="https://github.com/w3c/geolocation-sensor/issues">GitHub issues</a>.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
is welcome via <a href="https://github.com/w3c/geolocation-sensor/issues">GitHub issues</a>.
is welcome via <a href="[REPOSITORYURL]/issues">GitHub issues</a>.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @reillyeon. Addressed your suggestion in an update, also updated the commit message.

Could you open a new issue linking to the proposal from 2014 you mentioned?

I'd like to link to that issue from this status text as to raise awareness of options that have been considered and to invite feedback and comments from the web community on that proposal.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like spec-prod validator does not like [REPOSITORYURL] text macro inside href, CI error:

error: Bad value “[REPOSITORYURL]/issues” for attribute “href” on element “a”: Illegal character in path segment: “[” is not allowed.

Local build was fine. May need to revert back to the plain old URL unless some Bikeshed expert tells me otherwise.

Copy link
Contributor

@himorin himorin Jun 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't define repository in macro, so add repository line (with text macro: REPOSITORY w3c/geolocation-sensor etc.)

(fixed)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(wrong again...) text macro will be created following Repository: line, and should be created automatically if bikeshed runs via ghaction... hrm.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR updated. The rendered version will have a very visible issue block on top of the API section pointing to that issue. The status text links to the API section too setting expectations with regard to changes expected.

Also fixed the text macro, thanks @himorin for filing an issue for that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is fine but please remove the "Fix #60" comment in the description as this does not resolve the issue raised there. To be clear, I think there are two paths forward for resolving that issue. Either,

  1. Mark this specification as Obsolete and start a new specification tailored to the background/geofencing use cases.
  2. Get consensus from working group participants that the API presented in the current draft is something that should be pursued as a replacement for the existing Geolocation API.

I recommend (1). I don't foresee (2) being possible because there has been no interest for many years in implementing this API.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for (1)!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@reillyeon thanks for the suggestions. I removed the left-over "Fix" from the comment. PTAL and merge at will.

Some considerations:

  • Background/geofencing use cases and path forward will be an excellent TPAC discussion.
  • If the WG finds out relevant foreground use cases are addressed by the old Geolocation API, this new API could be rescoped to address background/geofencing use cases.
  • For background/geofencing API shape considerations, @mkruisselbrink's proposals referenced in Background geofencing #62 are (still) good input and this PR makes those proposals prominent and notes the API is expected to change. (Good proposals age like fine wine! 🍷)

A process-related clarification for option (1):

Given this spec is a Working Draft, and given (1) proposes a narrower scope, the WG only needs to decide to publish an updated Working Draft with foreground functionality removed without any heavy-handed mechanisms beyond that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, we can just modify the Working Draft.

@anssiko
Copy link
Member Author

anssiko commented Jun 17, 2024

Thanks for the review @reillyeon. My expectation is we'll continue to keep the status actively updated to ensure the wider community is aware of the latest developments and can direct its review and feedback accordingly.

@anssiko anssiko merged commit a411813 into main Jun 17, 2024
2 checks passed
@anssiko anssiko deleted the sotd branch June 17, 2024 16:48
github-actions bot added a commit that referenced this pull request Jun 17, 2024
SHA: a411813
Reason: push, by anssiko

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

4 participants