Giving startOrOptions a default value for IDL conformance. #65
Conversation
According to the Web IDL spec, section 2.5.3 (some formatting is mine): If - the type of an argument is a dictionary type or a union type that has a dictionary type as one of its flattened member types, - and that dictionary type and its ancestors have no required members, - and the argument is either the final argument or is followed only by optional arguments, then the argument must be specified as optional and have a default value provided. IIUC, the 'startOrOptions' parameter for Performance.Measure falls into the category where it needs to be marked optional and given a default value. Right now it is marked optional but does not have a default value. This change adds a default value of an empty dictionary for the startOrOptions parameter. This should not change any user-visible behaviour because an omitted paremeter would have been treated as an empty dictionary in the bindings layer anyways.
|
This was a recent addition in whatwg/webidl#750. Chrome currently is not even able to parse such language, see https://bugs.chromium.org/p/chromium/issues/detail?id=948139#c6. Therefore, I'm unsure if we should immediately attempt to follow this rule. We (and possibly other browser vendors) cannot implement it until bindings has been fixed. @yoavweiss thoughts? |
|
Also the ipr failure needs to be fixed https://labs.w3.org/repo-manager/pr/id/w3c/user-timing/65 |
|
Oops, I didn't realize that change to IDL was so new. +1 for waiting to merge until IDL support is updated. For the ipr failure, I think I linked my accounts up correctly... Are there any other steps that need to be done? |
|
Marked as non substantive for IPR from ash-nazg. |
|
Catching up on comments... we implement the new syntax in Gecko. |
Co-Authored-By: Marcos Cáceres <marcos@marcosc.com>
marcoscaceres
left a comment
There was a problem hiding this comment.
Just noting Chromium can’t parse this syntax yet, but otherwise all good. It has no noticeable change in userland code.
|
Oops, thought this was a new pull request. Serves me right for reviewing stuff on my phone 😊 |
npm1
left a comment
There was a problem hiding this comment.
Looks like ReSpec is marking in red the whole WebIDL section just because it fails to contain the newly added "{}" syntax. That seems a bit excessive. LGTM to make the spec pretty again.
|
Missed one in the constructor of PerformanceMark if you want to do another PR for that @tommckee1 :) |
Similar to w3c#65 but the 'markOptions' argument to the PerformanceMark constructor needs a default value too.
According to the Web IDL spec, section 2.5.3 (some formatting is mine):
If
a dictionary type as one of its flattened member types,
optional arguments,
then the argument must be specified as optional and have a default value
provided.
IIUC, the 'startOrOptions' parameter for Performance.Measure falls into
the category where it needs to be marked optional and given a default
value. Right now it is marked optional but does not have a default
value.
This change adds a default value of an empty dictionary for the
startOrOptions parameter. This should not change any user-visible
behaviour because an omitted paremeter would have been treated as an
empty dictionary in the bindings layer anyways.
Preview | Diff