Re: [w3c/manifest] Adding protocol handlers (#972)

@mgiuca commented on this pull request.

Some drive-by comments.

> +          via custom protocol links.
+        </p>
+        <p>
+          How protocol handlers are presented, and how many of them are shown
+          to the user, is at the discretion of the user agent and/or operating
+          system.
+        </p>
+        <p>
+          To <dfn>process the `protocol_handlers` member</dfn>, given
+          [=object=] |json:JSON|, |manifest:ordered map|, and |manifest
+          URL:URL|:
+        </p>
+        <ol>
+          <li>Let <var>processedProtocolHandlers</var> be a new [=list=].
+          </li>
+          <li>[=list/For each=] <var>protocol_handler</var> (<a>protocol

Be consistent with variable naming. Looks like we're using camelCase, so `protocolHandler` not `protocol_handler`.

Question for @kenchris : did this change recently? I'm looking at the shortcuts algorithm above which uses variables like _processedShortcuts_. I swear we used to use _variable names with spaces in them_, not _camelCaseVariableNames_.

> +        </p>
+        <p>
+          How protocol handlers are presented, and how many of them are shown
+          to the user, is at the discretion of the user agent and/or operating
+          system.
+        </p>
+        <p>
+          To <dfn>process the `protocol_handlers` member</dfn>, given
+          [=object=] |json:JSON|, |manifest:ordered map|, and |manifest
+          URL:URL|:
+        </p>
+        <ol>
+          <li>Let <var>processedProtocolHandlers</var> be a new [=list=].
+          </li>
+          <li>[=list/For each=] <var>protocol_handler</var> (<a>protocol
+          handler description</a>):

"of _json_["protocol_handlers"]"

> +          communication where one app directly invokes another and passes data
+          via custom protocol links.
+        </p>
+        <p>
+          How protocol handlers are presented, and how many of them are shown
+          to the user, is at the discretion of the user agent and/or operating
+          system.
+        </p>
+        <p>
+          To <dfn>process the `protocol_handlers` member</dfn>, given
+          [=object=] |json:JSON|, |manifest:ordered map|, and |manifest
+          URL:URL|:
+        </p>
+        <ol>
+          <li>Let <var>processedProtocolHandlers</var> be a new [=list=].
+          </li>

Need to add a step: "Set _manifest_["protocol_handlers"] to _processedProtocolHandlers_". Otherwise this result isn't going anywhere.

> +                the base URL, and [=this=]'s relevant [=environment settings
+                object=]. If the result is failure, [=iteration/continue=].
+              </li>
+            </ol>
+          </li>
+          <li>If [=normalizedUrl=] is not [=manifest/within scope=] of
+          <var>manifest URL</var>, [=iteration/continue=].
+          </li>
+          <li>If [=normalizedUrl=] already exists in
+          <var>processedProtocolHandlers</var>, [=iteration/continue=].
+          </li>
+          <li>[=List/Append=] |protocol_handler| to
+          <var>processedProtocolHandlers</var>.
+          </li>
+        </ol>
+        <ul>

Formatting: Why is there a `<ul>` here with one `li` in it, and then a bunch of other things, then a `</ul>`? Was it meant to be closed after the `li` but you forgot, and then tidy-html added it way down there?

> +                object=]. If the result is failure, [=iteration/continue=].
+              </li>
+            </ol>
+          </li>
+          <li>If [=normalizedUrl=] is not [=manifest/within scope=] of
+          <var>manifest URL</var>, [=iteration/continue=].
+          </li>
+          <li>If [=normalizedUrl=] already exists in
+          <var>processedProtocolHandlers</var>, [=iteration/continue=].
+          </li>
+          <li>[=List/Append=] |protocol_handler| to
+          <var>processedProtocolHandlers</var>.
+          </li>
+        </ol>
+        <ul>
+          <li>[=list/For each=] |processedProtocolHandlers|, [=register a

I don't think this can appear here without context (not part of any algorithm). It doesn't say when it should happen, or what the requirement level is (I think it should be a SHOULD; without that it implies MUST).

At a minimum, this should be in an algorithm "To register the protocol handlers for a processed manifest _manifest_", then you can properly index into that manifest in the for loop, e.g. "For each _protocolHandler_ of _manifest_["protocol_handlers"]".

Ideally, this would be called from an algorithm that's invoked at install time, but I don't see one of those. So maybe just add a note to the "Installable web applications" section to say the user agent MAY `[=`register the protocol handlers`=]`".

> +      <section>
+        <h3>
+          <dfn>Handling a protocol launch</dfn>
+        </h3>
+        <p>
+          When a <a>protocol handler description</a>
+          <var>protocol_handler</var> having [=manifest=] <var>manifest</var>
+          is invoked, it goes through the same steps used to [=invoke a
+          protocol handler=] defined in [=HTML=], where the user agent SHOULD
+          navigate to [=url=] and the appropriate browsing context is set to a
+          new top level browsing context.
+        </p>
+      </section>
+      <section>
+        <h3>
+          Privacy and Security considerations

I'm not sure this should go here. There's a main P&S section at the end of the document that collects all of these (this would be the only section to have it's own P&S subsection, other than Installable Web Apps, and that is probably a mistake too -- note that it is duplicated text from the end of the document).

However, I think P&S should only have non-normative discussion; a MUST requirement shouldn't be in it. So I would move this MUST requirement above to Handling a protocol launch. Then explain why it is so in the P&S section.

> +          navigate to [=url=] and the appropriate browsing context is set to a
+          new top level browsing context.
+        </p>
+      </section>
+      <section>
+        <h3>
+          Privacy and Security considerations
+        </h3>
+        <p>
+          Privacy concerns for custom scheme handling are detailed in the
+          Security and privacy section of
+          {{NavigatorContentUtils/registerProtocolHandler()}}.
+        </p>
+        <p>
+          The user agent MUST ask for permission when using a protocol handler
+          for the first time. This feature requires user interaction and a

I don't understand this second sentence. Are you trying to write a normative requirement that registration requires user interaction? Are you justifying the previous MUST requirement? Please rephrase this.

> @@ -2403,6 +2601,7 @@ <h4>
           </p>
           <pre class="example js" title=
           "Accessing 'display-mode' media feature with script">
+

stray blank line

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/w3c/manifest/pull/972#pullrequestreview-716705942