-
Notifications
You must be signed in to change notification settings - Fork 166
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
Address #1857: Fix typo/phrasing through AudioWorkletNode #1902
Conversation
* Fix typos and phrasing * Add/adjust some links
There are a few more items that I need to ask about before finishing this PR. |
@@ -9814,7 +9814,8 @@ This interface has "entries", "forEach", "get", "has", "keys", | |||
<pre class="idl"> | |||
[Exposed=Window, | |||
SecureContext, | |||
Constructor (BaseAudioContext context, DOMString name, optional AudioWorkletNodeOptions options)] | |||
Constructor (BaseAudioContext context, DOMString name, | |||
optional AudioWorkletNodeOptions options)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Split line so that the generated html isn't a pretty long line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
{{AudioNode/channelCount}}. | ||
|
||
: <dfn>parameterData</dfn> | ||
:: | ||
This is a list of user-defined key-value pairs that are used | ||
to initialize {{AudioParam}} values in | ||
to initialize {{AudioParam}} objects in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -10035,16 +10036,16 @@ Constructors</h5> | |||
: <dfn>port</dfn> | |||
:: | |||
Every {{AudioWorkletProcessor}} has an associated | |||
<code>port</code> which is a {{MessagePort}}. It is connected to the port on the | |||
<code>port</code> which is a {{MessagePort}}. It is connected to a port on the | |||
corresponding {{AudioWorkletProcessor}} object allowing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the port for the AudioWorkletProcessor. Should this say it's connected to the port for associated AudioWorkletNode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This port is created by MessageChannel. So the association between port1 and port2 are already implied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I undo this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the port
is more specific. So yes, I think it's fine as-is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer "the port" because it's more specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
7. Let <var>processorPortSerialization</var> be | ||
7. Let <var>processorPortSerialization</var> be the result of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
I think I filed new issues for the items I was not sure about. This is ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but this has a conflict with #1875.
index.bs
Outdated
@@ -9560,8 +9560,8 @@ within a special scope named {{AudioWorkletGlobalScope}}. | |||
|
|||
Each {{BaseAudioContext}} possesses exactly one | |||
{{AudioWorklet}}. Scripts imported using this {{AudioWorklet}} | |||
are run in one or more {{AudioWorkletGlobalScope}} global scopes, | |||
which are created to process audio associated with that | |||
are run in one {{AudioWorkletGlobalScope}} global scope, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove "global scope" at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -10035,16 +10036,16 @@ Constructors</h5> | |||
: <dfn>port</dfn> | |||
:: | |||
Every {{AudioWorkletProcessor}} has an associated | |||
<code>port</code> which is a {{MessagePort}}. It is connected to the port on the | |||
<code>port</code> which is a {{MessagePort}}. It is connected to a port on the | |||
corresponding {{AudioWorkletProcessor}} object allowing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This port is created by MessageChannel. So the association between port1 and port2 are already implied.
@@ -10067,7 +10068,7 @@ in the node. | |||
* {{[[callable process]]}} is `true`. | |||
|
|||
* The {{AudioWorkletNode}} is [=actively processing=], meaning | |||
ANY of the following states are met: | |||
ANY of the following conditions are met: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, well, I was only reading the current spec, not considering things PRs that might change things. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can merge this first, and then I can handle the conflict in #1875.
|
||
7. Let <var>processorPortSerialization</var> be | ||
7. Let <var>processorPortSerialization</var> be the result of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
@@ -9814,7 +9814,8 @@ This interface has "entries", "forEach", "get", "has", "keys", | |||
<pre class="idl"> | |||
[Exposed=Window, | |||
SecureContext, | |||
Constructor (BaseAudioContext context, DOMString name, optional AudioWorkletNodeOptions options)] | |||
Constructor (BaseAudioContext context, DOMString name, | |||
optional AudioWorkletNodeOptions options)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
{{AudioNode/channelCount}}. | ||
|
||
: <dfn>parameterData</dfn> | ||
:: | ||
This is a list of user-defined key-value pairs that are used | ||
to initialize {{AudioParam}} values in | ||
to initialize {{AudioParam}} objects in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -10035,16 +10036,16 @@ Constructors</h5> | |||
: <dfn>port</dfn> | |||
:: | |||
Every {{AudioWorkletProcessor}} has an associated | |||
<code>port</code> which is a {{MessagePort}}. It is connected to the port on the | |||
<code>port</code> which is a {{MessagePort}}. It is connected to a port on the | |||
corresponding {{AudioWorkletProcessor}} object allowing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer "the port" because it's more specific.
@@ -10067,7 +10068,7 @@ in the node. | |||
* {{[[callable process]]}} is `true`. | |||
|
|||
* The {{AudioWorkletNode}} is [=actively processing=], meaning | |||
ANY of the following states are met: | |||
ANY of the following conditions are met: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can merge this first, and then I can handle the conflict in #1875.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed nits.
@@ -10035,16 +10036,16 @@ Constructors</h5> | |||
: <dfn>port</dfn> | |||
:: | |||
Every {{AudioWorkletProcessor}} has an associated | |||
<code>port</code> which is a {{MessagePort}}. It is connected to the port on the | |||
<code>port</code> which is a {{MessagePort}}. It is connected to a port on the | |||
corresponding {{AudioWorkletProcessor}} object allowing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Preview | Diff