[webkit-reviews] review granted: [Bug 228104] PCM: Safari on iOS and macOS are not sending ad click attribution reports for Private Click Measurement : [Attachment 443031] Patch

[bugzilla-daemon at webkit.org]

John Wilander <wilander at apple.com> has granted Kate Cheney
<katherine_cheney at apple.com>'s request for review:
Bug 228104: PCM: Safari on iOS and macOS are not sending ad click attribution
reports for Private Click Measurement
https://bugs.webkit.org/show_bug.cgi?id=228104

Attachment 443031: Patch

https://bugs.webkit.org/attachment.cgi?id=443031&action=review




--- Comment #28 from John Wilander <wilander at apple.com> ---
Comment on attachment 443031
  --> https://bugs.webkit.org/attachment.cgi?id=443031
Patch

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

r=me with comments.

> Source/WebCore/ChangeLog:10
> +	   passing of existing conversion tests will confirm this behavior.

Please add a note that several existing tests would timeout with the removal of
m_firePendingAttributionRequestsTimer.startOneShot(m_isRunningTest ? 0_s :
seconds) if your fix wasn't in place. That makes it clear that you have tests.

> Source/WebCore/loader/PrivateClickMeasurement.cpp:150
> +static Seconds randomlyBetweenTwentyFourAndFortyEightHours(bool
isRunningTest)

Please use a boolean enum IsRunningTest to make it clear at the call sites.

>
Source/WebKit/NetworkProcess/PrivateClickMeasurement/PrivateClickMeasurementDat
abase.h:48
> +   
std::pair<std::optional<WebCore::PrivateClickMeasurement::AttributionSecondsUnt
ilSendData>, DebugInfo> attributePrivateClickMeasurement(const
WebCore::PrivateClickMeasurement::SourceSite&, const
WebCore::PrivateClickMeasurement::AttributionDestinationSite&, const
ApplicationBundleIdentifier&,
WebCore::PrivateClickMeasurement::AttributionTriggerData&&, bool
isRunningTest);

Ditto on boolean enum IsRunningTest.

>
Source/WebKit/NetworkProcess/PrivateClickMeasurement/PrivateClickMeasurementMan
ager.h:85
> +    Seconds interval() const;

Please give this a more expressive name like
randomlyBetweenFifteenAndThirtyMinutes().