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

Text incorporating the Permissions API's definition of permission handling #319

Merged
merged 5 commits into from Apr 3, 2016

Conversation

alvestrand
Copy link
Contributor

This is text for review. ReSpec complains about one error still, but I can't see it.

Fixes #305

<code><a>MediaStream</a></code> object representing a media
stream.</p>
<p>Choose a device of each requested media type from
<var>finalSet</var>.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternate: Present the whole set to the user (somehow) and let them pick. Implementations should be allowed to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Incorporated.

@stefhak stefhak assigned martinthomson and unassigned adam-be Mar 3, 2016
@stefhak
Copy link
Contributor

stefhak commented Mar 3, 2016

@martinthomson we'd really like you to take a look at this to make sure it is compliant with FF UI.

@dontcallmedom
Copy link
Member

fwiw, the Travis error depends on getting fixed w3c/respec#597 I think

permission for the source in question, the given permission is revoked
is <dfn id="source-stopped">stopped</dfn>. If
the <a>permission</a> for the source is marked "transient", the
User Agent will <a href="#remove-permission">remove permission</a>,
Copy link
Member

Choose a reason for hiding this comment

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

The 'remove permission' link is broken. We could simply use <a>remove permission</a> to fix the link or <a href="#dfn-remove-permission">remove the permission</a> to make it more natural to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed this part. Not the best design.

@adam-be
Copy link
Member

adam-be commented Mar 10, 2016

This might be a follow up change..

In the description of the deviceId attribute [1] we talk about 'stored permission' and link to 'Best Practice 2: Stored Permissions'. Should that be expressed via the permissions terminology you introduce here?

[1] https://w3c.github.io/mediacapture-main/#widl-MediaDeviceInfo-deviceId

permission</dfn> and <dfn>create a PermissionStatus</dfn> are
defined in [[!permissions]].
<p class="note">"request permission" isn't defined yet.
https://github.com/w3c/permissions/issues/62
Copy link
Member

Choose a reason for hiding this comment

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

Putting it as a link rather than a plain-text URL would be more reader-friendly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@dontcallmedom
Copy link
Member

except for some minor nits and the corrections that @adam-be identified, LGTM

@alvestrand
Copy link
Contributor Author

See if this works for people. In particular pinging @martinthomson to see if text is consistent with Firefox' behavior.

@@ -2731,6 +2757,17 @@
and can thus be used as a fingerprinting surface.</p>
</li>
<li>
<p><a>Retrieve the permission state</a> for all
candidate devices that are not open in the current
browsing context. Remove from the set any device for
Copy link
Contributor

Choose a reason for hiding this comment

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

"open" seems to not be defined in mediacapture-main. Could we use something along the lines of "... for all candidate live sources not currently attached to at least one live MediaStreamTrack"?

Earlier in the document "live source" is used as a term for microphones and cameras OTOH in the introduction we use "local multimedia devices". Or we could reference MediaDevices, or define device.

Perhaps defining device is the best thing to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "life cycle and media flow" section defines the term "live" for MediaStreamTracks as "not ended". I'll use that.

@alvestrand
Copy link
Contributor Author

I'm leaving the "best practice: stored permissions" for a later change. This might involve cross-spec surgery.

alvestrand and others added 4 commits March 30, 2016 14:53
In particular, make "temporary permissions" a thing that exists
within the getusermedia spec, not in the permissions spec.
@alvestrand
Copy link
Contributor Author

PTAL.

@stefhak
Copy link
Contributor

stefhak commented Mar 30, 2016

LGTM (I made a comment, but am fine merging regardless of if we do anything about it or not)

@elagerway
Copy link
Contributor

LGTM

@adam-be
Copy link
Member

adam-be commented Mar 31, 2016

I think we should stick to "live" MediaStreamTrack, but other than that, I think this is mergable.

@alvestrand
Copy link
Contributor Author

Found 2 more "active" in an unrelated section. Changed them to "live".

@alvestrand
Copy link
Contributor Author

PTAL. Adam, if happy, merge.

@stefhak
Copy link
Contributor

stefhak commented Apr 2, 2016

LGTM - we should merge this one now IMO.

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

Successfully merging this pull request may close these issues.

None yet

7 participants