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

FF116 Audio Output Device API updates #27818

Merged
merged 17 commits into from
Jul 20, 2023

Conversation

hamishwillee
Copy link
Collaborator

@hamishwillee hamishwillee commented Jul 7, 2023

FF116 adds support for the Audio Output Device API.

This improves some of the docs. There feels like quite a lot of duplication, in particular around the way permissions/security considerations work. But at least it should not be "wrong". Happy to take advice.

Other docs work can be tracked in #27748

@github-actions github-actions bot added the Content:WebAPI Web API docs label Jul 7, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jul 7, 2023

Preview URLs (6 pages)
Flaws (14)

Note! 5 documents with no flaws that don't need to be listed. 🎉

URL: /en-US/docs/Web/API/HTMLMediaElement
Title: HTMLMediaElement
Flaw count: 14

  • macros:
    • /en-US/docs/Web/API/HTMLMediaElement/played does not exist
    • /en-US/docs/Web/API/HTMLMediaElement/preload does not exist
    • /en-US/docs/Web/API/HTMLMediaElement/seeking does not exist
    • /en-US/docs/Web/API/HTMLMediaElement/onencrypted redirects to /en-US/docs/Web/API/HTMLMediaElement
    • /en-US/docs/Web/API/HTMLMediaElement/onwaitingforkey redirects to /en-US/docs/Web/API/HTMLMediaElement
    • and 9 more flaws omitted

(comment last updated: 2023-07-20 23:24:19)

@hamishwillee hamishwillee marked this pull request as ready for review July 10, 2023 08:59
@hamishwillee hamishwillee requested a review from a team as a code owner July 10, 2023 08:59
@hamishwillee hamishwillee requested review from wbamberg and removed request for a team July 10, 2023 08:59
Copy link

@karlt karlt left a comment

Choose a reason for hiding this comment

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

Looks great, thank you very much!

files/en-us/web/api/audio_output_devices_api/index.md Outdated Show resolved Hide resolved
files/en-us/web/api/audio_output_devices_api/index.md Outdated Show resolved Hide resolved
files/en-us/web/api/htmlmediaelement/setsinkid/index.md Outdated Show resolved Hide resolved
files/en-us/web/api/htmlmediaelement/sinkid/index.md Outdated Show resolved Hide resolved
@hamishwillee
Copy link
Collaborator Author

@karlt Thanks so much for your review. Makes a huge difference having people who intimately understand the spec look at the changes.

I have accepted your changes. Now will see if @wbamberg has more to add!

@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

Just little things.

files/en-us/web/api/audio_output_devices_api/index.md Outdated Show resolved Hide resolved
files/en-us/web/api/audio_output_devices_api/index.md Outdated Show resolved Hide resolved
files/en-us/web/api/htmlmediaelement/setsinkid/index.md Outdated Show resolved Hide resolved
The **`HTMLMediaElement.setSinkId()`** method of the [Audio Output Devices API](/en-US/docs/Web/API/Audio_Output_Devices_API) sets the ID of the audio device to use for output and returns a [`Promise`](/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise).

This only works when the application is permitted to use the specified device.
For more information see the [security requirements](#security_requirements) below.

## Syntax
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not in your PR, but I think we need a better way of talking about "exceptions" for functions that return promises, because presumably these exceptions aren't thrown (unless the function is called with await) but rather the promise rejects with them.

So maybe after ### Exceptions we should say something like "The returned promise may reject with any of the following excepotions"? I wonder if @teoli2003 has an opinion on this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have been avoiding this question by just listing the exceptions - not "this exception will be thrown" or "promise will be rejected with this exception". I see this as less wrong that either of the above options, but that's just an opinion.

We should have a decision and standard words on this - raise a discussion?

Comment on lines +37 to +38
The option is intended for applications that want to store a device id so that the same device can be used by default in future sessions.
Note that the method may return a new ID for the same device, and that persisted ids _must be passed_ through `selectAudioOutput()` successfully before they will work with {{domxref("HTMLMediaElement.setSinkId","setSinkId()")}}.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FYI @wbamberg This is updated explanation of what it means to persist a device id

@hamishwillee
Copy link
Collaborator Author

@wbamberg Thanks for the feedback. I think everything that is an action on this PR has been addressed now.

Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

Thank you @hamishwillee ! I won't merge this because of the Prettier failures. I wish #27070 would get some kind of resolution :(.

@hamishwillee hamishwillee merged commit d458c22 into mdn:main Jul 20, 2023
6 checks passed
@hamishwillee hamishwillee deleted the ff116_audio_output_device_api branch July 20, 2023 23:28
@hamishwillee
Copy link
Collaborator Author

Thanks @wbamberg. I'm not happy about that prettier PR stalling. It's such an obvious barrier to contribution.

@wbamberg
Copy link
Collaborator

What is a bit frustrating is that we had this exact conversation about 18 months ago, and this was exactly the reason we didn't just run Prettier in CI then. And now we did, and exactly what we thought would happen, is happening.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:WebAPI Web API docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants