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

replace "abort these steps" #3152

Closed
wants to merge 5 commits into from
Closed

replace "abort these steps" #3152

wants to merge 5 commits into from

Conversation

sam0410
Copy link
Contributor

@sam0410 sam0410 commented Oct 22, 2017

Hi, This is for issue number #2878

source Outdated
@@ -1959,7 +1959,7 @@ a.setAttribute('href', 'https://example.com/'); // change the content attribute

<ol>
<li><p>If <var>nameList</var> <span data-x="list contains">contains</span> <var>name</var>,
reject <var>p</var> with a <code>TypeError</code> and abort these steps.</p></li>
reject <var>p</var> with a <code>TypeError</code>.</p></li>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure whether I need to add "return" (same in line 1991)

Copy link
Member

Choose a reason for hiding this comment

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

I think we need to keep "abort these steps" here and that also goes for other algorithms that run "in parallel". You can't really return from those algorithms. At least not as we have currently organized things.

source Outdated
@@ -86602,7 +86595,7 @@ interface <dfn>NavigatorOnLine</dfn> {
<li><p>If <var>response</var>'s <span data-x="concept-response-type">type</span> is "<code
data-x="">error</code>", or <var>response</var>'s <span
data-x="concept-response-status">status</span> is not an <span>ok status</span>, asynchronously
complete this algorithm with null, and abort these steps.</p></li>
complete this algorithm with null.</p></li>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure whether I need to add "return" (same with line 86800)

Copy link
Member

Choose a reason for hiding this comment

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

Here too.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on. Found a couple nits.

source Outdated
@@ -1959,7 +1959,7 @@ a.setAttribute('href', 'https://example.com/'); // change the content attribute

<ol>
<li><p>If <var>nameList</var> <span data-x="list contains">contains</span> <var>name</var>,
reject <var>p</var> with a <code>TypeError</code> and abort these steps.</p></li>
reject <var>p</var> with a <code>TypeError</code>.</p></li>
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to keep "abort these steps" here and that also goes for other algorithms that run "in parallel". You can't really return from those algorithms. At least not as we have currently organized things.

source Outdated
@@ -1988,7 +1988,7 @@ a.setAttribute('href', 'https://example.com/'); // change the content attribute

<ol>
<li><p>If <var>nameList</var> <span data-x="list contains">contains</span> <var>name</var>,
reject <var>p</var> with a <code>TypeError</code> and abort these steps.</p></li>
reject <var>p</var> with a <code>TypeError</code>.</p></li>
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

source Outdated
@@ -120888,7 +120881,6 @@ INSERT INTERFACES HERE
Stuart Parmenter,
Subramanian Peruvemba,
Sudhanshu Jaiswal,
sudokus999, <!-- GitHub -->
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a mistake.

source Outdated
Hickson (Google, ian@hixie.ch). More recently, Simon Pieters (no affiliation, Opera until
2017-09-15, zcorpan@gmail.com), Anne van Kesteren (Mozilla, annevk@annevk.nl), Philip
J&auml;genstedt (Google, philip@foolip.org), and Domenic Denicola (Google, d@domenic.me), all
previously long-time contributors, have joined Ian in editing the text directly.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

source Outdated
@@ -100256,8 +100249,8 @@ dictionary <dfn>StorageEventInit</dfn> : <span>EventInit</span> {
</li>

<li><p>If the transport layer specifies a character encoding, and it is supported, return that
encoding with the <span data-x="concept-encoding-confidence">confidence</span> <i>certain</i>, and
abort these steps.</p></li>
encoding with the <span data-x="concept-encoding-confidence">confidence</span> <i>certain</i>.
Copy link
Member

Choose a reason for hiding this comment

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

You need to wrap certain to the next line. We don't want to have whitespace before a closing tag.

source Outdated
@@ -86541,7 +86534,7 @@ interface <dfn>NavigatorOnLine</dfn> {
<li><p>If <var>response</var>'s <span data-x="concept-response-type">type</span> is "<code
data-x="">error</code>", or <var>response</var>'s <span
data-x="concept-response-status">status</span> is not an <span>ok status</span>, asynchronously
complete this algorithm with null, and abort these steps.</p></li>
complete this algorithm with null.</p></li>
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to keep abort these steps here.

source Outdated
@@ -86602,7 +86595,7 @@ interface <dfn>NavigatorOnLine</dfn> {
<li><p>If <var>response</var>'s <span data-x="concept-response-type">type</span> is "<code
data-x="">error</code>", or <var>response</var>'s <span
data-x="concept-response-status">status</span> is not an <span>ok status</span>, asynchronously
complete this algorithm with null, and abort these steps.</p></li>
complete this algorithm with null.</p></li>
Copy link
Member

Choose a reason for hiding this comment

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

Here too.

source Outdated
<var>moduleMap</var>[<var>url</var>] to null, asynchronously complete this algorithm with null,
and abort these steps:</p>
<var>moduleMap</var>[<var>url</var>] to null, asynchronously complete this algorithm with null
:</p>
Copy link
Member

Choose a reason for hiding this comment

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

And here. (You also cannot break before ":".)

source Outdated
@@ -87979,7 +87972,7 @@ document.querySelector("button").addEventListener("click", bound);

<ol>
<li><p><span>Check if we can run script</span> with <var>job settings</var>. If this returns
"do not run" then abort these steps.</p></li>
"do not run", then abort these steps.</p></li>
Copy link
Member

Choose a reason for hiding this comment

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

then return*

@sam0410
Copy link
Contributor Author

sam0410 commented Oct 31, 2017

Hi @annevk , I made the changes that you suggested.

@annevk
Copy link
Member

annevk commented Nov 1, 2017

Could you rebase this to resolve the conflicts? I can then have another go at reviewing. I'm sorry that happened by the way.

@sam0410
Copy link
Contributor Author

sam0410 commented Nov 1, 2017

in pull request #3185

@sam0410 sam0410 closed this Nov 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants