Bug 235160 – PCM: Same-site triggering events should support ephemeral measurement

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[John Wilander]

We added ephemeral measurement for direct response advertising in https://bugs.webkit.org/show_bug.cgi?id=228984.

We added support for same-site triggering events in https://bugs.webkit.org/show_bug.cgi?id=233173.

These two features should work together.

[John Wilander]

<rdar://87423294>

[John Wilander]

Created attachment 449006 [details]
[fast-cq] Patch

[Alex Christensen]

Comment on attachment 449006 [details]
[fast-cq] Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=449006&action=review

> Source/WebKit/NetworkProcess/NetworkSession.cpp:375
> +        if (ephemeralMeasurement.isNeitherSameSiteNorCrossSiteTriggeringEvent(redirectDomain, firstPartyForCookies, attributionTriggerData))

I think the code would read nicer if this were phrased positively, like this:
if (!ephemeralMeasurement.isSameSiteOrCrossSiteTriggeringEvent(...))
    return;

[John Wilander]

(In reply to Alex Christensen from comment #3)
> Comment on attachment 449006 [details]
> [fast-cq] Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=449006&action=review
> 
> > Source/WebKit/NetworkProcess/NetworkSession.cpp:375
> > +        if (ephemeralMeasurement.isNeitherSameSiteNorCrossSiteTriggeringEvent(redirectDomain, firstPartyForCookies, attributionTriggerData))
> 
> I think the code would read nicer if this were phrased positively, like this:
> if (!ephemeralMeasurement.isSameSiteOrCrossSiteTriggeringEvent(...))
>     return;

I did consider that, but opted against it because we can't say for sure with the rather shallow analysis we do in that function that it *is* a triggering event. But we can say for sure that it *isn't* a triggering event. Unfortunate for readability but I don't want to take on the risk of someone calling that function and then assuming it's a fully validated triggering event based on the boolean return value.

Makes sense?

[Alex Christensen]

sounds reasonable

[John Wilander]

(In reply to John Wilander from comment #4)
> (In reply to Alex Christensen from comment #3)
> > Comment on attachment 449006 [details]
> > [fast-cq] Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=449006&action=review
> > 
> > > Source/WebKit/NetworkProcess/NetworkSession.cpp:375
> > > +        if (ephemeralMeasurement.isNeitherSameSiteNorCrossSiteTriggeringEvent(redirectDomain, firstPartyForCookies, attributionTriggerData))
> > 
> > I think the code would read nicer if this were phrased positively, like this:
> > if (!ephemeralMeasurement.isSameSiteOrCrossSiteTriggeringEvent(...))
> >     return;
> 
> I did consider that, but opted against it because we can't say for sure with
> the rather shallow analysis we do in that function that it *is* a triggering
> event. But we can say for sure that it *isn't* a triggering event.
> Unfortunate for readability but I don't want to take on the risk of someone
> calling that function and then assuming it's a fully validated triggering
> event based on the boolean return value.
> 
> Makes sense?

I at one point had the function named isLikely… which felt vague and not really what the caller wanted.

[John Wilander]

win bot test flakiness unrelated.

[John Wilander]

The mac-AS-debug-wk2 bot is hitting an XPC or process bootstrap crash.

[John Wilander]

Comment on attachment 449006 [details]
[fast-cq] Patch

The remaining bots are taking forever to run. Thanks for the review, Alex!

[EWS]

Committed r287970 (245999@main): <https://commits.webkit.org/245999@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 449006 [details].