We should support ephemeral measurement with non-persistent WebCore::PrivateClickMeasurement for direct response advertising.
<rdar://problem/81778213>
Created attachment 435325 [details] Patch
Comment on attachment 435325 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=435325&action=review > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:1521 > +void WebResourceLoadStatisticsStore::attributePrivateClickMeasurement(const PrivateClickMeasurement::SourceSite& sourceSite, const PrivateClickMeasurement::AttributionDestinationSite& destinationSite, PrivateClickMeasurement::AttributionTriggerData&& attributionTriggerData, std::optional<PrivateClickMeasurement> ephemeralMeasurement, CompletionHandler<void(std::optional<WebCore::PrivateClickMeasurement::AttributionSecondsUntilSendData>)>&& completionHandler) Should probably be optional<>&& since that could otherwise trigger ref/unref > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:1530 > + postTask([this, sourceSite, destinationSite, attributionTriggerData = WTFMove(attributionTriggerData), ephemeralMeasurement, completionHandler = WTFMove(completionHandler)]() mutable { postTask probably means going to a background thread. In that case we should isolate copy ephemeralMeasurement which has some strings.
(In reply to youenn fablet from comment #3) > Comment on attachment 435325 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=435325&action=review > > > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:1521 > > +void WebResourceLoadStatisticsStore::attributePrivateClickMeasurement(const PrivateClickMeasurement::SourceSite& sourceSite, const PrivateClickMeasurement::AttributionDestinationSite& destinationSite, PrivateClickMeasurement::AttributionTriggerData&& attributionTriggerData, std::optional<PrivateClickMeasurement> ephemeralMeasurement, CompletionHandler<void(std::optional<WebCore::PrivateClickMeasurement::AttributionSecondsUntilSendData>)>&& completionHandler) > > Should probably be optional<>&& since that could otherwise trigger ref/unref > > > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:1530 > > + postTask([this, sourceSite, destinationSite, attributionTriggerData = WTFMove(attributionTriggerData), ephemeralMeasurement, completionHandler = WTFMove(completionHandler)]() mutable { > > postTask probably means going to a background thread. > In that case we should isolate copy ephemeralMeasurement which has some > strings. Thanks, Youenn! Will fix in the patch that also has the test.
Created attachment 435456 [details] Patch
Comment on attachment 435456 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=435456&action=review Needs to be rebased to fix some data races but otherwise LGTM. > Source/WebCore/loader/PrivateClickMeasurement.h:50 > +enum class PrivateClickMeasurementAttributionPersistent : bool { Yes, No }; I think we usually use { No, Yes }. > Source/WebCore/loader/PrivateClickMeasurement.h:392 > + PrivateClickMeasurementAttributionPersistent m_isPersistent; You may need to set the default value to be ::Yes, I think enums default to 0 which will be ::No if you change the ordering above. > Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:248 > + m_ephemeralMeasurement = measurement; m_ephemeralMeasurement = WTFMove(measurement) I think.
(In reply to Kate Cheney from comment #6) > Comment on attachment 435456 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=435456&action=review > > Needs to be rebased to fix some data races but otherwise LGTM. I saw that. Will fix. > > Source/WebCore/loader/PrivateClickMeasurement.h:50 > > +enum class PrivateClickMeasurementAttributionPersistent : bool { Yes, No }; > > I think we usually use { No, Yes }. Will fix. > > Source/WebCore/loader/PrivateClickMeasurement.h:392 > > + PrivateClickMeasurementAttributionPersistent m_isPersistent; > > You may need to set the default value to be ::Yes, I think enums default to > 0 which will be ::No if you change the ordering above. > > > Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:248 > > + m_ephemeralMeasurement = measurement; > > m_ephemeralMeasurement = WTFMove(measurement) I think. Good catch. Will fix. Thanks for the review, Kate!
Comment on attachment 435456 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=435456&action=review > Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:300 > + resourceLoadStatistics->attributePrivateClickMeasurement(sourceSite, destinationSite, WTFMove(attributionTriggerData), WTFMove(m_ephemeralMeasurement), [this, weakThis = makeWeakPtr(*this)] (auto attributionSecondsUntilSendData) { Please use std::exchange() instead of WTFMove() for m_ephemeralMeasurement for avoid use-after-move bugs.
Created attachment 436248 [details] Patch for landing
(In reply to Chris Dumez from comment #8) > Comment on attachment 435456 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=435456&action=review > > > Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:300 > > + resourceLoadStatistics->attributePrivateClickMeasurement(sourceSite, destinationSite, WTFMove(attributionTriggerData), WTFMove(m_ephemeralMeasurement), [this, weakThis = makeWeakPtr(*this)] (auto attributionSecondsUntilSendData) { > > Please use std::exchange() instead of WTFMove() for m_ephemeralMeasurement > for avoid use-after-move bugs. I made this change. Thanks, Chris!
Committed r281480 (240858@main): <https://commits.webkit.org/240858@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 436248 [details].