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(oopifs): skip OOPIF network records in some gatherers #7640

Merged
merged 2 commits into from
Mar 29, 2019

Conversation

patrickhulce
Copy link
Collaborator

@patrickhulce patrickhulce commented Mar 22, 2019

Summary
Some cleanup now that we're in an OOPIF world. Because they are different targets the commands will fail for these records and generated a lot of log noise. Nothing is changed functionality-wise from before we enabled OOPIFs (or even current master), and I think a discussion of whether we should surface these in the report is a separate topic. For now, we skip them.

Related Issues/PRs
#6922 #7608

@@ -103,6 +103,18 @@ module.exports = class NetworkRequest {
this.fetchedViaServiceWorker = false;
/** @type {string|undefined} */
this.frameId = '';
/**
* @type {string|undefined}
* Only set for OOPIFs. This is the targetId of the protocol target from which this
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

technically according to the types this can be undefined even when there was a source, but I have never been able to see this happen with an OOPIF

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

So the changes to the 3 gatherers I don't really agree with.

But it seems like you want to flip those in a separate PR?

Once this is landed, our results are unchanged from last release, right? And perhaps we flip the others in a 5.0 commit?
Assuming so, lgtm

@patrickhulce
Copy link
Collaborator Author

So the changes to the 3 gatherers I don't really agree with.
But it seems like you want to flip those in a separate PR?

I unconvinced myself we should surface them in the regular audit results. The things flagged in OOPIFs will tend to be on another level actionability-wise. While you might have some control over the third parties you include, you almost definitely have no control over the third parties that your third parties include. Without being able to pinpoint that this ad iframe X was added by script Y and so the script is the thing you need to fix, I feel like surfacing the OOPIF data will just be noise. You could probably make this argument with a lot of things in the report, but the things discovered in OOPIFs will just overwhelmingly be in this bucket compared to unknown CDN assets and whatnot.

Add to this the complexity that we need to start talking to other targets to get this information and it just feels extra not worth it. I'm not totally against it for 5.0 if others feel differently, but IMO, it's not the best use of our efforts.

Once this is landed, our results are unchanged from last release, right?

Correct.

And perhaps we flip the others in a 5.0 commit?

Perhaps, depending on the ongoing, spirited (but perhaps short-lived if I am out numbered) debate :)

@paulirish paulirish merged commit bed02b4 into master Mar 29, 2019
@paulirish paulirish deleted the network_records_oopif branch March 29, 2019 04:00
PeeYa added a commit to PeeYa/lighthouse that referenced this pull request Apr 2, 2019
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.

2 participants