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

Upstream 'SharedWorker()' changes from Secure Contexts. #1560

Merged
merged 1 commit into from Jul 15, 2016
Merged

Upstream 'SharedWorker()' changes from Secure Contexts. #1560

merged 1 commit into from Jul 15, 2016

Conversation

mikewest
Copy link
Member

Monkey-patches bad. Upstreaming good. Closes w3c/webappsec-secure-contexts#31.

@mikewest
Copy link
Member Author

@domenic, @foolip: Mind taking a look at this? It attempts to integrate https://w3c.github.io/webappsec-secure-contexts/#monkey-patching-shared-workers with HTML; basically copy/paste, with one reordering so that I wouldn't have to redefine a term.

WDYT?

spec="SECURE-CONTEXTS"></p>

<ul class="brief">
<li><dfn data-noexport="" data-x-href="https://w3c.github.io/webappsec-secure-contexts/#settings-object">Is environment settings object a secure context?</dfn></li>
Copy link
Member

Choose a reason for hiding this comment

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

data-noexport="" is not actually needed; we made a mistake adding it to other things in this area.

@domenic
Copy link
Member

domenic commented Jul 15, 2016

Some editorial nits, but overall this seems good.

@mikewest
Copy link
Member Author

I've taken another pass; thanks for the review! If you're happy with the changes, I'll squash the patch down to one commit for you.

<li><p>Let <var>settings object</var> be the <span>relevant settings object</span> for
<var>worker global scope</var>.</p></li>

<li><p>If the result of executing the <span>Is environment settings object a secure
Copy link
Member

Choose a reason for hiding this comment

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

Either remove "the" or add "algorithm" afterward

@domenic
Copy link
Member

domenic commented Jul 15, 2016

LGTM with one nit. Did you want @foolip's sign-off too, or should I merge after you push?

@mikewest
Copy link
Member Author

Nit addressed (by removing "the"). I CC'd @foolip because he's in my time zone; I think you have enough context to decide whether this is mergable or not. I'm happy to see it just land rather than waiting until Monday. :)

@domenic domenic merged commit 45e2322 into whatwg:master Jul 15, 2016
@domenic
Copy link
Member

domenic commented Jul 15, 2016

\o/!

@bzbarsky
Copy link
Contributor

Do we want to throw, or to fire an error event like we would for an origin check failure?

//cc @annevk

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.

None yet

3 participants