Skip to content
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

Add IDL section and define IDL for all of the members #612

Closed
wants to merge 0 commits into from

Conversation

kenchris
Copy link
Collaborator

@kenchris kenchris commented Sep 19, 2017

Created an IDL section in appendix and defined some of the members as IDL.

I can continue this, but would like some initial feedback :-)


Preview | Diff

@mgiuca
Copy link
Collaborator

mgiuca commented Sep 20, 2017

Hi Ken,

This is a good start. Thanks for taking this; I wasn't expecting you to do it but if you're happy to continue, go ahead.

One point that keeps coming up is DOMString vs USVString. The difference is DOMString allows illegal surrogate pairs while USVString does not. The WebIDL spec tells you not to use USVString unless you have a good reason (the main accepted use of USVString is URLs). My opinion is the opposite; that USVString should generally be preferred because illegal surrogate pairs are meaningless, and cannot be represented outside of UTF-16. I think the rule of thumb should be that if the string characters are displayed to the user or leave the browser, then it should be a USVString, but if it's an internal string only (e.g., an ID string) then it can be a DOMString.

I successfully argued this logic in Web Share which is why the ShareData fields are USVStrings.

At the very least, all URL fields should be USVStrings. For Manifest, many of the strings will be exported out to the OS (e.g., on shortcuts) and illegal surrogate pairs may not be representable. I would advise the following strings be USVString:

  • name
  • short_name
  • description
  • scope
  • start_url
  • related_applications: all fields
  • categories
  • Image.src (but not size)

Agree/disagree?

@marcoscaceres
Copy link
Member

Apart from agreeing with @mgiuca about URLs, I don't have a strong opinion about the others.

@marcoscaceres
Copy link
Member

... but as @mgiuca points out, these strings leave the browser... so USVString might be safer.

@kenchris
Copy link
Collaborator Author

Great, that was actually one of those questions that I had. I will move it all to USVString

@kenchris kenchris changed the title Add IDL section and define IDL for some of the members Add IDL section and define IDL for all of the members Sep 20, 2017
@kenchris
Copy link
Collaborator Author

Ok, new version up, though I don't know how to fix the platform member because it is reused across two members and I cannot have two data-dfn-for. Help me Obi Wan Caceres @marcoscaceres, you are my only hope :-)

@mgiuca
Copy link
Collaborator

mgiuca commented Sep 21, 2017

How come this got reverted? (e0e053f)

@marcoscaceres
Copy link
Member

@mgiuca, @kenchris accidentally pushed on main branch, so he reverted it.

@marcoscaceres
Copy link
Member

The changes from e0e053f are in #613

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants