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

Clarify+make fitness distance algorithm agnostic to dictionary member presence. #526

Merged
merged 5 commits into from
Jul 12, 2018

Conversation

jan-ivar
Copy link
Member

@jan-ivar jan-ivar commented Jul 9, 2018

Fix for #525. @bzbarsky PTAL.

0.</li>
<li>If no ideal value is specified (<var>constraintValue</var>
either contains no member named 'ideal', or is itself a bare
value and bare values are to be treated as 'ideal'), the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

grammar error here - the second part is in the positive tense and should have been in the negative tense - may be better said as "either contains no member named 'ideal' or, if bare values are to be treated as 'ideal', isn't a bare value".

Copy link
Contributor

@alvestrand alvestrand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, apart from the nit.

@jan-ivar
Copy link
Member Author

jan-ivar commented Jul 11, 2018

@alvestrand Had to add more. Can you take another look? This is to allow whatwg/webidl#76 removing the invariant that member dictionary members are always present.

This means we have to test for undefined (using present) and null because unions, as I understand it. @bzbarsky ptal.

@bzbarsky
Copy link

This means we have to test for undefined (using present)

Correct.

and null because unions

What? Why?

@jan-ivar
Copy link
Member Author

@bzbarsky You're right, you explained to me that null is a valid value for an IDL dictionary type and gets converted to empty dictionary. Will fix.

@alvestrand alvestrand merged commit c89deeb into w3c:master Jul 12, 2018
@@ -4114,15 +4114,19 @@ <h2>Methods</h2>
value being compared against.</p>
<p>We define the <dfn>fitness distance</dfn> between a
<a>settings dictionary</a> and a constraint set <var>CS</var> as

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So a constraint set is a webidl dictionary, right? That's not really specified anywhere.

Also, https://w3c.github.io/mediacapture-main/getusermedia.html#dfn-selectsettings should make it clear that it has a constraint set argument, not an ambient constraint set.

<var>CS</var>, of the following values:</p>
<ol>
<li>
<p>If the constraint is not supported by the browser, the
fitness distance is 0.</p>
<p>If <var>constraintName</var> is not supported by the

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is a little odd, actually.

The input to this thing is a WebIDL dictionary, right? If a browser doesn't support some random spec that defines some constraint, it wouldn't know to include that name in the dictionary to start with, so it would not trigger this clause.

Is this clause meant to just apply to constraints that the browser knows about in the sense of having them in its IDL but doesn't actually implement matching for? Or is it meant to apply to constraints that the browser really doesn't know about? In the latter case, the input here obviously can't be a dictionary, because those presuppose a fixed set of known members and ignore everything else.

What do UAs actually implement here?

0.</li>
<li>If no ideal value is specified (<var>constraintValue</var>
either contains no member named 'ideal', or, if bare values
are to be treated as 'ideal', isn't a bare value), the

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks wrong to me. It's saying that if bare values are to be treated as ideal and constraintValue is not a bare value then the ideal value is not specified. That's clearly not the intent, but that's what it says.

I would strongly urge clarity of processing model over brevity here. If we have an "ideal value is specified" boolean with a complicated way to compute it, we should compute it in some substeps instead of trying to jam the computation into a parenthetical. Code that does that is not considered a good idea; neither are specs that do it.

<li>If no ideal value is specified (<var>constraintValue</var>
either contains no member named 'ideal', or, if bare values
are to be treated as 'ideal', isn't a bare value), the
fitness distance is 0.</li>
<li>For all positive numeric non-required constraints (such as

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "numeric" mean here? Is that a property a constraint can have as part of its definition (in which case please link to the concept's definition) or something else?

@@ -4185,7 +4189,7 @@ <h2>Methods</h2>
<li>Each <a data-lt="constraints">constraint</a> specifies one
or more values (or a range of values) for its property. A
property MAY appear more than once in the list of 'advanced'
ConstraintSets. If an empty object or list has been given as
ConstraintSets. If an empty list has been given as

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's really not clear to me what cases this is supposed to be handling. Is this dealing with constraints that are sequence-valued?

@jan-ivar jan-ivar deleted the notpresent branch March 5, 2019 21:50
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