-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
core(lantern): never exclude main document from graphs #5124
Conversation
This reverts commit 16cf671.
@paulirish @brendankenny no longer WIP, it's 10/10 so far on passing extension test so lookin' good |
* @return {LH.WebInspector.NetworkRequest} | ||
*/ | ||
static findMainDocument(records) { | ||
// TODO(phulce): handle more edge cases like client redirects, or plumb through finalUrl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding finalUrl
to all calls of request* is just insane work atm, should be easier after a refactor to pass in artifacts and make that dance a little easier
if you fellas wanna take a look we can have a lot more green builds ;) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I'm kind of worried this is a future potential issue if we don't always remember to get the main resource in a graph, but it's also reasonable to want to take subgraphs that don't have the main resource in it. So I'm not sure how to make the correct way the default way without making usability awkward in a different way.
should fix the travis flake we've been seeing on extension tests
cannot find node ID 02u309rjfja0fsdjflajsfd