- From: Domenic Denicola <notifications@github.com>
- Date: Tue, 06 Mar 2018 20:50:54 -0800
- To: w3c/payment-request <payment-request@noreply.github.com>
- Cc: Subscribed <subscribed@noreply.github.com>
- Message-ID: <w3c/payment-request/pull/654/review/101806158@github.com>
domenic requested changes on this pull request.
Lots of editorial issues, and a few questions. Overall pretty solid stuff though!
> - <ol data-link-for="PaymentAddress">
- <li>Let <var>address</var> be a new instance of
- <a>PaymentAddress</a>.
- </li>
- <li>Set <var>address</var>.<a>[[\addressLine]]</a> to the result of
- splitting the user-provided address line into a <a data-cite=
+ <section>
+ <h2>
+ Internal Constructor
+ </h2>
+ <p class="note">
+ This constructor algorithm is used internally by the user agent.
+ It's not available to scripts.
+ </p>
+ <p>
+ The steps to <dfn data-lt=
This is only called from one place, right? I guess it's a separate algorithm because you plan to add a constructor soon? If so make sure to call it out in the commit message, and perhaps in the spec (although that spec call out would disappear shortly).
> + <h2>
+ Internal Constructor
+ </h2>
+ <p class="note">
+ This constructor algorithm is used internally by the user agent.
+ It's not available to scripts.
+ </p>
+ <p>
+ The steps to <dfn data-lt=
+ "PaymentAddress.PaymentAddress()">internally construct a
+ <code>PaymentAddress</code></dfn> with <a>AddressInit</a>
+ <var>details</var> are given by the following algorithm:
+ </p>
+ <ol data-link-for="AddressInit">
+ <li>Let <var>address</var> be a new instance of
+ <a>PaymentAddress</a>.Set
Missing `<li>`
> + Internal Constructor
+ </h2>
+ <p class="note">
+ This constructor algorithm is used internally by the user agent.
+ It's not available to scripts.
+ </p>
+ <p>
+ The steps to <dfn data-lt=
+ "PaymentAddress.PaymentAddress()">internally construct a
+ <code>PaymentAddress</code></dfn> with <a>AddressInit</a>
+ <var>details</var> are given by the following algorithm:
+ </p>
+ <ol data-link-for="AddressInit">
+ <li>Let <var>address</var> be a new instance of
+ <a>PaymentAddress</a>.Set
+ <var>address</var>.<a>[[\addressLine]]</a>to the empty frozen
Missing space after `</a>`
> + <p class="note">
+ This constructor algorithm is used internally by the user agent.
+ It's not available to scripts.
+ </p>
+ <p>
+ The steps to <dfn data-lt=
+ "PaymentAddress.PaymentAddress()">internally construct a
+ <code>PaymentAddress</code></dfn> with <a>AddressInit</a>
+ <var>details</var> are given by the following algorithm:
+ </p>
+ <ol data-link-for="AddressInit">
+ <li>Let <var>address</var> be a new instance of
+ <a>PaymentAddress</a>.Set
+ <var>address</var>.<a>[[\addressLine]]</a>to the empty frozen
+ array, and all other <a data-lt="PaymentAddress slots">internal
+ slots</a>set to the empty string.
Missing space and extra "set" after `</a>`
> + </p>
+ <p>
+ The steps to <dfn data-lt=
+ "PaymentAddress.PaymentAddress()">internally construct a
+ <code>PaymentAddress</code></dfn> with <a>AddressInit</a>
+ <var>details</var> are given by the following algorithm:
+ </p>
+ <ol data-link-for="AddressInit">
+ <li>Let <var>address</var> be a new instance of
+ <a>PaymentAddress</a>.Set
+ <var>address</var>.<a>[[\addressLine]]</a>to the empty frozen
+ array, and all other <a data-lt="PaymentAddress slots">internal
+ slots</a>set to the empty string.
+ </li>
+ <li>If <var>details</var> was not passed to the constructor, return
+ <var>address</var> and terminate this algorithm.
What constructor?
Is details meant to be an optional argument to this algorithm? If so it should be stated ("with an optional AddressInit details")
These days we're not saying "return and terminate this algorithm"; just returning suffices (like in any reasonable programming language).
> + The steps to <dfn data-lt=
+ "PaymentAddress.PaymentAddress()">internally construct a
+ <code>PaymentAddress</code></dfn> with <a>AddressInit</a>
+ <var>details</var> are given by the following algorithm:
+ </p>
+ <ol data-link-for="AddressInit">
+ <li>Let <var>address</var> be a new instance of
+ <a>PaymentAddress</a>.Set
+ <var>address</var>.<a>[[\addressLine]]</a>to the empty frozen
+ array, and all other <a data-lt="PaymentAddress slots">internal
+ slots</a>set to the empty string.
+ </li>
+ <li>If <var>details</var> was not passed to the constructor, return
+ <var>address</var> and terminate this algorithm.
+ </li>
+ <li>If <var>details</var>["<a>country</a>"] is present:
Should we strip whitespace from country too?
> + </li>
+ <li>If <var>details</var>["<a>languageCode</a>"] is present:
+ <ol>
+ <li>If <a data-cite=
+ "ecma-402#sec-isstructurallyvalidlanguagetag">IsStructurallyValidLanguageTag</a>(<var>details</var>["<a>languageCode</a>"])
+ is false, throw a <a>RangeError</a> exception.
+ </li>
+ <li>Set <var>address</var>.<a>[[\languageCode]]</a> to
+ <a data-cite=
+ "ecma-402#sec-canonicalizelanguagetag">CanonicalizeLanguageTag</a>(<var>details</var>["<a>languageCode</a>"]).
+ </li>
+ </ol>
+ </li>
+ <li>If <var>details</var>["<a>addressLine</a>"] is present, set
+ <var>address</var>.<a>[[\addressLine]]</a> to a new frozen array
+ created from <var>details</var>["<a>addressLine</a>"].
Should we strip whitespace from each address line perhaps?
> + </section>
+ <section>
+ <h2>
+ <dfn>toJSON()</dfn> method
+ </h2>
+ <p>
+ When called, runs [[!WEBIDL]]'s <a data-cite=
+ "WEBIDL#default-tojson-operation">default toJSON operation</a>.
+ </p>
+ </section>
+ <section>
+ <h2>
+ <dfn>country</dfn> attribute
+ </h2>
+ <p>
+ When getting, returns the value of the <a>PaymentAddress</a>'s
Missing a "represents"
> + <section>
+ <h2>
+ <dfn>addressLine</dfn> attribute
+ </h2>
+ <p>
+ Represents the <a>address line</a> of the address. When getting,
+ returns the value of the <a>PaymentAddress</a>'s
+ <a>[[\addressLine]]</a> internal slot.
+ </p>
+ </section>
+ <section>
+ <h2>
+ <dfn>region</dfn> attribute
+ </h2>
+ <p>
+ Represents the <a>region</a> of the address. When getting, returns
Links to the wrong place, here and in several other "represents". You can tell because in the preview they are in code font, but they should not be.
> + DOMString organization;
+ DOMString recipient;
+ DOMString phone;
+ };
+ </pre>
+ <p>
+ A <a>AddressInit</a> is passed when <a data-lt=
+ "PaymentAddress.PaymentAddress()">constructing</a> a
+ <a>PaymentAddress</a>. Its members are as follows.
+ </p>
+ <dl data-dfn-for="AddressInit" data-link-for="" data-sort="ascending">
+ <dt>
+ <dfn>country</dfn> member
+ </dt>
+ <dd>
+ An <a>country</a>, represented as a a [[!ISO3166-1]] country code.
a a -> a
> + A <a>AddressInit</a> is passed when <a data-lt=
+ "PaymentAddress.PaymentAddress()">constructing</a> a
+ <a>PaymentAddress</a>. Its members are as follows.
+ </p>
+ <dl data-dfn-for="AddressInit" data-link-for="" data-sort="ascending">
+ <dt>
+ <dfn>country</dfn> member
+ </dt>
+ <dd>
+ An <a>country</a>, represented as a a [[!ISO3166-1]] country code.
+ </dd>
+ <dt>
+ <dfn>addressLine</dfn> member
+ </dt>
+ <dd>
+ An <a>address line</a>, represented as sequence.
as sequence -> as a sequence
> + </dd>
+ </dl>
+ </section>
+ <section>
+ <h2>
+ Creating a <code>PaymentAddress</code> from user-provided input
+ </h2>
+ <p>
+ The steps to <dfn>create a <code>PaymentAddress</code> from
+ user-provided input</dfn> are given by the following algorithm. The
+ algorithm takes a <a>list</a> <var>redactList</var>, for which user
+ input will not be gathered.
+ </p>
+ <ol data-link-for="AddressInit">
+ <li>Let <var>details</var> be an object with no members that will
+ serve as a <a>AddressInit</a> dictionary.
Just say "be an AddressInit dictionary with no members present". It's technically not an object since we're not in JS-land, we're in Web IDL-land.
> </li>
- <li>Set <var>address</var>.<a>[[\region]]</a> to the user-provided
- region, or to the empty string if none was provided.
+ <li>If "region" is not in <var>redactList</var>:
+ <ol>
+ <li>Set <var>details</var>["<a>region</a>"] to the user-provided
This should not be a nested list, since none of the other steps are
> @@ -2855,8 +3170,13 @@ <h2>
<a>Queue a task</a> on the <a>user interaction task source</a> to
run the following steps:
<ol>
+ <li>Let <var>redactList</var> be the empty list. Optionally,
+ append the following items to <var>redactList</var>: «
+ "organization", "phone", "recipient", "addressLine" ».
Do you have to append all of them together, or can you pick a subset? If all together, I'd say "optionally, set redactList to « ... »" If a subset, then I'd say "optionally add any of the following to redactList: ..." (the latter with no «»s)
In other places with optional steps, we include a nice note explaining why someone would do this. Can you do the same here?
> <li>Let <var>address</var> be the result of running the steps to
- <a>create a payment address</a>.
+ <a>create a <code>PaymentAddress</code> from user-provided
+ input</a>.
Don't forget to pass redactList!
--
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/payment-request/pull/654#pullrequestreview-101806158
Received on Wednesday, 7 March 2018 04:51:17 UTC