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 User Experience requirements section describing permissions display requirements #411

Closed
wants to merge 3 commits into from

Conversation

burnburn
Copy link
Contributor

@burnburn burnburn commented Oct 13, 2016

This is the second part of the permissions display update (see issue #387) that uses the indicator values defined in PR #401.

@burnburn
Copy link
Contributor Author

@jan-ivar can you please review?

@stefhak
Copy link
Contributor

stefhak commented Oct 13, 2016

@jan-ivar please review.

Copy link
Member

@jan-ivar jan-ivar left a comment

Choose a reason for hiding this comment

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

Great! The logic seems sound to me, and everything seems covered. We need s/device/deviceId/ for indexes, other than that, I have mostly editorial concerns about clarity on a couple of points.

I wonder if there are ways to compact this, now that it's all together, but maybe we should just get it in and iterate later.

@@ -5026,6 +5026,83 @@
</div>
</section>
<section>
<h1>User Experience Requirements</h1>
Copy link
Member

Choose a reason for hiding this comment

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

Should we call this something without UX in it, since we've worked hard to take all actual UX parts out?

Like: "Privacy Indicator Requirements" ?

<p>For each <var>device</var> for which there is a
[[\devicesLiveMap]]<var>[device]</var> internal slot,
define <var>&lt;device&gt;Live</var> to be On whenever the
slot has a value of true and Off otherwise.</p>
Copy link
Member

Choose a reason for hiding this comment

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

The indexes must be deviceIds the way it's currently written. Also, slots shouldn't ever be absent if we filter the same way as in 9.2, e.g.:

"For each individual device that getUserMedia() exposes, using the device's deviceId,

  • define <device>Accessible to be On whenever [[devicesAccessibleMap]][deviceId] is true and Off otherwise.
  • define <device>Live to be On whenever [[devicesLiveMap]][deviceId] is true and Off otherwise.

encouraged to make any device-specific hardware indicator light
match the corresponding <var>&lt;device&gt;Live</var>
value.</li>
<li>Any optional ongoing indication MAY be used instead of the
Copy link
Member

Choose a reason for hiding this comment

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

s/optional/of these/ ?

<li>The User Agent MUST indicate to the user when the value
of <var>Live</var> changes.</li>
<li>If the User Agent provides any indication of Accessible or
Live to the user per <var>kind</var>, then for each such
Copy link
Member

Choose a reason for hiding this comment

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

"Accessible or Live per kind" and "Accessible or Live per device" are a bit confusing, when Accessible and Live are the union states. Is there another way to say this?

whenever <var>&lt;device&gt;Live</var> is On for
any <var>device</var> of that <var>kind</var> and
Off otherwise.</li>
</ul>
Copy link
Member

Choose a reason for hiding this comment

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

Everything up to here is 1-1 renaming of names ([[fooMap]][barId] to <bar>foo) and states (true/false to On/Off). Might it simplify to skip this and inline everything below instead, settling for the slightly-worse-sounding "false-to-true transitions"?

I mention this because there are a lot of states to keep track of. I know the existing state terms are hideous, but they're more well-defined than the replacements (I worry we need to explain the <>).

hardware device indicator light match.</li>
<li>If the User Agent provides any indication
of <var>Accessible</var> or <var>Live</var> to the user
per <var>kind</var>, it is encouraged to provide ongoing
Copy link
Member

Choose a reason for hiding this comment

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

Accessible and Live are italicized here, implying the union states are being referenced here, which seems wrong. These can be concepts or states but not both.

</ul>
<p>Define <var>Accessible</var> to be On
whenever <var>&lt;kind&gt;Accessible</var> is On for
any <var>kind</var> and Off otherwise.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we rename Accessible as Any-Accessible, to separate it more visually from all the other Accessibles?

Copy link
Member

Choose a reason for hiding this comment

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

We'd need to do the same thing for Live then.

@burnburn
Copy link
Contributor Author

Still reviewing. Will reply during IETF week.

@burnburn
Copy link
Contributor Author

@jan-ivar I agree with the inlining proposal you sent me by email. Can you submit it as a replacement PR for this one and I'll close this one? I'm fine if you want to update it with the change from Accessible to Any-Accessible (and same for Live), or if you leave it. I think it's clear enough without that change, but I also don't think the addition of "Any" to the name makes things worse.

@alvestrand
Copy link
Contributor

Note - the reason why I think "Any-Accessible" is better than "Accessible" is that "Accessible" can be read by a casual reader as "All-Accessible", which is not what's intended.
A careful reader will understand what it is, so I don't care that much, but we do have casual readers.

@jan-ivar
Copy link
Member

jan-ivar commented Dec 7, 2016

@burnburn OK I'll do that.

@burnburn
Copy link
Contributor Author

burnburn commented Dec 15, 2016

Superceded by #421. Will close this if/when that PR resolves.

@burnburn burnburn closed this Dec 15, 2016
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.

4 participants