Summary: | PCM: Same-site triggering events should support ephemeral measurement | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | John Wilander <wilander> | ||||
Component: | WebKit Misc. | Assignee: | John Wilander <wilander> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | achristensen, cdumez, ews-watchlist, japhet, webkit-bug-importer | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
John Wilander
2022-01-12 15:15:23 PST
Created attachment 449006 [details]
[fast-cq] Patch
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; (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? sounds reasonable (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. win bot test flakiness unrelated. The mac-AS-debug-wk2 bot is hitting an XPC or process bootstrap crash. Comment on attachment 449006 [details]
[fast-cq] Patch
The remaining bots are taking forever to run. Thanks for the review, Alex!
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]. |