Skip to content
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(network-analyzer): more distrustful of chrome connection info #4828

Merged
merged 3 commits into from
Mar 21, 2018

Conversation

patrickhulce
Copy link
Collaborator

fixes https://travis-ci.org/paulirish/lighthouse/jobs/356048954#L602

huge 👏 to @paulirish for struggling through layers of docker/extension/puppeteer to get me a devtools log of this happening

@paulirish
Copy link
Member

lint & line length. ;)

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

LGTM (with some questions)

* @return {boolean}
*/
static canTrustConnectionInformation(records) {
const connectionIdWasStarted = new Map();
Copy link
Member

Choose a reason for hiding this comment

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

connectionIdWasNew or something like that, maybe? As stand more opposite to connectionReused

}

// We probably can't trust the network information if all the connection IDs were the same
if (connectionIdWasStarted.size <= 1) return false;
Copy link
Member

Choose a reason for hiding this comment

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

could this happen for a single request page? (or am I missing the nature of connections? :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

most definitely, but in that case it won't matter since our inferred result will be identical to the real data

@paulirish
Copy link
Member

can you do me a favor and nuke these:

// FIXME(phulce): fix timing failing on travis.
!item.debugString.includes('No timing information available')

@patrickhulce patrickhulce merged commit 1c23205 into master Mar 21, 2018
@patrickhulce patrickhulce deleted the fix_extension_stable branch March 21, 2018 19:38
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.

4 participants