-
Notifications
You must be signed in to change notification settings - Fork 43
Add PushManager.supportedContentEncodings #252
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
Conversation
I'm sure the phrasing can be improved, @martinthomson. Most open to suggestions! |
index.html
Outdated
@@ -314,6 +314,10 @@ | |||
specification assumes the use of this protocol; alternative protocols are expected to | |||
provide compatible semantics. | |||
</p> | |||
<p> | |||
The <dfn>Content-Encoding</dfn> HTTP header, described in section 3.1.2.2. of [[!RFC7231]], |
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 RFC style guide references sections using "Section 3.1.2.2", capital on Section and no trailing period.
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.
@@ -642,12 +646,25 @@ | |||
</p> | |||
<pre class="idl"> | |||
interface PushManager { | |||
[SameObject] static readonly attribute FrozenArray<DOMString> supportedContentEncodings; |
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.
Hmm, it's called a content coding, but the header is content-encoding. I think that supportedContentEncodings
is probably the best choice, because other than us nerds who work on this stuff, the difference is meaningless.
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.
Agreed - the developer is exposed to the Content-Encoding header more than the fact it accepts a (list of) content coding.
index.html
Outdated
Promise<PushSubscription> subscribe(optional PushSubscriptionOptionsInit options); | ||
Promise<PushSubscription?> getSubscription(); | ||
Promise<PushPermissionState> permissionState(optional PushSubscriptionOptionsInit options); | ||
}; | ||
</pre> | ||
<p> | ||
The <dfn>supportedContentEncodings</dfn> attribute exposes the sequence of supported content | ||
codings using which the payload of a <a>push message</a> can be encrypted. When a content |
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.
"... codings that can be used to encrypt the payload of a push message."
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.
index.html
Outdated
The <dfn>supportedContentEncodings</dfn> attribute exposes the sequence of supported content | ||
codings using which the payload of a <a>push message</a> can be encrypted. When a content | ||
coding is used, it MUST be set in the <a>Content-Encoding</a> HTTP header when sending a | ||
<a>push message</a> to the <a>push service</a>. |
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 don't think that you want the MUST requirement here.
"A content coding is indicated using the Content-Encoding header field when requesting the sending of a push message from the push service." ...Maybe.
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. I considered changing it around (sending from the AS) but there's other cases too, so I adopted your suggestion.
72e89f4
to
0bae071
Compare
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.
Thanks!
index.html
Outdated
@@ -314,6 +314,10 @@ | |||
specification assumes the use of this protocol; alternative protocols are expected to | |||
provide compatible semantics. | |||
</p> | |||
<p> | |||
The <dfn>Content-Encoding</dfn> HTTP header, described in section 3.1.2.2. of [[!RFC7231]], |
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.
@@ -642,12 +646,25 @@ | |||
</p> | |||
<pre class="idl"> | |||
interface PushManager { | |||
[SameObject] static readonly attribute FrozenArray<DOMString> supportedContentEncodings; |
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.
Agreed - the developer is exposed to the Content-Encoding header more than the fact it accepts a (list of) content coding.
index.html
Outdated
Promise<PushSubscription> subscribe(optional PushSubscriptionOptionsInit options); | ||
Promise<PushSubscription?> getSubscription(); | ||
Promise<PushPermissionState> permissionState(optional PushSubscriptionOptionsInit options); | ||
}; | ||
</pre> | ||
<p> | ||
The <dfn>supportedContentEncodings</dfn> attribute exposes the sequence of supported content | ||
codings using which the payload of a <a>push message</a> can be encrypted. When a content |
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.
index.html
Outdated
The <dfn>supportedContentEncodings</dfn> attribute exposes the sequence of supported content | ||
codings using which the payload of a <a>push message</a> can be encrypted. When a content | ||
coding is used, it MUST be set in the <a>Content-Encoding</a> HTTP header when sending a | ||
<a>push message</a> to the <a>push service</a>. |
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. I considered changing it around (sending from the AS) but there's other cases too, so I adopted your suggestion.
Fixes #251