Re: [w3c/clipboard-apis] Add Web Custom format (PR #175)

@BoCupp-Microsoft commented on this pull request.

Thanks for the changes!  I think if you address these comments I'll be ready to approve for a merge.

>      On Linux, ChromeOS, and Android, follow the convention described below:
 
     1. Assign "image/png" to |wellKnownFormat|.
 
-    1. Return |wellKnownFormat|.
+  1. Return |wellKnownFormat|.
+
+ </div><!-- algorithm -->
+
+ <div class="algorithm" data-algorithm="read-unsanitized-format">
+ <h3 id="to-read-unsanitized-format"><dfn>read unsanitized format</dfn></h3>
+
+  : Input
+  :: |mimeType|, a string

why don't you call this |customFormatKey| and include the parse a mime-type steps in this algorithm to transform that into a "web " prefixed and parsed name that can be used to construct the clipboard item.  the name you construct should probably be an output parameter.

>      On Linux, ChromeOS, and Android, follow the convention described below:
 
     1. Assign "image/png" to |wellKnownFormat|.
 
-    1. Return |wellKnownFormat|.
+  1. Return |wellKnownFormat|.
+
+ </div><!-- algorithm -->
+
+ <div class="algorithm" data-algorithm="read-unsanitized-format">
+ <h3 id="to-read-unsanitized-format"><dfn>read unsanitized format</dfn></h3>

Since we started calling everything web custom format and no unsanitized format can you change this to be consistent, i.e. Read Web Custom Format

> +
+  : Input
+  :: |mimeType|, a string
+
+  : Output
+  :: |item|, a {{Blob}}
+
+  1. Let |webCustomFormatMap| be the [=os specific custom map name=].
+
+  1. Read |webCustomFormatMap| from the [=system clipboard=].
+
+  1. Let |webCustomFormatMapString| be the JSON string deserialized from |webCustomFormatMap|.
+
+   Note: Need a JSON reader to deserialize the content from the |webCustomFormatMap|.
+
+  1. Let |webCustomFormat| be the value corresponding to the |mimeType| key in |webCustomFormatMapString|.

Can you make |webCustomFormat| be |webCustomFormatName| to make it clear that the value we're getting from the JSON is the name of the custom format on the system clipboard which actually contains the value?  It should actually be clear given the Read |webCustomFormat| step below already, so this is an optional nit.

> @@ -786,7 +786,7 @@ urlPrefix: https://w3c.github.io/FileAPI/#dfn-; type: dfn;
 
      1. Abort these steps.
 
-    1. Let |data| be a copy of the [=system clipboard data=] represented as [=clipboard items=]. For the MIME types defined in the [=mandatory data types=] list, |data| MAY be sanitized, but image/png format has unsanitized payload to preserve meta data.
+    1. Let |data| be a copy of the [=system clipboard data=] represented as [=clipboard items=]. For the MIME types defined in the [=mandatory data types=] list, |data| MAY be sanitized, but image/png format has unsanitized payload to preserve meta data. For the [=/MIME type=]s that have a "web "("web" followed by U+0020 SPACE) prefix, [=read unsanitized format=] from the [=system clipboard=] and append the content of the format to |data|.

This step is getting a bit long and does a lot.  Also, it sounds like if data is a copy of the system clipboard data that there will be more than just the mandatory mime-types and web custom formats in data.  

I believe we want a loop over the mandatory data types where we insert each into a data object that will be used to construct a single clipboarditem with multiple representations corresponding to each of those types.  

Additionally, we want a step here to first read the JSON from the Web Custom Format Map, and then we can loop over its keys too.  For each key we can obtain the name of the format corresponding to that mime-type on the system clipboard, read it, and then add its blob to data using a new key that is the result of running the key we got from the JSON through the parse a mime-type algorithm with "web " prepended.

Most of that can happen in the read helper function you have defined.  Consider returning the "web " prefixed and parsed name from that method in addition to the blob itself.

> +
+   1. Let |webCustomFormat| be the [=os specific custom name=].
+
+   1. Let |webCustomFormatIdx| be the result of appending |idx| to |webCustomFormat|.
+
+   1. Insert |mimeType| as key and |webCustomFormatIdx| as value into the |webCustomFormatMap|.
+
+    Note: Need a JSON writer to serialize the content into the |webCustomFormatMap|.
+
+   1. Insert the |item| into the [=system clipboard=] using |webCustomFormatIdx| as the format.
+
+   1. Increment |idx|.
+
+   1. If |idx| is greater than 100, then break from this loop.
+
+  1. Insert the |webCustomFormatMap| into the [=system clipboard=].

This should say something like insert the JSON serialized representation of |webCustomFormatMap| into the clipboard.

> +
+  : Input
+  :: |items|,  a [=sequence=]&lt;{{Blob}}&gt;
+  :: |mimeType|, a string
+
+  1. Let |idx| be a number initialized to 0.
+
+  1. Let |webCustomFormatMap| be the [=os specific custom map name=].
+
+  1. For each |item| in |items|:
+
+   1. Let |webCustomFormat| be the [=os specific custom name=].
+
+   1. Let |webCustomFormatIdx| be the result of appending |idx| to |webCustomFormat|.
+
+   1. Insert |mimeType| as key and |webCustomFormatIdx| as value into the |webCustomFormatMap|.

Before inserting mimeType, you should be stripping the "web " prefix.  I think the what follows the space is guaranteed to be a parsed as a mime-type value per the steps in the construction of the clipboarditem.

>      On Linux, ChromeOS, and Android, follow the convention described below:
 
     1. Assign "image/png" to |wellKnownFormat|.
 
-    1. Return |wellKnownFormat|.
+  1. Return |wellKnownFormat|.
+
+ </div><!-- algorithm -->
+
+ <div class="algorithm" data-algorithm="read-unsanitized-format">
+ <h3 id="to-read-unsanitized-format"><dfn>read unsanitized format</dfn></h3>
+
+  : Input
+  :: |mimeType|, a string

Also, to make it clear what's an acceptable key, when parsing a mime-type, you actually use the ParseMimeTypeWithoutParameter function in Chromium, so can you clarify in the steps that you take the type and sub-type and concatenate those outputs from the parse a mime-type algorithm with '/', but throw away the parameters?

https://chromium-review.googlesource.com/c/chromium/src/+/3650952

>      On Linux, ChromeOS, and Android, follow the convention described below:
 
     1. Assign "image/png" to |wellKnownFormat|.
 
-    1. Return |wellKnownFormat|.
+  1. Return |wellKnownFormat|.
+
+ </div><!-- algorithm -->
+
+ <div class="algorithm" data-algorithm="read-unsanitized-format">
+ <h3 id="to-read-unsanitized-format"><dfn>read unsanitized format</dfn></h3>
+
+  : Input
+  :: |mimeType|, a string

Lastly, the parse a mime type can fail (by returning failure per the spec) so can you clarify that if failure is returned what you will do?  I believe this function should also return failure and then you simply don't append anything to the collection of data used to construct the clipboarditems that will be given to the author.

> @@ -786,7 +786,7 @@ urlPrefix: https://w3c.github.io/FileAPI/#dfn-; type: dfn;
 
      1. Abort these steps.
 
-    1. Let |data| be a copy of the [=system clipboard data=] represented as [=clipboard items=]. For the MIME types defined in the [=mandatory data types=] list, |data| MAY be sanitized, but image/png format has unsanitized payload to preserve meta data.
+    1. Let |data| be a copy of the [=system clipboard data=] represented as [=clipboard items=]. For the MIME types defined in the [=mandatory data types=] list, |data| MAY be sanitized, but image/png format has unsanitized payload to preserve meta data. For the [=/MIME type=]s that have a "web "("web" followed by U+0020 SPACE) prefix, [=read unsanitized format=] from the [=system clipboard=] and append the content of the format to |data|.

Two additional nits:
1. Be sure to include MAY be sanitized as a step as you iterate over the mandatory data types to preserve what you added in your previous PR. 
2. Instead of looping over mandatory data types, you can loop over a set of data types supported by the user agent as a well-known format, which must at least include the mandatory data types listed in this spec.

> +
+  : Input
+  :: |items|,  a [=sequence=]&lt;{{Blob}}&gt;
+  :: |mimeType|, a string
+
+  1. Let |idx| be a number initialized to 0.
+
+  1. Let |webCustomFormatMap| be the [=os specific custom map name=].
+
+  1. For each |item| in |items|:
+
+   1. Let |webCustomFormat| be the [=os specific custom name=].
+
+   1. Let |webCustomFormatIdx| be the result of appending |idx| to |webCustomFormat|.
+
+   1. Insert |mimeType| as key and |webCustomFormatIdx| as value into the |webCustomFormatMap|.

It looks like we didn't update the ClipboardItem constuctor.  We have a step in there that runs the parse a mime-type algorithm, but it needs to test and allow for "web " prefixes.

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

Message ID: <w3c/clipboard-apis/pull/175/review/985175096@github.com>