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): surface oopifs in audits #7795

Closed
wants to merge 1 commit into from
Closed

Conversation

paulirish
Copy link
Member

@paulirish paulirish commented Mar 29, 2019

continuing conversation from #7640 (review) -- i figure we'll just use this issue to host our discussion.

i said:

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

and @patrickhulce replied

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.


Here's the PR that would actually surface OOPIF differences in audit results.

questions

  1. @patrickhulce as of current state of master, does oopif network activity impact network quietness for _waitForFullyLoaded and tti?
  2. do we surface oopif results in audits? Personally I think their visibility should be dictated by report(third party): filter out third party urls #6351 (third party report filter) and not their OOPIF status. Open to other opinions though.

@brendankenny
Copy link
Member

does oopif network activity impact network quietness for _waitForFullyLoaded and tti?

yes, which is why the OOPIF network requests as breaking change doesn't really make sense...

do we surface oopif results in audits?

do things like Network.getResponseBody need the proper target? If so, at least some of these gatherers won't work so the OOPIF requests will be skipped (or give a warning) anyways

@patrickhulce
Copy link
Collaborator

does oopif network activity impact network quietness for _waitForFullyLoaded and tti?

Yes.

do things like Network.getResponseBody need the proper target?

Yes, I probably should have explained better, that's why #7640 skips them, they just generate errors in the console right now.

@patrickhulce
Copy link
Collaborator

which is why the OOPIF network requests as breaking change doesn't really make sense...

@brendankenny would you mind explaining more what you mean here? what breaking change? :)

@brendankenny
Copy link
Member

which is why the OOPIF network requests as breaking change doesn't really make sense...

@brendankenny would you mind explaining more what you mean here? what breaking change? :)

I just meant that if we consider adding OOPIF network requests into audits a breaking change, we've already made a breaking change with our metrics :)

So if we're still ok with that then it's not really part of the decision.

@patrickhulce
Copy link
Collaborator

I just meant that if we consider adding OOPIF network requests into audits a breaking change

Oh I didn't realize we were thinking that :) yes agreed IMO there's no difference and it's not part of the decision

@brendankenny
Copy link
Member

@paulirish close?

@brendankenny
Copy link
Member

not doing this now, at least

@brendankenny brendankenny deleted the oopifaudit branch April 11, 2019 22:28
@brendankenny brendankenny restored the oopifaudit branch April 11, 2019 22:28
@paulirish paulirish deleted the oopifaudit branch May 2, 2019 21:56
@connorjclark
Copy link
Collaborator

do we surface oopif results in audits? Personally I think their visibility should be dictated by #6351 (third party report filter) and not their OOPIF status. Open to other opinions though.

no one addressed this. seems pretty reasonable.

@patrickhulce
Copy link
Collaborator

@connorjclark I sort of did. My response:

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.

Was against myself making the argument earlier that they should be dealt with using the third-party filter :)

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