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

Fix more editorial issues in processing model #44

Merged
merged 2 commits into from Jul 30, 2018

Conversation

Projects
None yet
3 participants
@npm1
Copy link
Contributor

commented Jul 24, 2018

This PR solves the remaining problems in #15.
I had to add <a href> in some places due to the direct <a> or [=../...] causing link errors (but I may have missed the correct keyword for bikeshed to create the linking in some cases).
@igrigorik @spanicker @domenic PTAL

@igrigorik
Copy link
Member

left a comment

On first pass, looks good, but I'll defer to @domenic on the algorithm updates. :)

@domenic
Copy link
Contributor

left a comment

LGTM with some linking nits, and a preexisting issue with a concept definition.

index.bs Outdated
@@ -231,57 +231,57 @@ Given a task |task|, perform the following algorithm:

2. Let |destinationRealms| be an empty set.

3. Determine the set of JavaScript Realms to which reports will be delivered:
3. Determine the set of <a href="https://tc39.github.io/ecma262/#sec-code-realms">JavaScript Realms</a> to which reports will be delivered:

This comment has been minimized.

Copy link
@domenic

domenic Jul 30, 2018

Contributor

The more "Bikeshed" way of doing this would be to add to the link block, e.g. https://github.com/whatwg/streams/blob/51227372cc84846bdcf68312724c4cac6a4b9e58/index.bs#L25. No big deal.

This comment has been minimized.

Copy link
@npm1

npm1 Jul 30, 2018

Author Contributor

Done, also removed other 'a href' in favor of urlPrefix.

index.bs Outdated

For each environment settings object |settings| in task's script evaluation environment settings object set:
For each <a>environment settings object</a> |settings| in |task|'s [=task/script evaluation environment settings=] object set:

This comment has been minimized.

Copy link
@domenic

domenic Jul 30, 2018

Contributor

Looks like a preexisting problem, but "object set" should still be part of the definition/link

This comment has been minimized.

Copy link
@npm1

npm1 Jul 30, 2018

Author Contributor

Done

2. Add |topmostBC|'s Window's relevant Realm to |destinationRealms|.
3. Let |descendantBCs| be |topmostBC|'s active document's list of descendant browsing contexts.
4. For each |descendantBC| in |descendantBCs|, add |descendantBC|'s Window's relevant Realm to |destinationRealms|.
1. Let |topmostBC| be |settings|'s <a>responsible browsing context</a>'s <a>top-level browsing context</a>.

This comment has been minimized.

Copy link
@domenic

domenic Jul 30, 2018

Contributor

FWIW I said <a>x</a> in that issue but [=x=] also works.

This comment has been minimized.

Copy link
@npm1

npm1 Jul 30, 2018

Author Contributor

Ack

index.bs Outdated
4. For each |descendantBC| in |descendantBCs|, add |descendantBC|'s Window's relevant Realm to |destinationRealms|.
1. Let |topmostBC| be |settings|'s <a>responsible browsing context</a>'s <a>top-level browsing context</a>.
2. Add |topmostBC|'s Window's <a>relevant Realm</a> to |destinationRealms|.
3. Let |descendantBCs| be |topmostBC|'s <a>active document</a>'s <a href="https://html.spec.whatwg.org/multipage/browsers.html#list-of-the-descendant-browsing-contexts">list of the descendant browsing contexts</a>.

This comment has been minimized.

Copy link
@domenic

domenic Jul 30, 2018

Contributor

You shouldn't need a manual link here; <a> by itself should work. It doesn't because HTML isn't exporting this like it should. You can file a HTML bug, and/or use the link block.

This comment has been minimized.

Copy link
@npm1

npm1 Jul 30, 2018

Author Contributor

Done

@npm1

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2018

Addressed comments, please merge if it looks good

@domenic domenic merged commit f86e56b into w3c:master Jul 30, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.