We should pipe through any Ad Click Attribution data to WebKit::NavigationActionData where we can keep track of it across a complete navigational load.
<rdar://problem/47840283>
Created attachment 361268 [details] Patch
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.
(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.
Created attachment 361309 [details] Patch
Fixed everything but the API::Navigation object initialization. Per Arne had an idea of how to fix the Windows issue. Waiting for EWS.
(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?
(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.
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().
Comment on attachment 361309 [details] Patch mac-debug's tree seems red and the test failures on win are unrelated.
Comment on attachment 361309 [details] Patch Clearing flags on attachment: 361309 Committed r241049: <https://trac.webkit.org/changeset/241049>
All reviewed patches have been landed. Closing bug.