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

Make navigate's resource handling more explicit #1346

Merged
merged 3 commits into from
Jun 1, 2016

Conversation

annevk
Copy link
Member

@annevk annevk commented May 30, 2016

Also remove the speculative 401 features that seem unlikely to ever
materialize. This definitely requires more work to actually talk about
responses instead of resources and more clearly tie it to the
resource variable from earlier in the navigate algorithm, but since
all those interactions are still somewhat unclear this will have to do.

Fixes #511.

annevk added 2 commits May 30, 2016 14:32
Also remove the speculative 401 features that seem unlikely to ever
materialize. This definitely requires more work to actually talk about
responses instead of resources and more clearly tie it to the
_resource_ variable from earlier in the navigate algorithm, but since
all those interactions are still somewhat unclear this will have to do.

Fixes #511.
<li><p><i>Resource handling</i>: If the resource is a network error, then <span
data-x="navigate-ua-inline">display the inline content</span> with the newly created
<code>Document</code> object's <span>origin</span> set to a new <span>opaque
origin</span>.</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.

Not xrefed properly, probably concept-opaque-origin?

Copy link
Member

Choose a reason for hiding this comment

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

This step is entirely new, right? Maybe it could be clearer by saying "display inline content indicating to the user an appropriate error"? Maybe even add a clarifying note saying something like "This is where user agents display network error pages (etc.) to the user".

ADD THIS TO THE EVENTUAL COMMIT MESSAGE:

401 is handled by Fetch.
@annevk annevk assigned domenic and unassigned annevk Jun 1, 2016
processed as if they had no challenge, e.g. rendering the entity body as if the response had
been 200 OK.</p>
<p class="note">This is where the network errors defined and propagated by the WHATWG Fetch
standard, such as DNS or TLS errors, end up being displayed to users. <ref spec=FETCH></p>
Copy link
Member

Choose a reason for hiding this comment

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

Should network error, here and above, xref to https://fetch.spec.whatwg.org/#concept-network-error ?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe this should be a follow-up issue. There are 18 occurrences of "network error" in the spec (after this change), of which probably most should be xrefed, but others occur alongside "4xx" and "5xx", and should probably be replaced with an "ok" check. I'll open a new issue.

@domenic
Copy link
Member

domenic commented Jun 1, 2016

LGTM with xref nit. Will open a separate issue on Fetch for something I'm confused on about 401 handling.

@domenic
Copy link
Member

domenic commented Jun 1, 2016

I'll actually merge this now and leave #1363 as a cleanup for later since it's a larger problem than a single missing xref.

@domenic domenic merged commit 806bc62 into master Jun 1, 2016
@domenic domenic deleted the navigate-resource-handling branch June 1, 2016 15:55
@Manishearth
Copy link

Sorry, I didn't notice this before.

It seems like we still prompt to unload the document unconditionally, though. For 204/205/content-dispositon: attachment everything unload-related shouldn't be run.

(Perhaps add a should-unload variable?)

@annevk
Copy link
Member Author

annevk commented Jun 3, 2016

@Manishearth I'm not sure I follow that completely. Are you saying we regressed the previous text or is this a separate issue we need to address?

@Manishearth
Copy link

Separate issue. I'd mentioned this in #511, "204 and 205 should require the user agent to not navigate (and not unload the current page)". The old spec didn't handle this either.

@annevk
Copy link
Member Author

annevk commented Jun 3, 2016

I opened #1382. Hope that's okay. Will try to get to it soonish.

@Manishearth
Copy link

No problem! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants