Bug 194325 – Forward Ad Click Attribution data from HTMLAnchorElement::handleClick() to WebKit::NavigationActionData

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[John Wilander]

We should pipe through any Ad Click Attribution data to WebKit::NavigationActionData where we can keep track of it across a complete navigational load.

[Radar WebKit Bug Importer]

<rdar://problem/47840283>

[John Wilander]

Created attachment 361268 [details]
Patch

[Chris Dumez]

Comment on attachment 361268 [details]
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.

[John Wilander]

(In reply to Chris Dumez from comment #3)
> Comment on attachment 361268 [details]
> 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.

Thanks, Chris! I'll have a look at all of these comments. And also figure out what Windows is complaining about.

[John Wilander]

Created attachment 361309 [details]
Patch

[John Wilander]

Fixed everything but the API::Navigation object initialization. Per Arne had an idea of how to fix the Windows issue. Waiting for EWS.

[Chris Dumez]

(In reply to John Wilander from comment #6)
> Fixed everything but the API::Navigation object initialization. Per Arne had
> an idea of how to fix the Windows issue. Waiting for EWS.

Why not?

[John Wilander]

(In reply to Chris Dumez from comment #7)
> (In reply to John Wilander from comment #6)
> > Fixed everything but the API::Navigation object initialization. Per Arne had
> > an idea of how to fix the Windows issue. Waiting for EWS.
> 
> Why not?

I wanted to discuss that part with you but also see if I had a fix for the Windows issue.

[John Wilander]

I discussed with Chris and we agree next steps fit well in a follow-up patch. Probably a getter on API::Navigation similar to currentRequestIsRedirect().

[John Wilander]

Comment on attachment 361309 [details]
Patch

mac-debug's tree seems red and the test failures on win are unrelated.

[WebKit Commit Bot]

Comment on attachment 361309 [details]
Patch

Clearing flags on attachment: 361309

Committed r241049: <https://trac.webkit.org/changeset/241049>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.