[webkit-reviews] review granted: [Bug 194325] Forward Ad Click Attribution data from HTMLAnchorElement::handleClick() to WebKit::NavigationActionData : [Attachment 361268] 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 194325: Forward Ad Click Attribution data from
HTMLAnchorElement::handleClick() to WebKit::NavigationActionData
https://bugs.webkit.org/show_bug.cgi?id=194325

Attachment 361268: Patch

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




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

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

> Source/WebCore/loader/AdClickAttribution.h:183
> +    AdClickAttribution attribution { Campaign { campaignId.value() }, Source
{ sourceRegistrableDomain.value() }, Destination {
destinationRegistrableDomain.value() } };

Should probably use WTFMove() more here, e.g. WTFMove(*compaignId).

> Source/WebCore/loader/AdClickAttribution.h:209
> +    return Conversion { data.value(), Priority { priority.value() } };

Ditto about WTFMove().

> Source/WebCore/loader/FrameLoader.cpp:407
> +    loadFrameRequest(WTFMove(frameRequest), triggeringEvent, { },
(m_frame.isMainFrame() ? WTFMove(adClickAttribution) : WTF::nullopt));

parentheses are not needed. Also, why do we need the isMainFrame() check here
since you're already doing it later in loadURL()?

> Source/WebCore/loader/FrameLoader.h:383
> +    void loadURL(FrameLoadRequest&&, const String& referrer, FrameLoadType,
Event*, RefPtr<FormState>&&, Optional<AdClickAttribution>&& = WTF::nullopt,
CompletionHandler<void()>&& = [] { });

Since everybody should provide a completion handler (AFAIK), I don't think we
should add default parameter values here.

> Source/WebCore/loader/NavigationAction.h:138
> +    Optional<AdClickAttribution> adClickAttribution() { return
m_adClickAttribution; };

should return a const Optional<AdClickAttribution>&.

> Source/WebCore/loader/NavigationAction.h:139
> +    void setAdClickAttribution(AdClickAttribution adClickAttribution) {
m_adClickAttribution = adClickAttribution; };

Couldn't this take a AdClickAttribution&& in parameter? Assuming you can
WTFMove() at the call site, which I think you can.

> Source/WebKit/Shared/NavigationActionData.h:64
> +    Optional<WebCore::AdClickAttribution> adClickAttribution;

I would do the API::Navigation object initialization on the UIProcess side in
the same patch too. It's just a few more lines and it is still merely about
plumbing the value through to the UIProcess.