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

Make 'Conditions to expose sensor readings' must #288

Merged
merged 1 commit into from
Sep 26, 2017

Conversation

pozdnyakov
Copy link

@pozdnyakov pozdnyakov commented Sep 26, 2017

Fixes #287


Preview | Diff

Copy link

@alexshalamov alexshalamov left a comment

Choose a reason for hiding this comment

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

Should spec open possibility to introduce new conditions for UA to implement required mitigation strategies?

@@ -648,10 +648,8 @@ environment constraints, e.g., software power consumption optimizations or the u

## Conditions to expose sensor readings ## {#concepts-can-expose-sensor-readings}

The user agent can define the list of [=conditions=] to check if it

Choose a reason for hiding this comment

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

The user agent must verify that all [=mandatory conditions=] are true to check if it
<dfn>can expose sensor readings</dfn>.

The list of <dfn>mandatory conditions</dfn> is presented below:
 - [=steps to determine the visibility state|Visibility state=] for an
   [=active document=] of [=platform sensor=]'s associated [=browsing context=]
   is "visible".
 - [=Currently focused area=] belongs to an [=active document=] of [=platform sensor=]'s associated
   [=browsing context=] or to an [=active document=] of a [=nested browsing context=] whose
   [=active document=]'s [=origin=] is the [=same origin-domain=] as the
   [=top-level browsing context=]'s [=active document=]

The user agent may extend list of [=mandatory conditions=] to implement required [=mitigation strategies=],
only if added conditions do not contradict or negate [=mandatory conditions=].

@pozdnyakov
Copy link
Author

Should spec open possibility to introduce new conditions for UA to implement required mitigation strategies?

@alexshalamov, yeah and we should also provide an entry point to the list for concrete sensor specs, I was planning to make it in a following PR , do you prefer to make it all at once?

@tobie
Copy link
Member

tobie commented Sep 26, 2017

I think the better strategy here would be to revert the offending PR as there are other issues with it, including introducing normative text in informative sections and naming a normative DFN with an informative keyword ("can") which introduces further ambiguities.

@alexshalamov
Copy link

@pozdnyakov We must do it to complete level 1 feature set, can be separate PRs.

@anssiko
Copy link
Member

anssiko commented Sep 26, 2017

@alexshalamov, that is a good suggestion. Consider adding an explicit extension point for such optional conditions extension specs can hook into similarly to e.g.:

https://w3c.github.io/manifest/#dfn-steps-for-processing-a-manifest
https://w3c.github.io/manifest/#dfn-extension-point

@anssiko
Copy link
Member

anssiko commented Sep 26, 2017

naming a normative DFN with an informative keyword ("can") which introduces further ambiguities.

Quite the contrary, I feel it would be confusing to use RFC 2119 terms in a definition.

For precedence, see e.g.:

https://html.spec.whatwg.org/#can-share-memory-with
https://html.spec.whatwg.org/#check-if-we-can-run-script

@tobie
Copy link
Member

tobie commented Sep 26, 2017

Quite the contrary, I feel it would be confusing to use RFC 2119 terms in a definition.

I wasn't suggesting that either (i.e. the "perform a security check" name was clearer).

I'm just saying that the offending PR (#280) introduces numerous issues and ambiguities which were absent from the spec prior to it, and I'm not quite sure what problem it's trying to solve. This new PR doesn't solve all of the problems introduced in #280, hence my suggestion is simply to roll back the previous PR, and move from there.

@anssiko
Copy link
Member

anssiko commented Sep 26, 2017

introducing normative text in informative sections

To address this comment from @tobie, I suggest you either rename:

To mitigate this threat, the user agent should check if it [=can expose sensor readings=].

Into:

To mitigate this threat, the user agent is expected to check if it [=can expose sensor readings=].

Or remove that line.

@tobie tobie removed their request for review September 26, 2017 11:33
Copy link
Member

@anssiko anssiko left a comment

Choose a reason for hiding this comment

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

These changes look good to me, and seem to address the comments raised. Thanks for the fast turnaround!

Copy link

@alexshalamov alexshalamov left a comment

Choose a reason for hiding this comment

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

looks better now, thanks!

@anssiko
Copy link
Member

anssiko commented Sep 26, 2017

Thanks for the review folks!

@anssiko anssiko merged commit 705d384 into w3c:master Sep 26, 2017
@pozdnyakov pozdnyakov deleted the fix-287 branch November 29, 2017 07:26
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.

What's the rationale of moving security checks outside of normative requirements?
5 participants