Re: [whatwg/xhr] Integrate feature policy (#177)

annevk commented on this pull request.

Couple nits left, mostly looking good now. Thanks.

> @@ -29,6 +29,9 @@ spec:dom; type:interface; text:Document
 <pre class=anchors>
 urlPrefix: https://w3c.github.io/DOM-Parsing/; spec: dom-parsing
     type: dfn; text: fragment serializing algorithm; url: dfn-fragment-serializing-algorithm
+urlPrefix: https://wicg.github.io/feature-policy/; spec: FEATURE-POLICY
+    type: dfn; text: policy-controlled feature; url: policy-controlled-feature
+    type: dfn; text: default allowlist; url: default-allowlist

These should be properly exported by Feature Policy directly. This anchors block is only there for legacy.

> @@ -1021,6 +1025,12 @@ method must run these steps:
   <p>Otherwise, if the <a>synchronous flag</a> is set, run these substeps:
 
   <ol>
+   <li>
+    <p>If <a>context object</a>'s <a>relevant settings object</a> has a
+    <a>responsible document</a> and it is <em>not</em> <a>allowed to use</a> the
+    <code><a>sync-xhr</a></code> feature, then run <a>handle response

If this is meant to be a string I think it should be `"<code><a>sync-xhr</a></code>"` here and below. That's how you write down strings per Infra.

Also, no wrapping inside phrasing elements.

> @@ -2031,6 +2041,12 @@ attributes initialized to false, so it is suggested that for consistency all
 {{ProgressEvent}} interface do the same.
 
 
+<h3 id=feature-policy>Feature Policy Integration</h3>
+
+<p>This specification defines a <a>policy-controlled feature</a> identified by
+the string <code><dfn>sync-xhr</dfn></code>. Its <a>default allowlist</a> is
+<code>*</code>.

More can be on one line here. Please use 100 columns for new/modified text.

> @@ -2031,6 +2041,12 @@ attributes initialized to false, so it is suggested that for consistency all
 {{ProgressEvent}} interface do the same.
 
 
+<h3 id=feature-policy>Feature Policy Integration</h3>
+
+<p>This specification defines a <a>policy-controlled feature</a> identified by
+the string <code><dfn>sync-xhr</dfn></code>. Its <a>default allowlist</a> is
+<code>*</code>.
+

You want an extra newline here for consistency.

> @@ -1021,6 +1025,12 @@ method must run these steps:
   <p>Otherwise, if the <a>synchronous flag</a> is set, run these substeps:
 
   <ol>
+   <li>
+    <p>If <a>context object</a>'s <a>relevant settings object</a> has a
+    <a>responsible document</a> and it is <em>not</em> <a>allowed to use</a> the
+    <code><a>sync-xhr</a></code> feature, then run <a>handle response

Oh, also, since the `<p>` is the only child you need to do `<li><p>...`.

> @@ -1021,6 +1025,12 @@ method must run these steps:
   <p>Otherwise, if the <a>synchronous flag</a> is set, run these substeps:
 
   <ol>
+   <li>
+    <p>If <a>context object</a>'s <a>relevant settings object</a> has a
+    <a>responsible document</a> and it is <em>not</em> <a>allowed to use</a> the

Should this be "and that"/"which is"? To me it seems that "it" refers to a settings object here.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/xhr/pull/177#pullrequestreview-91516116