Skip to content

LibWeb: Implement Resource Timing #3704

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

Merged
merged 5 commits into from
Mar 6, 2025

Conversation

Lubrsi
Copy link
Contributor

@Lubrsi Lubrsi commented Feb 26, 2025

See individual commits for details.
There are two fetch spec issues to file:

  • Setting fetch timing's end time to a relative timestamp is incorrect, as Resource Timing will convert it to a relative timestamp itself (like all the other timestamps)
  • A resource timing entry is not created if the end of the body is not reached and the fetch params doesn't have a consume body callback. This code produces different results in different engines and changes depending on when you read from the performance buffer:
fetch("/alwayserror").then(resp => {
    if (!resp.ok) {
        return "status_" + resp.status;
    }
    return resp.text();
}).then(() => {
    console.log(JSON.stringify(performance.getEntriesByType("resource")));
});

@Lubrsi Lubrsi requested a review from alimpfard as a code owner February 26, 2025 16:08
@tcl3
Copy link
Member

tcl3 commented Feb 26, 2025

I've not had a thorough look through the code yet, but here are some WPT results for the resource-timing tests:

Before:

Ran 129 tests finished in 80.6 seconds.
  • 7 ran as expected. 0 tests skipped.
  • 1 tests crashed unexpectedly
  • 11 tests had errors unexpectedly
  • 40 tests timed out unexpectedly
  • 105 tests had unexpected subtest results

Ran 1154 checks (1025 subtests, 129 tests)
Expected results: 115
Unexpected results: 1039                                                                                                                                                    test: 52 (1 crash, 11 error, 40 timeout)
  subtest: 987 (410 fail, 543 notrun, 34 timeout)

After:

Ran 129 tests finished in 83.1 seconds.
  • 31 ran as expected. 0 tests skipped.
  • 1 tests crashed unexpectedly
  • 9 tests had errors unexpectedly
  • 37 tests timed out unexpectedly
  • 76 tests had unexpected subtest results

Ran 793 checks (664 subtests, 129 tests)
Expected results: 374
Unexpected results: 419                                                                                                                                                     test: 47 (1 crash, 9 error, 37 timeout)
  subtest: 372 (163 fail, 184 notrun, 25 timeout)

I'm not sure why fewer subtests get executed now, I suspect http://wpt.live/resource-timing/response-status-code.html , may be timing out.

@Lubrsi Lubrsi force-pushed the resource-timing branch 2 times, most recently from 612ab2b to 733fde2 Compare February 28, 2025 13:42
@github-actions github-actions bot added the conflicts Pull request has merge conflicts that need resolution label Mar 4, 2025
Copy link

github-actions bot commented Mar 4, 2025

Your pull request has conflicts that need to be resolved before it can be reviewed and merged. Make sure to rebase your branch on top of the latest master.

Lubrsi added 5 commits March 4, 2025 17:23
This timing info will be used to create a PerformanceResourceTiming
entry.
The User Timing, Resource Timing and Navigation Timing specifications
have not been updated to account for the queue method to also append to
the underlying performance buffer and it's method of checking it's
full.

This separates the functionality into a different function, with a flag
to indicate whether to use the custom full buffer logic or not.
Marking a resource timing entry requires calling non-const methods on
the global object to append to the performance buffer.
@Lubrsi Lubrsi force-pushed the resource-timing branch from 733fde2 to 53e3dbb Compare March 4, 2025 18:55
@github-actions github-actions bot removed the conflicts Pull request has merge conflicts that need resolution label Mar 4, 2025
Copy link
Member

@ADKaster ADKaster left a comment

Choose a reason for hiding this comment

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

Couple comments, and I saw that there's a few AD-HOC and "Spec-Issue" comments that should probably be associated with an actual spec issue. Doesn't much matter to me if that actual link is set up in a follow-up though.

HighResolutionTime::DOMHighResTimeStamp convert_fetch_timestamp(HighResolutionTime::DOMHighResTimeStamp time_stamp, JS::Object const& global)
{
// 1. If ts is zero, return zero.
if (time_stamp == 0.0)
Copy link
Member

Choose a reason for hiding this comment

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

what if it's -0.0 ? do we have an is_zero helper for doubles?

@Lubrsi
Copy link
Contributor Author

Lubrsi commented Mar 5, 2025

Fetch spec issues for the issues mentioned in the PR description have been filed:
whatwg/fetch#1812
whatwg/fetch#1813

@ADKaster
Copy link
Member

ADKaster commented Mar 6, 2025

I'mma just merge this. if that 0.0 thing is an issue I'm sure we'll find out eventually.

@ADKaster ADKaster merged commit 6d1f781 into LadybirdBrowser:master Mar 6, 2025
7 checks passed
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.

3 participants