Put timing values of DocumentTiming in a GCed object [chromium/src : main]
Guohui Deng (Gerrit)
Guohui Deng added 1 comment![]( "Open in Gerrit")
This is the first CL to address issue 40793421. I think I should do the same to "DocumentLoadTiming" because it has the same problem that it can be GCed before the timing values are collected. I will do that in a follow-up CL.
Thanks for reviewing!
Related details
- David Baron
- Noam Rosenthal
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal added 1 comment![]( "Open in Gerrit")
Thanks for working on this! Please cover this with a test (WPT) and then I'll be happy to review.
Related details
- David Baron
- Guohui Deng
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Baron (Gerrit)
David Baron added 1 comment![]( "Open in Gerrit")
As Noam said, this could use a test in WPT -- probably somewhere near the existing test `third_party/blink/web_tests/external/wpt/navigation-timing/nav2-test-document-replaced.html` that the bug was reported about.
(Also, the lines in the commit message got a little bit mixed up... but once you add a WPT test you also don't need a "Test:" line there.)
Related details
- Guohui Deng
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guohui Deng (Gerrit)
Guohui Deng added 1 comment![]( "Open in Gerrit")
Thanks guys! I added an assert to /nav2-test-document-replaced.html and I edited commit message.
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal added 2 comments![]( "Open in Gerrit")
This is a good addition, thanks!
But I want to see also a new test that only tests this specifically:
- Create an iframe
- navigate the iframe
- Wait for the navigation timing entry to appear
- Access the timing entry from the iframe's parent
- Remove the iframe
- Check that *all* the properties are non-zero
WDYT?
assert_true(pnt1.domInteractive > 0.0000001);Use assert_greater_than
Related details
- Guohui Deng
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guohui Deng (Gerrit)
Guohui Deng added 1 comment![]( "Open in Gerrit")
This is a good addition, thanks!
But I want to see also a new test that only tests this specifically:
- Create an iframe
- navigate the iframe
- Wait for the navigation timing entry to appear
- Access the timing entry from the iframe's parent
- Remove the iframe
- Check that *all* the properties are non-zero
WDYT?
Sounds good! I am working on another CL and will get back to this once I submit that one.
Related details
- Noam Rosenthal
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guohui Deng (Gerrit)
Guohui Deng added 3 comments![]( "Open in Gerrit")
Guohui DengThis is a good addition, thanks!
But I want to see also a new test that only tests this specifically:
- Create an iframe
- navigate the iframe
- Wait for the navigation timing entry to appear
- Access the timing entry from the iframe's parent
- Remove the iframe
- Check that *all* the properties are non-zero
WDYT?
Sounds good! I am working on another CL and will get back to this once I submit that one.
1) I made a new test that does what you mentioned.
2) I reverted nav2-test-document-replaced.html except for fixing a misspelling.
A new patch is uploaded. Thanks for reviewing!
assert_true(pnt1.domInteractive > 0.0000001);Guohui DengUse assert_greater_than
Used in the new test.
Related details
- Noam Rosenthal
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal voted and added 3 comments![]( "Open in Gerrit")
Votes added by Noam Rosenthal
| Code-Review | +1 |
3 comments
Really nice work! Just a couple of nits and we're done
visitor->Trace(document_timing_values_);Nit: Perhaps move this to the end of the function?
var ifr = document.createElement("iframe");ifr -> iframe.
Related details
- Guohui Deng
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guohui Deng (Gerrit)
Guohui Deng added 3 comments![]( "Open in Gerrit")
Really nice work! Just a couple of nits and we're done
Thanks for the guidance along the way!
visitor->Trace(document_timing_values_);Nit: Perhaps move this to the end of the function?
Looks like the convention is to trace the members first then Trace the super class.
https://source.chromium.org/chromium/chromium/src/+/main:v8/include/cppgc/garbage-collected.h;l=46;drc=981ce1e8f82050b33153a9c4652f2ad1cefabff1
and an example
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/frame/report.h;l=43;drc=d7ac47d32bb347baf4318f1c2c0aec00dde493b6
Would you take one more look to ensure I got it right?
var ifr = document.createElement("iframe");Guohui Dengifr -> iframe.
Done
Related details
- Noam Rosenthal
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal added 1 comment![]( "Open in Gerrit")
visitor->Trace(document_timing_values_);Guohui DengNit: Perhaps move this to the end of the function?
Looks like the convention is to trace the members first then Trace the super class.
https://source.chromium.org/chromium/chromium/src/+/main:v8/include/cppgc/garbage-collected.h;l=46;drc=981ce1e8f82050b33153a9c4652f2ad1cefabff1
and an example
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/frame/report.h;l=43;drc=d7ac47d32bb347baf4318f1c2c0aec00dde493b6
Would you take one more look to ensure I got it right?
- Guohui Deng
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Guohui Deng
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Baron (Gerrit)
David Baron voted and added 8 comments![]( "Open in Gerrit")
Votes added by David Baron
| Code-Review | +1 |
8 comments
LGTM with one C++ change and a bunch of comments on the test (mostly javascript style comments)
virtual void Trace(Visitor*) const {}You can remove `virtual` here.
var timingAttributes = [Probably `const` rather than `var`.
for (i in timingAttributes) {Use `for (const att of timingAttributes)` and then you can just use `att` instead of `timingAttributes[i]`. (This both switches from `for..in` to `for..of` and adds `const` to declare the variable.)
assert_greater_than(pnt[timingAttributes[i]], 0, description);The message for this should probly be
```
`${description} ${att}`
```
rather than just
```
description
```
var iframe = document.createElement("iframe");probably `let` or `const` rather than `var`, both here and 9 lines below.
assert_equals(iframe.contentWindow.performance.getEntriesByType("navigation")[0].name, iframe.contentWindow.location.toString(), "navigation name matches the window.location");
assert_true(iframe.contentWindow.performance.getEntriesByType("navigation")[0].name.endsWith("blank_page_green.html"), "navigation name is blank_page_green.html");
var pnt = iframe.contentWindow.performance.getEntriesByType("navigation")[0];Maybe declare `pnt` 2 lines earlier and use it in the two assertions that look at item `[0]`?
</html>Better to have a newline at end of file. This removes the "No newline at end of right file" note in the diff viewer. (Windows uses newline as a line separator; Unix uses newline as a line terminator.) (But there still shouldn't be a blank line when you look at the diff -- just no "No newline at end of right file" note.)
Related details
- Guohui Deng
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guohui Deng (Gerrit)
Guohui Deng added 8 comments![]( "Open in Gerrit")
Thanks for reviewing and the guidance! I modified the CL according to the comments.
The code-review votes are gone after my update... Looks like the CL needs approvals again. 😐
You can remove `virtual` here.
Done
Probably `const` rather than `var`.
Done
Use `for (const att of timingAttributes)` and then you can just use `att` instead of `timingAttributes[i]`. (This both switches from `for..in` to `for..of` and adds `const` to declare the variable.)
Done
assert_greater_than(pnt[timingAttributes[i]], 0, description);The message for this should probly be
```
`${description} ${att}`
```
rather than just
```
description
```
Done
probably `let` or `const` rather than `var`, both here and 9 lines below.
Done
assert_equals(iframe.contentWindow.performance.getEntriesByType("navigation")[0].name, iframe.contentWindow.location.toString(), "navigation name matches the window.location");
assert_true(iframe.contentWindow.performance.getEntriesByType("navigation")[0].name.endsWith("blank_page_green.html"), "navigation name is blank_page_green.html");
var pnt = iframe.contentWindow.performance.getEntriesByType("navigation")[0];Maybe declare `pnt` 2 lines earlier and use it in the two assertions that look at item `[0]`?
Done
Better to have a newline at end of file. This removes the "No newline at end of right file" note in the diff viewer. (Windows uses newline as a line separator; Unix uses newline as a line terminator.) (But there still shouldn't be a blank line when you look at the diff -- just no "No newline at end of right file" note.)
- David Baron
- Noam Rosenthal
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Baron (Gerrit)
David Baron voted Code-Review+1![]( "Open in Gerrit")
- Guohui Deng
- Noam Rosenthal
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Guohui Deng
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/49240.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
- Guohui Deng
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guohui Deng (Gerrit)
Guohui Deng voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Put timing values of DocumentTiming in a GCed object
So they can outlive the document, and PerformanceNavigationTiming can
collect them.
- M third_party/blink/renderer/core/dom/document_timing.cc
- M third_party/blink/renderer/core/dom/document_timing.h
- M third_party/blink/renderer/core/timing/performance_navigation_timing.cc
- M third_party/blink/renderer/core/timing/performance_navigation_timing.h
- M third_party/blink/web_tests/external/wpt/navigation-timing/nav2-test-document-replaced.html
- A third_party/blink/web_tests/external/wpt/navigation-timing/nav2-test-timing-persistent.html
Code-Review: +1 by David Baron, +1 by Noam Rosenthal
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/49240
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |