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

Exclude PerformanceServerTiming objects for resources that fail the "timing allow check" algorithm #36

Merged
merged 2 commits into from Oct 18, 2017

Conversation

cvazac
Copy link
Contributor

@cvazac cvazac commented Oct 6, 2017

index.html Outdated
@@ -171,14 +171,9 @@
<p>The user-agent MUST process <a>Server-Timing header field</a> communicated via a trailer field (see [[!RFC7230]] section 4.1.2) using the same algorithm.</p>

### Cross-origin Resources
Cross-origin resources (i.e. non <a data-cite="!RFC6454#section-5">same origin</a>) MUST be included as <a>PerformanceServerTiming</a> objects in the <a data-cite="!PERFORMANCE-TIMELINE-2#sec-performance-timeline">Performance Timeline</a>. If the "timing allow check" algorithm, as defined in [[RESOURCE-TIMING-2]], fails for a cross-origin resource:
Cross-origin resources (i.e. non <a data-cite="!RFC6454#section-5">same origin</a>) MUST be included as <a>PerformanceServerTiming</a> objects in the <a data-cite="!PERFORMANCE-TIMELINE-2#sec-performance-timeline">Performance Timeline</a> if the "timing allow check" algorithm, as defined in [[RESOURCE-TIMING-2]], passes. As the mere presence of a particularly named <a>PerformanceServerTiming</a> object is enough to communicate boolean information, if the "timing allow check" algorithm fails, the cross-origin resource MUST be excluded.
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest.. Let's drop this section entirely, and add the explicit TAO check steps into the processing algorithm? As a first step in the for loop, check that TAO algorithm passes for response origin and proceed if that passes, skip to next record otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we check before the loop? TAO check either passes or fails for all the entries of a single resource.

Copy link
Member

Choose a reason for hiding this comment

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

Good point, yep.

Copy link
Contributor

@yoavweiss yoavweiss left a comment

Choose a reason for hiding this comment

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

LGTM

@yoavweiss yoavweiss merged commit 7ed706e into w3c:gh-pages Oct 18, 2017
@igrigorik
Copy link
Member

Late to the party.. :)

When processing the response of the current document, set the serverTiming attribute on the newly created `PerformanceNavigationTiming` object to the return value of the server-timing header parsing algorithm

Why are we citing Nav Timing here? We should be referencing Resource Timing, from which NT inherits. Also, I don't think "newly created" step is correct.. At that point there is no response header to parse, you have to wait until we have the response (much later in processing).

@yoavweiss
Copy link
Contributor

Why are we citing Nav Timing here? We should be referencing Resource Timing, from which NT inherits.

ServerTiming applies to both the HTML document as well as subresources, so NavTiming seems appropriate.

Also, I don't think "newly created" step is correct.. At that point there is no response header to parse, you have to wait until we have the response (much later in processing).

Which "newly created" step you're referring to? (there are 2 of them :D)

@cvazac
Copy link
Contributor Author

cvazac commented Oct 18, 2017

Yes, when we fix #19, we'll have to make it so that the server-timing processing steps don't run until after the browser gets then entire response.

@igrigorik
Copy link
Member

ServerTiming applies to both the HTML document as well as subresources, so NavTiming seems appropriate.

No, because hooking into NavTiming only gives you NT, whereas using RT would give you both. As written, ST would only show up for navigations. My guess is, that's not the intent.

Which "newly created" step you're referring to? (there are 2 of them :D)

Which means we need fix things upstream..

Can we create a new bug for this?

@cvazac
Copy link
Contributor Author

cvazac commented Oct 18, 2017

We hook into NT (first paragraph) and RT (second paragraph) here.

This was referenced Oct 27, 2017
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

Successfully merging this pull request may close these issues.

None yet

3 participants