[webkit-reviews] review granted: [Bug 196558] Pick up Ad Click Attribution conversions in NetworkResourceLoader::willSendRedirectedRequest() : [Attachment 366891] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted John Wilander
<wilander at apple.com>'s request for review:
Bug 196558: Pick up Ad Click Attribution conversions in
NetworkResourceLoader::willSendRedirectedRequest()
https://bugs.webkit.org/show_bug.cgi?id=196558

Attachment 366891: Patch

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




--- Comment #22 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 366891
  --> https://bugs.webkit.org/attachment.cgi?id=366891
Patch

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

> Source/WebCore/loader/AdClickAttribution.cpp:53
> +	   return WTF::nullopt;

I prefer "return { };", personal taste only.

> Source/WebCore/loader/AdClickAttribution.cpp:68
> +	   return conversion;

Can it be written return Conversion {
static_cast<uint32_t>(*conversionDataUInt64, Priority { 0 } }?
Or change Conversion constructor to take a Priority optional parameter,
something like:

explicit Conversion(ConversionData data, Priority priority = 0)

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:588
> +    auto optionalAdClickConversion =
WebCore::AdClickAttribution::parseConversionRequest(redirectURL);

No need for WebCore::

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:589
> +    if (optionalAdClickConversion) {

if (auto optionalAdClickConversion =
WebCore::AdClickAttribution::parseConversionRequest(redirectURL))

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:595
> +	      
networkSession->convertAdClickAttribution(WebCore::AdClickAttribution::Source {
redirectDomain }, WebCore::AdClickAttribution::Destination { firstPartyURL },
WTFMove(*optionalAdClickConversion));

Ideally, we would do AdClickAttribution::Source { WTFMove(redirectDomain) }
Remove WebCore::

> Tools/TestWebKitAPI/Tests/WebCore/AdClickAttribution.cpp:92
> +    const URL conversionURLWithoutPriority { { },
"https://webkit.org/.well-known/ad-click-attribution/22"_s };

Can you add 63?

> Tools/TestWebKitAPI/Tests/WebCore/AdClickAttribution.cpp:102
> +    const URL conversionURLWithPriority { { },
"https://webkit.org/.well-known/ad-click-attribution/22/12"_s };

Can you add 63 for priority?

> Tools/TestWebKitAPI/Tests/WebCore/AdClickAttribution.cpp:228
> +    optionalConversion =
AdClickAttribution::parseConversionRequest(conversionURLWithTooLargeConversionD
ataAndTooLargePriority);

Can you add error cases like
"https://webkit.org/.well-known/ad-click-attribution//22/12"_s
"https://webkit.org/.well-known/ad-click-attribution/22/12/"_s 
"https://webkit.org/.well-known/ad-click-attribution/22/12?"_s