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.

Sign up for GitHub

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

Jump to bottom

Add response status code to PerfomanceResourceTiming #335

Merged
merged 6 commits into from
Oct 27, 2022
Merged

Add response status code to PerfomanceResourceTiming #335

merged 6 commits into from
Oct 27, 2022

Conversation

abinpaul1
Copy link
Contributor

@abinpaul1 abinpaul1 commented Jul 9, 2022 •
edited by pr-preview bot
Loading

This was referenced Jul 9, 2022
Merged
Closed
Closed
Closed
yoavweiss
yoavweiss reviewed Jul 13, 2022
View reviewed changes
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.

Looks great, other than some respec errors

@abinpaul1 abinpaul1 mentioned this pull request Jul 13, 2022
Closed
1 task
@martinthomson
Copy link
Member

I don't know if this is the place you want this feedback, but please see mozilla/standards-positions#665 (comment) for comments on the use of TAO here.

@abinpaul1 abinpaul1 force-pushed the resource-response-status branch from 4b1f190 to 5567229 Compare July 21, 2022 11:09
yoavweiss
yoavweiss reviewed Jul 26, 2022
View reviewed changes
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 here. Did you make the corresponding change in the Fetch PR as well

index.html Outdated
@@ -942,7 +948,10 @@ <h3>
{{PerformanceResourceTiming/encodedBodySize}}, and
{{PerformanceResourceTiming/decodedBodySize}}. Further, the
{{PerformanceResourceTiming/nextHopProtocol}} attribute will be set
to the empty string.
to the empty string. In addition, if the <a data-cite=
"FETCH#concept-cors-check"> CORS check</a> fails the following
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to enforce this in the processing model as well. (e.g. in the getter above)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we still need to enforce this in the getter, since whatwg/fetch#1468 (comment) notes it's already enforced when we recieve it from fetch?

Copy link
Contributor

Choose a reason for hiding this comment

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

That comment makes sense! Can you add a note indicating that info next to the getter? Thanks! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

@yoavweiss
Copy link
Contributor

@martinthomson - Thanks for the valuable feedback. @abinpaul1 modified the PR to address it.

@abinpaul1 abinpaul1 force-pushed the resource-response-status branch from 5567229 to 284faf4 Compare July 29, 2022 07:55
@chromium-wpt-export-bot chromium-wpt-export-bot mentioned this pull request Aug 30, 2022
Merged
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 30, 2022
@abinpaul1 @chromium-wpt-export-bot
Add response status code to Resource Timing
4fa5a46 
This CL introduces a responseStatus field to Performance Resource Timing object.
This field is behind a Runtime Enabled Flag.
Resource Timing PR : w3c/resource-timing#335
Fetch PR : whatwg/fetch#1468

Bug:1343293
Change-Id: I60d1b6ba00f055481ca526a490144d88ddaf881b
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 30, 2022
@abinpaul1 @chromium-wpt-export-bot
Add response status code to Resource Timing
53a0498 
This CL introduces a responseStatus field to Performance Resource
Timing object. This field is behind a Runtime Enabled Flag.

Resource Timing PR : w3c/resource-timing#335
Fetch PR : whatwg/fetch#1468

Bug:1343293
Change-Id: I60d1b6ba00f055481ca526a490144d88ddaf881b
aarongable pushed a commit to chromium/chromium that referenced this pull request Aug 30, 2022
@abinpaul1
Add response status code to Resource Timing
fe7385a 
This CL introduces a responseStatus field to Performance Resource
Timing object. This field is behind a Runtime Enabled Flag.

Resource Timing PR : w3c/resource-timing#335
Fetch PR : whatwg/fetch#1468

Bug: 1343293
Change-Id: I60d1b6ba00f055481ca526a490144d88ddaf881b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3754923
Reviewed-by: Noam Rosenthal <[email protected]>
Commit-Queue: Abin Paul <[email protected]>
Reviewed-by: Yoav Weiss <[email protected]>
Reviewed-by: Takashi Toyoshima <[email protected]>
Reviewed-by: Mike West <[email protected]>
Reviewed-by: Yutaka Hirano <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1040902}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 30, 2022
@abinpaul1 @chromium-wpt-export-bot
Add response status code to Resource Timing
8867b18 
This CL introduces a responseStatus field to Performance Resource
Timing object. This field is behind a Runtime Enabled Flag.

Resource Timing PR : w3c/resource-timing#335
Fetch PR : whatwg/fetch#1468

Bug: 1343293
Change-Id: I60d1b6ba00f055481ca526a490144d88ddaf881b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3754923
Reviewed-by: Noam Rosenthal <[email protected]>
Commit-Queue: Abin Paul <[email protected]>
Reviewed-by: Yoav Weiss <[email protected]>
Reviewed-by: Takashi Toyoshima <[email protected]>
Reviewed-by: Mike West <[email protected]>
Reviewed-by: Yutaka Hirano <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1040902}
KyleJu pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 30, 2022
@chromium-wpt-export-bot @abinpaul1
Add response status code to Resource Timing (#34828)
9e8b0db 
This CL introduces a responseStatus field to Performance Resource
Timing object. This field is behind a Runtime Enabled Flag.

Resource Timing PR : w3c/resource-timing#335
Fetch PR : whatwg/fetch#1468

Bug: 1343293
Change-Id: I60d1b6ba00f055481ca526a490144d88ddaf881b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3754923
Reviewed-by: Noam Rosenthal <[email protected]>
Commit-Queue: Abin Paul <[email protected]>
Reviewed-by: Yoav Weiss <[email protected]>
Reviewed-by: Takashi Toyoshima <[email protected]>
Reviewed-by: Mike West <[email protected]>
Reviewed-by: Yutaka Hirano <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1040902}

Co-authored-by: Abin K Paul <[email protected]>
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Sep 5, 2022
@chromium-wpt-export-bot @moz-wptsync-bot
Bug 1779354 [wpt PR 34828] - Add response status code to Resource Tim…
7112b21 
…ing, a=testonly

Automatic update from web-platform-tests
Add response status code to Resource Timing (#34828)

This CL introduces a responseStatus field to Performance Resource
Timing object. This field is behind a Runtime Enabled Flag.

Resource Timing PR : w3c/resource-timing#335
Fetch PR : whatwg/fetch#1468

Bug: 1343293
Change-Id: I60d1b6ba00f055481ca526a490144d88ddaf881b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3754923
Reviewed-by: Noam Rosenthal <[email protected]>
Commit-Queue: Abin Paul <[email protected]>
Reviewed-by: Yoav Weiss <[email protected]>
Reviewed-by: Takashi Toyoshima <[email protected]>
Reviewed-by: Mike West <[email protected]>
Reviewed-by: Yutaka Hirano <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1040902}

Co-authored-by: Abin K Paul <[email protected]>
--

wpt-commits: 9e8b0dbba979679efc9230834ea89cb3b0f06685
wpt-pr: 34828
@abinpaul1 abinpaul1 force-pushed the resource-response-status branch from cab0383 to 32ccbbf Compare September 20, 2022 13:34
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
@abinpaul1 @copybara-github
Add response status code to Resource Timing
78a65b2 
This CL introduces a responseStatus field to Performance Resource
Timing object. This field is behind a Runtime Enabled Flag.

Resource Timing PR : w3c/resource-timing#335
Fetch PR : whatwg/fetch#1468

Bug: 1343293
Change-Id: I60d1b6ba00f055481ca526a490144d88ddaf881b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3754923
Reviewed-by: Noam Rosenthal <[email protected]>
Commit-Queue: Abin Paul <[email protected]>
Reviewed-by: Yoav Weiss <[email protected]>
Reviewed-by: Takashi Toyoshima <[email protected]>
Reviewed-by: Mike West <[email protected]>
Reviewed-by: Yutaka Hirano <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1040902}
NOKEYCHECK=True
GitOrigin-RevId: fe7385a80df9cb4f7b7bc07609f3ac8228281541
annevk pushed a commit to whatwg/fetch that referenced this pull request Oct 22, 2022
@abinpaul1
Pass response status to mark resource timing
7c30987 
Resource Timing PR: w3c/resource-timing#335.

HTML PR: whatwg/html#8405.

Follow-up to provide more clarity around filtered responses (which this relies on): #1509.
yoavweiss
yoavweiss reviewed Oct 24, 2022
View reviewed changes
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
yoavweiss
yoavweiss approved these changes Oct 24, 2022
View reviewed changes
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 4e5d051 into w3c:gh-pages Oct 27, 2022
@abinpaul1 abinpaul1 deleted the resource-response-status branch November 7, 2022 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yoavweiss yoavweiss yoavweiss approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@abinpaul1 @martinthomson @yoavweiss