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

Update 'destination' for navigation requests. #4976

Merged
merged 1 commit into from
Jun 5, 2020
Merged

Conversation

mikewest
Copy link
Member

@mikewest mikewest commented Oct 7, 2019

Currently, top-level and nested navigations set a request destination of
"document" when performing the "process a navigate fetch" algorithm.
This patch follows whatwg/fetch#948, splitting "document" into "frame"
and "iframe" for nested navigation requests.

This patch aims to support the Fetch Metadata discussion in w3c/webappsec-fetch-metadata#45, and the Fetch patch in whatwg/fetch#948.


/browsing-the-web.html ( diff )
/iframe-embed-object.html ( diff )

source Outdated
<li><p>If <var>browsingContext</var>'s <span>browsing context container</span> is an
<code>iframe</code> element, set <var>request</var>'s <span
data-x="concept-request-destination">destination</span> to "<code
data-x="">iframe</code>".</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.

What would go wrong if we set it to browsing context container's local name?

Copy link
Member Author

Choose a reason for hiding this comment

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

That might be an elegant way of replacing all this text with something simpler. Hrm. Assuming portal does the same thing (which is a reasonable thing to expect), it seems pretty future-proof.

Let me think about it for a minute, but I don't see any obvious reason not to do it this way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Poked at this in fa00f9a. Thank you for the suggestion.

@mikewest mikewest changed the title Change nested navigations' fetches destinations. Update 'destination' for navigation requests. Nov 25, 2019
@mikewest
Copy link
Member Author

Updated this to include minimal <embed> and <object> changes.

@mikewest
Copy link
Member Author

Pinging this as well, as it should hopefully land alongside whatwg/fetch#948.

@annevk
Copy link
Member

annevk commented Feb 11, 2020

It does seem like this needs rebasing first though.

Currently, top-level and nested navigations set a request destination of
"document" when performing the "process a navigate fetch" algorithm.
This patch follows whatwg/fetch#948, splitting "document" into "frame"
and "iframe" for nested navigation requests.
@mikewest
Copy link
Member Author

It does seem like this needs rebasing first though.

Rebased and force-pushed in 50ce162. WDYT?

@annevk annevk added the needs tests Moving the issue forward requires someone to write tests label Feb 14, 2020
@annevk
Copy link
Member

annevk commented Feb 14, 2020

Although I suppose this will reuse the tests from whatwg/fetch#948, right?

@mikewest
Copy link
Member Author

Although I suppose this will reuse the tests from whatwg/fetch#948, right?

I think that's correct, yes.

@annevk annevk added do not merge yet Pull request must not be merged per rationale in comment and removed needs tests Moving the issue forward requires someone to write tests labels Feb 14, 2020
annevk pushed a commit to whatwg/fetch that referenced this pull request Jun 5, 2020
As discussed in w3c/webappsec-fetch-metadata#45, this splits the "document" destination into "document", "frame", and "iframe". These destinations distinguish top-level navigation from nested navigation, and exposing this data via `Sec-Fetch-Dest` will allow developers to better understand the nature of a request.

This patch also redefines "navigation request" and "non-subresource request" to include "embed" and "object" destinations as discussed at #948 (comment). That discussion also resulted in other changes:

* whatwg/html#4976
* w3c/ServiceWorker#1486

Tests:

* https://github.com/web-platform-tests/wpt/blob/master/fetch/metadata/embed.tentative.https.sub.html
* https://github.com/web-platform-tests/wpt/blob/master/fetch/metadata/object.tentative.https.sub.html
* https://github.com/web-platform-tests/wpt/blob/master/fetch/metadata/iframe.tentative.https.sub.html
* https://github.com/web-platform-tests/wpt/blob/master/fetch/metadata/navigation.tentative.https.sub.html
* https://github.com/web-platform-tests/wpt/blob/master/service-workers/service-worker/embed-and-object-are-not-intercepted.https.html
@annevk annevk removed the do not merge yet Pull request must not be merged per rationale in comment label Jun 5, 2020
@annevk annevk merged commit db2ce7e into master Jun 5, 2020
@annevk annevk deleted the framed-destination branch June 5, 2020 09:03
yutakahirano pushed a commit to whatwg/fetch that referenced this pull request Jun 23, 2020
As discussed in w3c/webappsec-fetch-metadata#45, this splits the "document" destination into "document", "frame", and "iframe". These destinations distinguish top-level navigation from nested navigation, and exposing this data via `Sec-Fetch-Dest` will allow developers to better understand the nature of a request.

This patch also redefines "navigation request" and "non-subresource request" to include "embed" and "object" destinations as discussed at #948 (comment). That discussion also resulted in other changes:

* whatwg/html#4976
* w3c/ServiceWorker#1486

Tests:

* https://github.com/web-platform-tests/wpt/blob/master/fetch/metadata/embed.tentative.https.sub.html
* https://github.com/web-platform-tests/wpt/blob/master/fetch/metadata/object.tentative.https.sub.html
* https://github.com/web-platform-tests/wpt/blob/master/fetch/metadata/iframe.tentative.https.sub.html
* https://github.com/web-platform-tests/wpt/blob/master/fetch/metadata/navigation.tentative.https.sub.html
* https://github.com/web-platform-tests/wpt/blob/master/service-workers/service-worker/embed-and-object-are-not-intercepted.https.html
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.

2 participants