[webkit-reviews] review granted: [Bug 196838] Send delayed Ad Click Attribution conversion requests to the click source : [Attachment 367360] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted John Wilander <wilander at apple.com>'s
request for review:
Bug 196838: Send delayed Ad Click Attribution conversion requests to the click
source
https://bugs.webkit.org/show_bug.cgi?id=196838

Attachment 367360: Patch

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




--- Comment #10 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 367360
  --> https://bugs.webkit.org/attachment.cgi?id=367360
Patch

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

> Source/WebCore/loader/AdClickAttribution.cpp:83
> +Optional<Seconds> AdClickAttribution::setConversion(Conversion&& conversion)

I personally find it confusing that a setter is returning a value. I also have
no idea what it returns. This should be refactored to be clearer.

> Source/WebCore/loader/AdClickAttribution.h:38
> +enum class ConversionWasSent : bool { No, Yes };

Can this be moved to the AdClickAttribution::Conversion scope to avoid
polluting the global scope? And likely renamed to "WasSent"

> Source/WebCore/loader/AdClickAttribution.h:222
> +	   ConversionWasSent wasSent = ConversionWasSent::No;

I think we prefer curly bracket initialization.

> Source/WebCore/loader/AdClickAttribution.h:245
> +    WEBCORE_EXPORT void markConversionWasSent();

I'd prefer markConversionAsSent

> Source/WebKit/NetworkProcess/AdClickAttributionManager.cpp:138
> +

We should drop this extra line.

> Source/WebKit/NetworkProcess/AdClickAttributionManager.cpp:176
> +	   return completionHandler("No stored Ad Click Attribution data.");

_s