-
Notifications
You must be signed in to change notification settings - Fork 66
spec: Make ShareData argument optional. #51
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
@sammc ptal. Web platform tests updated here: web-platform-tests/wpt@master...mgiuca:web-share-errors-and-arguments |
index.html
Outdated
<a data-cite= | ||
"!WEBIDL#exceptiondef-typeerror"><code>TypeError</code></a> if called | ||
with no arguments. WebIDL <a data-cite= | ||
"!WEBIDL#idl-operations">mandates that it be optional</a>, because |
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 don't think you want a comma here.
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.
Done.
index.html
Outdated
}; | ||
</pre> | ||
<div class="note"> | ||
The <var>data</var> argument is marked as "optional", but really it |
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.
The really feels a bit superfluous.
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.
Shane suggested I write a comment explaining why this is here, since it's quite unintuitive to have an optional argument that is mandatory.
In particular, I think while developers may not read the whole spec, the one thing that is fairly understandable to a non-web-spec person is IDL, so we may have confused developers reading the IDL and wondering why this is "optional". That's why I'm so strongly opposed to adding a misleading "optional" here, but if we are going to have it, I'd rather explain it.
If you'd like I can reduce the amount of text here and perhaps convert it into a //
comment in the IDL itself rather than a note.
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 meant just the word "really." The rest is fine.
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.
Oh lol, use quotes next time. (I read it as "This really feel a bit superfluous".)
index.html
Outdated
}; | ||
</pre> | ||
<div class="note"> | ||
The <var>data</var> argument is marked as "optional", but really it |
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 meant just the word "really." The rest is fine.
This has no behavioural change, as passing an empty dictionary is a TypeError. But it is required by the WebIDL spec. Closes w3c#47.
This has no behavioural change, as passing an empty dictionary is a
TypeError. But it is required by the WebIDL spec.
Closes #47.
Preview | Diff