core: resolve redirected script records #13751
Conversation
| @@ -372,11 +372,8 @@ class LegacyJavascript extends ByteEfficiencyAudit { | |||
| let transferRatio = transferRatioByUrl.get(url); | |||
| if (transferRatio !== undefined) return transferRatio; | |||
|
|
|||
| const mainDocumentRecord = NetworkAnalyzer.findOptionalMainDocument(networkRecords); | |||
There was a problem hiding this comment.
Using findOptionalMainDocument without specifying a URL will just find the first document request. This isn't necessarily the main document if there was a redirect though.
I think it makes more sense to just get the main document record using the script URL like any other script in this case. For timespan/snapshot, finalUrl isn't necessarily the main document URL either #13706.
There was a problem hiding this comment.
yup! and this would very much be a bug for inline scripts in iframes. not sure why we (Read: I) did it this way 🤔
| const networkRecord = url === artifacts.URL.finalUrl ? | ||
| mainDocumentRecord : | ||
| networkRecords.find(n => n.url === url); | ||
| const networkRecord = getRequestForScript(networkRecords, script); |
There was a problem hiding this comment.
could const url be removed?
There was a problem hiding this comment.
It's still used in multiple places, I think we can leave it.
| @@ -372,11 +372,8 @@ class LegacyJavascript extends ByteEfficiencyAudit { | |||
| let transferRatio = transferRatioByUrl.get(url); | |||
| if (transferRatio !== undefined) return transferRatio; | |||
|
|
|||
| const mainDocumentRecord = NetworkAnalyzer.findOptionalMainDocument(networkRecords); | |||
There was a problem hiding this comment.
yup! and this would very much be a bug for inline scripts in iframes. not sure why we (Read: I) did it this way 🤔
connorjclark
changed the title
adamraine
force-pushed
the
redirected-scripts
branch
from
96f0c8d
to
94e93cc
Compare
9.6.0 note: Added `fakeScript` var to places to make up for the fact that this release does not have "Scripts" refactor.
Fixes #13724