-
Notifications
You must be signed in to change notification settings - Fork 66
Make ShareData members non-nullable #20
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
@annevk, there's a member of URL type here too, should those always be USVString? |
Yeah. |
OK, I've thrown that onto the branch. For the commit messages to make sense, this should either be merged or rebased as two commits, if squashed the commit message needs updating. |
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.
Ah thanks for pointing this out. I was using ?
to mean "optional", but I see that in a dictionary context, all fields are optional by default, and the ?
adds the additional (unnecessary) possibility of it being supplied but null.
The first change is fine, but not sure about USVString
.
docs/interface.md
Outdated
DOMString? url; | ||
DOMString title; | ||
DOMString text; | ||
USVString url; |
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'm struggling to understand why we use USVString for URLs and DOMString for other things.
Looking at the WebIDL spec, it looks like DOMString is a sequence of 16-bit code units (i.e., a UTF-16 string that can have surrogate encoding errors) and a USVString is a sequence of Unicode code points (i.e., a string of unspecified encoding that has to be valid).
USVString seems like a desirable property for any string attribute, not just URLs. Why the inconsistency?
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.
URLs need to use UTF-8 encode. Distinction between JavaScript strings (DOMString) and scalar value strings (USVString) is best explained here: https://infra.spec.whatwg.org/#strings. (IDL will be updated to use Infra in due course.)
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.
@annevk knows the details, but basically anything that will end up serialized (encoded) and sent over the network benefits from using USVString, because then one can merely encode from Unicode code points, instead of first "decoding" the sequence of 16-bit values into Unicode.
Some things that aren't URLs also fit this description, for example the label argument here:
https://w3c.github.io/webrtc-pc/#peer-to-peer-data-api
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 for the explanation.
but basically anything that will end up serialized (encoded) and sent over the network benefits
Well we don't know what's going to happen to these fields... they're going to experience all sorts of transformations. In Chrome's Web Share Target prototype they are going to be encoded as UTF-8 and put into a URL. They could also be passed to arbitrarily many native apps that could be written in any programming language. It isn't a good thing if they can contain in valid UTF-16 surrogates.
Could all of these be USVString (ech my fingers keep typing "USBString")? Or do I have to have a specific reason to use that type?
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.
It sounds like you have a reason to use USVString here, although the reason wouldn't be apparent from looking at the spec along. @annevk, WDYT, is there a reason to not use USVString in a situation like this?
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.
This is rather unique and without precedent I think, but it sounds like a good reason to avoid surrogates.
Oh I didn't see the follow-up discussion. Sounds good, merging. I might later add a quick note about why USVString is being used instead of DOMString. |
Fixes #19