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 marking of navigation timing to include Service Worker Timing #6670

Merged
merged 4 commits into from
Jul 21, 2021

Conversation

noamr
Copy link
Collaborator

@noamr noamr commented May 11, 2021

Service Workers now expose a "service worker timing info" that
is associated with a request. HTML should pass it to Navigation Timing
together with the other info.

Amend the parameters passed when creating the navigation timing info entry to include the service worker timing info. Also, pass 0 for redirectCount When cross-origin redirects occurred as part of the navigation.

See also w3c/navigation-timing#143
and w3c/ServiceWorker#1575

[x ] At least two implementers are interested (and none opposed):
This is already in Chromium+Gecko implementation

  • [x ] Tests are written and already reviewed.

/browsing-the-web.html ( diff )
/infrastructure.html ( diff )


/browsing-the-web.html ( diff )
/infrastructure.html ( diff )

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Can you give a more detailed pointer to the tests? Thanks!

source Outdated
<var>navigationType</var>.</p></li>
data-x="concept-request-redirect-count">redirect count</span>, <var>navigationType</var>, and
<span data-x="navigation-params-response">response</span>'s <span
data-x="concept-response-service-worker-timing-info">service worker timing info</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.

It seems this should be next to "response's timing info" or perhaps we should just pass along response?

Also, where are the TAO checks happening for this?

Copy link
Collaborator Author

@noamr noamr May 11, 2021

Choose a reason for hiding this comment

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

I didn't want to pass along the response, to make it clear that navigation timing doesn't keep a response handle. But I don't mind going down that route to simplify.

There are no TAO checks in navigation timing (navigation is always "same origin", and IFrame TAO is handled separately)

I will post a new patch with a different order.

Copy link
Member

Choose a reason for hiding this comment

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

There are ways you can consider navigation to be same-origin, but I'm not sure it's that straightforward. If a user clicks a link to A, that redirects to B, that redirects to C, why should C be allowed to observe there were 2 redirects?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are ways you can consider navigation to be same-origin, but I'm not sure it's that straightforward. If a user clicks a link to A, that redirects to B, that redirects to C, why should C be allowed to observe there were 2 redirects?

This is a general question about Navigation Timing, but not entirely related to TAO. CC @yoavweiss

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Clarifying: we already to a cross-origin check in HTML for redirectCount. It's unrelated to TAO. We can use that same check to hide thing TAO-wise, I don't know if this is how things are in the current implementation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I may be missing something, but:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, maybe it's missing. It's been a while since I've worked on this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

@noamr
Copy link
Collaborator Author

noamr commented May 11, 2021

Can you give a more detailed pointer to the tests? Thanks!

For example:
https://github.com/web-platform-tests/wpt/blob/master/service-workers/service-worker/navigation-timing.https.html

@yoavweiss can point out if there are more relevant tests. Probably more is better.

@yoavweiss
Copy link
Contributor

@yoavweiss can point out if there are more relevant tests. Probably more is better.

Apologies for the delay :/
I think those are the relevant tests, which theoretically should cover the HTML parts (so, that the information is piped). I filed w3c/navigation-timing#148 to better test the specific timestamp, which is defined in the Service Worker spec.

@domenic
Copy link
Member

domenic commented Jun 23, 2021

What is the status on this, @annevk @yoavweiss @noamr?

noamr added 3 commits June 24, 2021 07:38
Service Workers now expose a "service worker timing info" that
is associated with a request. HTML should pass it to Navigation Timing
together with the other info.

See also w3c/navigation-timing#143
and w3c/ServiceWorker#1575
@noamr
Copy link
Collaborator Author

noamr commented Jun 24, 2021

What is the status on this, @annevk @yoavweiss @noamr?

What else is missing? From my end I feel it's complete. There could always be more tests but I'm not sure what's missing specifically. This spec change reflects current behavior.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

This looks okay now, though I haven't done a full audit. Could you suggest an updated commit message that also accounts for the redirectCount change in a new comment?

@noamr
Copy link
Collaborator Author

noamr commented Jul 20, 2021

This looks okay now, though I haven't done a full audit. Could you suggest an updated commit message that also accounts for the redirectCount change in a new comment?

Thanks! Updated the issue description (that’s where I’m supposed to suggest an updated commit message, right?)

@annevk annevk merged commit 80043ef into whatwg:main Jul 21, 2021
@annevk
Copy link
Member

annevk commented Jul 21, 2021

Usually we do it in a comment, but that works. Thanks!

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.

4 participants