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.

Sign up for GitHub

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

Jump to bottom

Editorial: combine safety check steps into new section #251

Merged
merged 3 commits into from
May 4, 2023
Merged

Editorial: combine safety check steps into new section #251

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
056a760
Editorial: combine safety check steps into new section
marcoscaceres Apr 13, 2023
9d6ac0a
Link to test hidden_document.html
marcoscaceres Apr 13, 2023
9a61afc
Review feeback
marcoscaceres May 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 29 additions & 23 deletions index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,19 +408,9 @@ <h2>
<li>Let |document:Document| be [=this=]'s [=relevant global
object=]'s [=associated `Document`=].
</li>
<li data-tests="non-fully-active.html">If |document| is not
[=Document/fully active=], return [=a promise rejected with=] an
{{"InvalidStateError"}} {{DOMException}}.
</li>
<li data-tests="lock-sandboxed-iframe.html">If |document| has the
[=sandboxed orientation lock browsing context flag=] set, or doesn't
meet the [=pre-lock conditions=], or locking would be a security
risk, return [=a promise rejected with=] a {{"SecurityError"}}
{{DOMException}} and abort these steps.
</li>
<li>If |document|'s [=Document/visibility state=] is "hidden", return
[=a promise rejected with=] an {{"SecurityError"}} {{DOMException}}
and abort these steps.
<li>Run the [=common safety checks=] with |document|. If an
[=exception=] is [=exception/throw|thrown=], return [=a promise
rejected with=] that exception and abort these steps.
</li>
<li>If the [=user agent=] does not support locking the screen
orientation to |orientation|, return [=a promise rejected with=] a
Expand Down Expand Up @@ -452,16 +442,9 @@ <h2>
<li>Let |document:Document| be [=this=]'s [=relevant global
object=]'s [=associated `Document`=].
</li>
<li>If |document| is not [=Document/fully active=],
[=exception/throw=] an {{"InvalidStateError"}} {{DOMException}}.
</li>
<li data-tests="lock-sandboxed-iframe.html">If |document| has the
[=sandboxed orientation lock browsing context flag=] set, return
`undefined`.
</li>
<li>If |document|'s [=Document/visibility state=] is "hidden",
[=exception/throw=] a {{"SecurityError"}} {{DOMException}} and abort
these steps.
<li>Run the [=common safety checks=] with |document|. If an
[=exception=] is [=exception/throw|thrown=], re-throw that exception
and abort these steps.
</li>
<li>If screen's [=Screen/active orientation lock=] is `null`, return
`undefined`.
Expand All @@ -481,6 +464,29 @@ <h2>
going to change at all.
</p>
</section>
<section>
<h2>
Common Safety Checks
</h2>
<p>
The <dfn>common safety checks</dfn> for a {{Document}}
|document:Document| are the following steps:
</p>
Comment on lines +471 to +474
Copy link
Member

Choose a reason for hiding this comment

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

You should make these throw an exception. Web IDL will do the conversion from a thrown exception to a rejected promise. Returning an exception is very weird.

Alternatively you could return a(n) (string) enum and branch on that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, yeah, good point... will make them throw directly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, sooooo... these all throw now. And yes, the IDL layer would automatically wrap the exceptions.

However, just from the sake of clarity and consistency, I'd still like to use "Return a promise rejected with X" for .lock(). I've had instances in the past of reviewers getting confused about methods throwing even though they always return a promise.

Similarly, for .unlock(), just for clarity, I have it re-throw and abort the steps.

Copy link
Member

Choose a reason for hiding this comment

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

I guess that's okay, but I'd rather we explain the mechanism in a note so implementations with correct binding layers don't do this redundantly as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it would be way cleaner. I'd also rather just be writing:

  • If X throw {{WhateverError}}.
  • If Y throw {{OtherError}}.

And so on... It feels like WebIDL's [=exception/throw=] should include such a note. Currently, it's not super clear what should happen and every example in WebIDL is using [=return a promise rejected with=].

I'll raise an issue on WebIDL.

Copy link
Member Author

Choose a reason for hiding this comment

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

Anyone interested whatwg/webidl#1301

<ol class="algorithm">
<li data-tests="non-fully-active.html">If |document| is not
[=Document/fully active=], [=exception/throw=] an
{{"InvalidStateError"}} {{DOMException}}.
</li>
<li data-tests="lock-sandboxed-iframe.html">If |document| has the
[=sandboxed orientation lock browsing context flag=] set,
[=exception/throw=] {{"SecurityError"}} {{DOMException}}.
</li>
<li data-tests="hidden_document.html">If |document|'s
[=Document/visibility state=] is "hidden", [=exception/throw=]
{{"SecurityError"}} {{DOMException}}.
</li>
Copy link
Member

Choose a reason for hiding this comment

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

It strikes me that if you adopt whatwg/html@2db564c here (and I think you should in a follow-up) you won't get to distinguish exceptions, but that's probably okay.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that's ok. And yeah, will do that as a followup.

</ol>
</section>
<section>
<h2>
`type` attribute
Expand Down