core(network-recorder): consider iframe responses finished #6078
Conversation
patrickhulce
requested review from
brendankenny and
paulirish
as code owners
This comment has been minimized.
This comment has been minimized.
| * @return {boolean} | ||
| */ | ||
| static _isFrameRootRequestAndFinished(record) { | ||
| const isFrameRootRequest = record.url === record.documentURL; |
There was a problem hiding this comment.
do we have to worry about query strings or anything else throwing this check off? (are these fields ever canonicalized through different paths, I guess I'm asking)
There was a problem hiding this comment.
From what I could tell this is always the full query string-included URL. There were lots of samples on the verge that had misc query string params.
| */ | ||
| static _isFrameRootRequestAndFinished(record) { | ||
| const isFrameRootRequest = record.url === record.documentURL; | ||
| const responseReceived = record.responseReceivedTime > 0; |
There was a problem hiding this comment.
I really hate trying to remember the difference between data-received/response-received/finished/etc (as well as where things like requestServedFromCache fall into things), but just to make sure I understand, typically responseReceived only fires once, we just usually expect a loadingFinished to also be fired?
There was a problem hiding this comment.
Correct that is my understanding as well, data received can fire lots of times, but we always expect response received + one of loadingFinished/loadingFailed to signal the end of the request.
There was a problem hiding this comment.
do we need to consider preloads or won't this affect anything? I was thinking if a preloaded iframe was requested we might get endTime of 0. (not sure if people preload iframes) or it even counts when appending with javascript
|
you'll need to sign the CLA 😢 |
|
I signed it! |
|
CLAs look good, thanks! |
|
@patrickhulce i think we can revert this now with the oopif #6922 fix? |
Summary
root requests inside frames never fire a
loadingFinisheddevtools event, we'll use the presence of a response as the criteria for finished instead.Related Issues/PRs
#6067