Fix regression in setting up SharedWorkerGlobalScope #1858
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
annevk
reviewed
Oct 5, 2016
| <ol> | ||
| <li><p>Set <var>worker global scope</var>'s <span | ||
| data-x="concept-SharedWorkerGlobalScope-constructor-url">constructor url</span> to | ||
| <var>url</var>.</p></li> |
There was a problem hiding this comment.
This is wrong, it needs to be a string, not a URL. (Or we should change the comparison operation elsewhere.)
annevk
reviewed
Oct 5, 2016
| <var>options</var>, it must run the following steps:</p> | ||
| <code>MessagePort</code> <var>outside port</var>, a <code>WorkerOptions</code> dictionary | ||
| <var>options</var>, and an optional string <var>name</var>, it must run the following steps. | ||
| (<var>name</var> must always be provided if <var>worker</var> is a <code>SharedWorker</code>.)</p> |
There was a problem hiding this comment.
Since the parenthesis is not a requirement on implementations and seems more like a statement of fact, I'd use "is" rather than "must be" I think.
domenic
force-pushed
the
fix-swgs-name-and-url
branch
from
8445883 to
9ab2891
Compare
|
Rebased and fixed; good catches both. I switched to using a URL record throughout for consistency with other URLs in the spec. |
annevk
reviewed
Oct 6, 2016
| <var>options</var>, it must run the following steps:</p> | ||
| <code>MessagePort</code> <var>outside port</var>, a <code>WorkerOptions</code> dictionary | ||
| <var>options</var>, and an optional string <var>name</var>, it must run the following steps. | ||
| (<var>name</var> is always be provided when <var>worker</var> is a <code>SharedWorker</code>.)</p> |
There was a problem hiding this comment.
s/be// (I tried to hint at this by saying "must be", but I should have been more explicit. Sorry.)
|
LGTM with that fixed. |
cf0355d regressed setting up a SharedWorkerGlobalScope by removing the steps that set its constructor url and name. This was noticed in #1782 (comment). This also changes the constructor url from a string to a URL record, for consistency with other URLs stored on objects throughout the spec.
domenic
force-pushed
the
fix-swgs-name-and-url
branch
from
9ab2891 to
1752559
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
cf0355d regressed setting up a SharedWorkerGlobalScope by removing the steps that set its constructor url and name. This was noticed in #1782 (comment).
No tests for a spec regression fix that shouldn't have impacted implementations.