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

Broken links and wrong definitions used #33

Closed
domenic opened this issue Jul 10, 2016 · 3 comments
Closed

Broken links and wrong definitions used #33

domenic opened this issue Jul 10, 2016 · 3 comments
Assignees
Milestone

Comments

@domenic
Copy link
Contributor

domenic commented Jul 10, 2016

I noticed the following in a quick review:

  • Several places use "settings object" where they should use "environment settings object". "settings object" just refers to the property of a script, whereas the actual object is an "environment settings object". Since there are no scripts involved in this document, the latter should be used. This is particularly a problem for the algorithm in 3.1.
  • Several places use "global object" when they should be using "environment settings object's global object". That is, they are talking about the specific global object belonging to a given environment settings object, instead of about the concept in general.
  • At least one place uses "relevant settings object for a global object" (a term that appears only in the outdated W3C fork of HTML) but it tries to apply it to a Document object, which is not a global object. This should instead be "relevant settings object" which is more general.
  • The browsing context use is confused. "ancestor browsing context" and "opener browsing context" apply to other browsing contexts, but the algorithm in 3.1 applies them to a document.
  • "opaque identifier" should be "opaque origin". This is especially important given the recent work to align the definition of origin with what is implemented, with regard to document.domain; using the old definition seems hazardous from a security perspective.
  • Similarly, the algorithm in 3.2 does not seem to account for the domain component of the origin. Maybe that is OK... A note reassuring us this is intentional would be a good idea.
  • The algorithm in 3.2 could link to the scheme, host, and port components of a tuple origin, and may want to also say that after step 1, the origin is guaranteed to be a tuple origin.
  • Section 2.2.2 feature detection talks about "the relevant settings object for getter's global object". "The getter's global object" is not a well-defined term. I'd suggest "the attribute getter's [[Realm]]'s settings object". But, see isSecureContext definition seems wrong in multi-global situations #36.
  • All references to "current settings object" go nowhere (they are broken links).
  • The SharedWorker monkeypatch refers to a step 7.7, but links to a document which does not have a step 7.7. Probably best to just move that monkeypatch per Upstream shared worker modifications to HTML  #31...
  • The Web IDL reference is referred to as [WebIDL-1] and uses a title of "WebIDL Level 1", but the URL goes to the current ED of Web IDL. [SecureContext] is not defined in Web IDL Level 1, so presumably this should just be changed to [WebIDL].
  • The Web Storage link goes to https://w3c.github.io/webstorage/, which is just a a redirect interstitial; it would be better to reference HTML directly. Also, I'm not sure why it's marked as a normative reference.
@mikewest
Copy link
Member

Thanks for the review and the bug reports. I'll get these taken care of!

mikewest added a commit that referenced this issue Jul 15, 2016
This patch addresses the first ~half of the comments in
#33, focusing on clarity
around the usage of 'environment settings objects' and 'global object', as well
as various and sundry ways of getting to both.

Additionally, it moves to new terminology around origins, and cleans up the
document's references a little bit. Not done, but a good first step.
@mikewest mikewest added this to the CR milestone Jul 15, 2016
@mikewest
Copy link
Member

Just about everything here is done. There's an outstanding issue against Bikeshed for the WebIDL reference, and an open PR against HTML to upstream the SharedWorker bits. I'll close out the bug once those are resolved.

Thanks again for the review, @domenic!

@mikewest mikewest self-assigned this Jul 15, 2016
@mikewest
Copy link
Member

I think this is all taken care of/upstreamed now, with the exception of the WebIDL reference.

triple-underscore added a commit to triple-underscore/triple-underscore.github.io that referenced this issue Jul 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants