Bug 196838 – Send delayed Ad Click Attribution conversion requests to the click source

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[John Wilander]

If a conversion is reported and matches an ad click, we should schedule a conversion request with appropriate data going to the click source.

[John Wilander]

<rdar://problem/47650157>

[John Wilander]

Created attachment 367271 [details]
Patch

[John Wilander]

The FIXME comments are for follow-up patches.

[Chris Dumez]

Comment on attachment 367271 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Test: http/tests/adClickAttribution/send-attribution-conversion-request.html

I think this usually goes *after* the description, not before.

> Source/WebCore/loader/AdClickAttribution.cpp:103
> +        builder.append("?conversion=");

appendLiteral

> Source/WebCore/loader/AdClickAttribution.cpp:105
> +        builder.append("&campaign=");

appendLiteral

> Source/WebCore/loader/AdClickAttribution.cpp:143
> +void AdClickAttribution::setConversionSent()

markConversionAsSent() ?

> Source/WebCore/loader/AdClickAttribution.cpp:150
> +bool AdClickAttribution::conversionSent() const

Needs a prefix, e.g. wasConversionSent()

> Source/WebCore/loader/AdClickAttribution.cpp:174
> +            builder.append((secondsUntilSend >= 24_h && secondsUntilSend <= 48_h) ? "Within 24-48 hours" : "Outside 24-48 hours");

appendLiteral

> Source/WebCore/loader/AdClickAttribution.cpp:177
> +        builder.append((m_conversion->hasBeenSent ? "true" : "false"));

appendLiteral

> Source/WebCore/loader/AdClickAttribution.h:206
> +        explicit Conversion(ConversionData data, Priority priority, bool hasBeenSent = false)

No need for explicit. Also, we do not like boolean parameters, please use a strong enum like:
enum class : bool WasSent { No, Yes };

> Source/WebCore/loader/AdClickAttribution.h:237
> +    WEBCORE_EXPORT URL url(Optional<URL> baseURLForTesting = { }) const;

const Optional<URL>&

> Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:59
> +void NetworkAdClickAttribution::setTimer(Seconds seconds)

startTimer?

> Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:61
> +    m_firePendingConversionRequestsTimer.stop();

Why stop first? I believe startOneShot() takes care of updating the nextFireTime if already scheduled. Calling stop() and then start() is actually less efficient.

> Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:65
> +        m_firePendingConversionRequestsTimer.startOneShot(seconds);

m_firePendingConversionRequestsTimer.startOneShot(m_isRunningTest ? 0_s : seconds);

> Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:117
> +    request.setHTTPMethod("POST");

"POST"_s

> Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:118
> +    request.setHTTPHeaderField(HTTPHeaderName::CacheControl, "max-age=0");

"max-age=0"_s

> Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:128
> +    loadParameters.identifier = identifier++;

++identifier? We usually consider 0 identifiers as invalid (also note that by default, 0 cannot be stored as key in hash maps).

> Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:154
> +

blank line looks weird here IMO.

> Source/WebKit/NetworkProcess/NetworkAdClickAttribution.h:63
> +    void setRequestTimerForTesting(bool value) { m_isRunningTest = value; }

setRequestTimerForTesting is not very understandable.

> Source/WebKit/NetworkProcess/NetworkAdClickAttribution.h:64
> +    void setConversionURLForTesting(const URL& testURL) { m_conversionBaseURLForTesting = testURL; }

URL&& and WTFMove(testURL)

> Source/WebKit/NetworkProcess/NetworkAdClickAttribution.h:65
>  private:

Blank line before.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2517
> +    if (auto session = networkSession(sessionID))

auto* assuming it returns a raw pointer.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2525
> +    if (auto session = networkSession(sessionID))

auto* assuming it returns a raw pointer.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2526
> +        session->setAdClickAttributionConversionURLForTesting(url);

WTFMove(url)

> Source/WebKit/NetworkProcess/NetworkProcess.h:331
> +    void setAdClickAttributionConversionURLForTesting(PAL::SessionID, const URL&, CompletionHandler<void()>&&);

URL&&

> Source/WebKit/NetworkProcess/NetworkSession.cpp:166
> +    m_adClickAttribution->setRequestTimerForTesting(value);

Do we need a second flag? Could we imply that we're running the tests if the conversionURLForTesting for set?

> Source/WebKit/NetworkProcess/NetworkSession.cpp:169
> +void NetworkSession::setAdClickAttributionConversionURLForTesting(const URL& url)

URL&&

> Source/WebKit/NetworkProcess/NetworkSession.cpp:171
> +    m_adClickAttribution->setConversionURLForTesting(url);

WTFMove(url)

> Source/WebKit/NetworkProcess/NetworkSession.h:87
> +    void setAdClickAttributionConversionURLForTesting(const URL&);

URL&&

> Source/WebKit/NetworkProcess/PingLoad.h:28
> +#include "NetworkConnectionToWebProcess.h"

Why is this needed?

> Source/WebKit/NetworkProcess/PingLoad.h:48
> +    void initialize(NetworkProcess&);

I would put this below the destructor.

> Source/WebKit/UIProcess/API/C/WKPage.cpp:2749
> +void WKPageSetAdClickAttributionConversionURLForTesting(WKPageRef page, WKStringRef urlString, WKPageSetAdClickAttributionRequestTimerForTestingFunction callback, void* callbackContext)

Why isn't this a WKURL instead of a WKStringRef urlString?

> Source/WebKit/UIProcess/API/C/WKPage.cpp:2751
> +    

Please drop this blank line.

> Source/WebKit/UIProcess/API/C/WKPagePrivate.h:192
> +typedef void (*WKPageSetAdClickAttributionRequestTimerForTestingFunction)(void* functionContext);

Copy paste error?

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1191
> +void NetworkProcessProxy::setAdClickAttributionConversionURLForTesting(PAL::SessionID sessionID, const URL& url, CompletionHandler<void()>&& completionHandler)

URL&&

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1198
> +    sendWithAsyncReply(Messages::NetworkProcess::SetAdClickAttributionConversionURLForTesting(sessionID, url), WTFMove(completionHandler));

WTFMove(url)

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.h:176
> +    void setAdClickAttributionConversionURLForTesting(PAL::SessionID, const URL&, CompletionHandler<void()>&&);

URL&&

> Source/WebKit/UIProcess/WebPageProxy.cpp:8874
> +void WebPageProxy::setAdClickAttributionConversionURLForTesting(const URL& url, CompletionHandler<void()>&& completionHandler)

URL&&

> Source/WebKit/UIProcess/WebPageProxy.cpp:8876
> +    m_process->processPool().setAdClickAttributionConversionURLForTesting(m_websiteDataStore->sessionID(), url, WTFMove(completionHandler));

WTFMove(url)

> Source/WebKit/UIProcess/WebPageProxy.h:1508
> +    void setAdClickAttributionConversionURLForTesting(const URL&, CompletionHandler<void()>&&);

URL&&

> Source/WebKit/UIProcess/WebProcessPool.h:498
> +    void setAdClickAttributionRequestTimerForTesting(PAL::SessionID, bool value, CompletionHandler<void()>&&);

Can we please omit those things from WebProcessPool? Why does the process pool need to know about ad click? The call sites can simply get the networkProcess from the pool and send IPC directly.

[youenn fablet]

Comment on attachment 367271 [details]
Patch

Some comments below.

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

> Source/WebCore/loader/AdClickAttribution.cpp:94
> +URL AdClickAttribution::url(Optional<URL> baseURLForTesting) const

I would create two urls getter, the current one and another one being: URL urlForTesting(const URL& baseURL) const;

> Source/WebCore/loader/AdClickAttribution.h:206
> +        explicit Conversion(ConversionData data, Priority priority, bool hasBeenSent = false)

No need for explicit.

> Source/WebCore/loader/AdClickAttribution.h:220
> +        bool hasBeenSent;

I would set it to false here as well.

> Source/WebCore/loader/AdClickAttribution.h:333
>  }

No need to move priority since it is an uint

> Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:59
> +void NetworkAdClickAttribution::setTimer(Seconds seconds)

The name NetworkAdClickAttribution is not great as it looks like it is one click attribution while it is a collection of attributions.
NetworkAdClickAttribution -> AdClickAttributionManager?

> Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:65
> +        m_firePendingConversionRequestsTimer.startOneShot(seconds);

m_firePendingConversionRequestsTimer.startOneShot(m_isRunningTest ? seconds : 0_s);

> Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:78
>      destinationIter->value.setConversion(WTFMove(conversion));

setConversion might exit early in case the conversion is not valid.
I would tend for setConversion to return a boolean.
Then, here we would exit early and probably remove destinationIter from sourceIter->value.

By exiting early, we would not have to check for earliestTimeToSend below.
Alternatively, setConversion could return an earliestTimeToSend directly.

> Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:86
> +    auto seconds = *earliestTimeToSend - WallTime::now();

setConversion is doing seconds + WallTime::now() and here we are doing earliestTimeToSend - WallTime::now().
Could we simplify things here and have setConversion return an Optional<Seconds>?

> Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:87
> +    if (seconds <= 0_s) {

Check probably not needed, if seconds is negative, the timer will probably convert it to 0_s.

> Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:100
> +    m_firePendingConversionRequestsTimer.stop();

Is it needed?

> Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:102
> +    if (!m_pingLoadFunction) {

Instead of doing this check, I would initialize m_pingLoadFunction to a no-op function.

> Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:140
> +    loadParameters.mainDocumentURL = conversionReferrerURL;

WTFMove(conversionReferrerURL)

> Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:155
> +            if (attribution.conversionSent()) {

When attribution is marked as conversionSent, I believe we should remove it from the map.
This should make this check unnecessary.

> Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:160
> +            auto earliestTimeToSend = attribution.earliestTimeToSend();

Instead of having one big m_adClickAttributionMap map, it might be better to have two maps:
- One for not yet converted ad click attributions
- One for converted ad click attributions that are pending being sent.
Here, we would only iterate over the second one, which would make earliestTimeToSend always available.

Maybe that would call for two structures, one with an earliestTimeToSend and one without.

> Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:175
> +                nextTimeToFire = seconds;

Shouldn't nextTimeToFire be set to max value initially and then std::min for each loop.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2523
> +void NetworkProcess::setAdClickAttributionConversionURLForTesting(PAL::SessionID sessionID, const URL& url, CompletionHandler<void()>&& completionHandler)

Use one method for both?

> Source/WebKit/NetworkProcess/NetworkSession.cpp:78
> +    m_adClickAttribution->setPingLoadFunction([this](NetworkResourceLoadParameters&& loadParameters, CompletionHandler<void(const WebCore::ResourceError&, const WebCore::ResourceResponse&)>&& completionHandler) {

We probably need to ref m_networkProcess or better makeWeakPtr it.

[Chris Dumez]

I see there is quite a bit of overlap between Youenn's comments and mine, good :)

[John Wilander]

(In reply to Chris Dumez from comment #4)
> Comment on attachment 367271 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367271&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Test: http/tests/adClickAttribution/send-attribution-conversion-request.html
> 
> I think this usually goes *after* the description, not before.

OK. I've always written the description after the template stuff, before the file enumeration.

> > Source/WebCore/loader/AdClickAttribution.cpp:103
> > +        builder.append("?conversion=");
> 
> appendLiteral

Will fix.

> > Source/WebCore/loader/AdClickAttribution.cpp:105
> > +        builder.append("&campaign=");
> 
> appendLiteral

Will fix.

> > Source/WebCore/loader/AdClickAttribution.cpp:143
> > +void AdClickAttribution::setConversionSent()
> 
> markConversionAsSent() ?

Will fix.

> > Source/WebCore/loader/AdClickAttribution.cpp:150
> > +bool AdClickAttribution::conversionSent() const
> 
> Needs a prefix, e.g. wasConversionSent()

Will fix.

> > Source/WebCore/loader/AdClickAttribution.cpp:174
> > +            builder.append((secondsUntilSend >= 24_h && secondsUntilSend <= 48_h) ? "Within 24-48 hours" : "Outside 24-48 hours");
> 
> appendLiteral

The compiler doesn't accept it for such ternary expressions.

> > Source/WebCore/loader/AdClickAttribution.cpp:177
> > +        builder.append((m_conversion->hasBeenSent ? "true" : "false"));
> 
> appendLiteral

Ditto.

> > Source/WebCore/loader/AdClickAttribution.h:206
> > +        explicit Conversion(ConversionData data, Priority priority, bool hasBeenSent = false)
> 
> No need for explicit. Also, we do not like boolean parameters, please use a
> strong enum like:
> enum class : bool WasSent { No, Yes };

Will fix.

> > Source/WebCore/loader/AdClickAttribution.h:237
> > +    WEBCORE_EXPORT URL url(Optional<URL> baseURLForTesting = { }) const;
> 
> const Optional<URL>&

I'll go with Youenn's suggestion of two URL getters instead.

> > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:59
> > +void NetworkAdClickAttribution::setTimer(Seconds seconds)
> 
> startTimer?

Will fix.

> > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:61
> > +    m_firePendingConversionRequestsTimer.stop();
> 
> Why stop first? I believe startOneShot() takes care of updating the
> nextFireTime if already scheduled. Calling stop() and then start() is
> actually less efficient.

I had a bunch of trouble with the timer firing repeatedly. I'll check that it doesn't happen without this call.

> > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:65
> > +        m_firePendingConversionRequestsTimer.startOneShot(seconds);
> 
> m_firePendingConversionRequestsTimer.startOneShot(m_isRunningTest ? 0_s :
> seconds);

Will fix.

> > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:117
> > +    request.setHTTPMethod("POST");
> 
> "POST"_s

Will fix.

> > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:118
> > +    request.setHTTPHeaderField(HTTPHeaderName::CacheControl, "max-age=0");
> 
> "max-age=0"_s

Will fix.

> > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:128
> > +    loadParameters.identifier = identifier++;
> 
> ++identifier? We usually consider 0 identifiers as invalid (also note that
> by default, 0 cannot be stored as key in hash maps).

Got it. Will fix.

> > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:154
> > +
> 
> blank line looks weird here IMO.

OK. This will be cleaner after my follow-up patch for removing entries.

> > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.h:63
> > +    void setRequestTimerForTesting(bool value) { m_isRunningTest = value; }
> 
> setRequestTimerForTesting is not very understandable.

Will fix.

> > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.h:64
> > +    void setConversionURLForTesting(const URL& testURL) { m_conversionBaseURLForTesting = testURL; }
> 
> URL&& and WTFMove(testURL)

Will fix.

> > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.h:65
> >  private:
> 
> Blank line before.

Will fix.

> > Source/WebKit/NetworkProcess/NetworkProcess.cpp:2517
> > +    if (auto session = networkSession(sessionID))
> 
> auto* assuming it returns a raw pointer.

Will fix.

> > Source/WebKit/NetworkProcess/NetworkProcess.cpp:2525
> > +    if (auto session = networkSession(sessionID))
> 
> auto* assuming it returns a raw pointer.

Will fix.

> > Source/WebKit/NetworkProcess/NetworkProcess.cpp:2526
> > +        session->setAdClickAttributionConversionURLForTesting(url);
> 
> WTFMove(url)

Will fix.

> > Source/WebKit/NetworkProcess/NetworkProcess.h:331
> > +    void setAdClickAttributionConversionURLForTesting(PAL::SessionID, const URL&, CompletionHandler<void()>&&);
> 
> URL&&

Will fix.

> > Source/WebKit/NetworkProcess/NetworkSession.cpp:166
> > +    m_adClickAttribution->setRequestTimerForTesting(value);
> 
> Do we need a second flag? Could we imply that we're running the tests if the
> conversionURLForTesting for set?

I want a way to override the timer without changing the conversion URL. It will be used in manual tests and demos where we want the real URL but don't want to wait 24-48 hours for the request to be sent.

> > Source/WebKit/NetworkProcess/NetworkSession.cpp:169
> > +void NetworkSession::setAdClickAttributionConversionURLForTesting(const URL& url)
> 
> URL&&

Will fix.

> > Source/WebKit/NetworkProcess/NetworkSession.cpp:171
> > +    m_adClickAttribution->setConversionURLForTesting(url);
> 
> WTFMove(url)

Will fix.

> > Source/WebKit/NetworkProcess/NetworkSession.h:87
> > +    void setAdClickAttributionConversionURLForTesting(const URL&);
> 
> URL&&

Will fix.

> > Source/WebKit/NetworkProcess/PingLoad.h:28
> > +#include "NetworkConnectionToWebProcess.h"
> 
> Why is this needed?

I think it was when I included PingLoad.h in WebKit::NetworkSession. That made the compiler to process WebKit::PingLoad earlier and complain about the missing dependency. But I'll do a forward declare instead.

> > Source/WebKit/NetworkProcess/PingLoad.h:48
> > +    void initialize(NetworkProcess&);
> 
> I would put this below the destructor.

Will fix.

> > Source/WebKit/UIProcess/API/C/WKPage.cpp:2749
> > +void WKPageSetAdClickAttributionConversionURLForTesting(WKPageRef page, WKStringRef urlString, WKPageSetAdClickAttributionRequestTimerForTestingFunction callback, void* callbackContext)
> 
> Why isn't this a WKURL instead of a WKStringRef urlString?

We seem to always convert them back to strings anyway, like so:
URL(URL(), toWTFString(URLRef))

Also, WKURL can only create objects from const char* strings.

But I managed to make it happen like this:
    auto wtfURLString = toWTFString(WKStringCreateWithJSString(urlString));
    WKRetainPtr<WKURLRef> messageBody(AdoptWK, WKURLCreateWithUTF8CString(wtfURLString.utf8().data()));


> > Source/WebKit/UIProcess/API/C/WKPage.cpp:2751
> > +    
> 
> Please drop this blank line.

Will fix.

> > Source/WebKit/UIProcess/API/C/WKPagePrivate.h:192
> > +typedef void (*WKPageSetAdClickAttributionRequestTimerForTestingFunction)(void* functionContext);
> 
> Copy paste error?

Yes. Thanks. Will fix.

> > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1191
> > +void NetworkProcessProxy::setAdClickAttributionConversionURLForTesting(PAL::SessionID sessionID, const URL& url, CompletionHandler<void()>&& completionHandler)
> 
> URL&&

Will fix.

> > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1198
> > +    sendWithAsyncReply(Messages::NetworkProcess::SetAdClickAttributionConversionURLForTesting(sessionID, url), WTFMove(completionHandler));
> 
> WTFMove(url)

Will fix.

> > Source/WebKit/UIProcess/Network/NetworkProcessProxy.h:176
> > +    void setAdClickAttributionConversionURLForTesting(PAL::SessionID, const URL&, CompletionHandler<void()>&&);
> 
> URL&&

Will fix.

> > Source/WebKit/UIProcess/WebPageProxy.cpp:8874
> > +void WebPageProxy::setAdClickAttributionConversionURLForTesting(const URL& url, CompletionHandler<void()>&& completionHandler)
> 
> URL&&

Will fix.

> > Source/WebKit/UIProcess/WebPageProxy.cpp:8876
> > +    m_process->processPool().setAdClickAttributionConversionURLForTesting(m_websiteDataStore->sessionID(), url, WTFMove(completionHandler));
> 
> WTFMove(url)

Will fix.

> > Source/WebKit/UIProcess/WebPageProxy.h:1508
> > +    void setAdClickAttributionConversionURLForTesting(const URL&, CompletionHandler<void()>&&);
> 
> URL&&

Will fix.

> > Source/WebKit/UIProcess/WebProcessPool.h:498
> > +    void setAdClickAttributionRequestTimerForTesting(PAL::SessionID, bool value, CompletionHandler<void()>&&);
> 
> Can we please omit those things from WebProcessPool? Why does the process
> pool need to know about ad click? The call sites can simply get the
> networkProcess from the pool and send IPC directly.

Yes, I'll removed all the Ad Click Attribution piping from WebKit::WebProcessPool and WebKit::NetworkProcessProxy.

Thanks for all the comments, Chris!

[John Wilander]

(In reply to youenn fablet from comment #5)
> Comment on attachment 367271 [details]
> Patch
> 
> Some comments below.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367271&action=review
> 
> > Source/WebCore/loader/AdClickAttribution.cpp:94
> > +URL AdClickAttribution::url(Optional<URL> baseURLForTesting) const
> 
> I would create two urls getter, the current one and another one being: URL
> urlForTesting(const URL& baseURL) const;

Good idea. Will fix.

> > Source/WebCore/loader/AdClickAttribution.h:206
> > +        explicit Conversion(ConversionData data, Priority priority, bool hasBeenSent = false)
> 
> No need for explicit.

OK. Will fix.

> > Source/WebCore/loader/AdClickAttribution.h:220
> > +        bool hasBeenSent;
> 
> I would set it to false here as well.

OK. Will fix.

> > Source/WebCore/loader/AdClickAttribution.h:333
> >  }
> 
> No need to move priority since it is an uint

Will fix.

> > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:59
> > +void NetworkAdClickAttribution::setTimer(Seconds seconds)
> 
> The name NetworkAdClickAttribution is not great as it looks like it is one
> click attribution while it is a collection of attributions.
> NetworkAdClickAttribution -> AdClickAttributionManager?

Will fix. (But this is not a new class. :)

> > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:65
> > +        m_firePendingConversionRequestsTimer.startOneShot(seconds);
> 
> m_firePendingConversionRequestsTimer.startOneShot(m_isRunningTest ? seconds
> : 0_s);

Will fix.

> > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:78
> >      destinationIter->value.setConversion(WTFMove(conversion));
> 
> setConversion might exit early in case the conversion is not valid.
> I would tend for setConversion to return a boolean.
> Then, here we would exit early and probably remove destinationIter from
> sourceIter->value.

We might return early because this conversion had lower priority than an previous one. In that case we should not remove the map entry. So instead I added a validity check first in NetworkAdClickAttribution::convert() and return early if it's false.

> By exiting early, we would not have to check for earliestTimeToSend below.
> Alternatively, setConversion could return an earliestTimeToSend directly.

Now we don't have to do this if the conversion is invalid.

> > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:86
> > +    auto seconds = *earliestTimeToSend - WallTime::now();
> 
> setConversion is doing seconds + WallTime::now() and here we are doing
> earliestTimeToSend - WallTime::now().
> Could we simplify things here and have setConversion return an
> Optional<Seconds>?

Good idea. Will fix.

> > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:87
> > +    if (seconds <= 0_s) {
> 
> Check probably not needed, if seconds is negative, the timer will probably
> convert it to 0_s.

If you say so. :)

> > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:100
> > +    m_firePendingConversionRequestsTimer.stop();
> 
> Is it needed?

I had problems earlier with repeated triggers but it seems fine to remove this.

> > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:102
> > +    if (!m_pingLoadFunction) {
> 
> Instead of doing this check, I would initialize m_pingLoadFunction to a
> no-op function.

OK. I managed to do this by calling the completion handler with empty resource objects.

> > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:140
> > +    loadParameters.mainDocumentURL = conversionReferrerURL;
> 
> WTFMove(conversionReferrerURL)

Will fix.

> > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:155
> > +            if (attribution.conversionSent()) {
> 
> When attribution is marked as conversionSent, I believe we should remove it
> from the map.
> This should make this check unnecessary.

Yes, but as indicated in one of my FIXME comments, I will do the maintenance of the map in a follow-up patch.

> > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:160
> > +            auto earliestTimeToSend = attribution.earliestTimeToSend();
> 
> Instead of having one big m_adClickAttributionMap map, it might be better to
> have two maps:
> - One for not yet converted ad click attributions
> - One for converted ad click attributions that are pending being sent.
> Here, we would only iterate over the second one, which would make
> earliestTimeToSend always available.

This sounds like a good idea. I will try to implement it in my next patch where I do the map maintenance anyway (one of the FIXMEs).

> Maybe that would call for two structures, one with an earliestTimeToSend and
> one without.
> 
> > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:175
> > +                nextTimeToFire = seconds;
> 
> Shouldn't nextTimeToFire be set to max value initially and then std::min for
> each loop.

Good idea. I'll try to use Seconds::infinity() for this.

> > Source/WebKit/NetworkProcess/NetworkProcess.cpp:2523
> > +void NetworkProcess::setAdClickAttributionConversionURLForTesting(PAL::SessionID sessionID, const URL& url, CompletionHandler<void()>&& completionHandler)
> 
> Use one method for both?

As explained above, I want the ability to override the timer while not overriding the URL for manual testing and demos.

> > Source/WebKit/NetworkProcess/NetworkSession.cpp:78
> > +    m_adClickAttribution->setPingLoadFunction([this](NetworkResourceLoadParameters&& loadParameters, CompletionHandler<void(const WebCore::ResourceError&, const WebCore::ResourceResponse&)>&& completionHandler) {
> 
> We probably need to ref m_networkProcess or better makeWeakPtr it.

OK. Will fix.

Thanks, Youenn!

[John Wilander]

Created attachment 367360 [details]
Patch

[Chris Dumez]

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

[John Wilander]

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

I agree it's a little weird. I'll make it not a setter but a more clear action.

> > 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"

I'll try to do that.

> > Source/WebCore/loader/AdClickAttribution.h:222
> > +        ConversionWasSent wasSent = ConversionWasSent::No;
> 
> I think we prefer curly bracket initialization.

OK.

> > Source/WebCore/loader/AdClickAttribution.h:245
> > +    WEBCORE_EXPORT void markConversionWasSent();
> 
> I'd prefer markConversionAsSent

I changed it to always say "was" but sure.

> > Source/WebKit/NetworkProcess/AdClickAttributionManager.cpp:138
> > +
> 
> We should drop this extra line.

Yup.

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

OK.

Thanks!

[John Wilander]

Created attachment 367440 [details]
Patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 367440 [details]:

imported/w3c/web-platform-tests/xhr/event-upload-progress.htm bug 196736 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 367440 [details]:

media/W3C/video/events/event_progress_manual.html bug 196929 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 367440 [details]
Patch for landing

Clearing flags on attachment: 367440

Committed r244288: <https://trac.webkit.org/changeset/244288>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[John Wilander]

(In reply to John Wilander from comment #8)
> (In reply to youenn fablet from comment #5)
> > Comment on attachment 367271 [details]
> > Patch
> > 
> > Instead of having one big m_adClickAttributionMap map, it might be better to
> > have two maps:
> > - One for not yet converted ad click attributions
> > - One for converted ad click attributions that are pending being sent.
> > Here, we would only iterate over the second one, which would make
> > earliestTimeToSend always available.
> 
> This sounds like a good idea. I will try to implement it in my next patch
> where I do the map maintenance anyway (one of the FIXMEs).
> 
> > Maybe that would call for two structures, one with an earliestTimeToSend and
> > one without.

This work is tracked in https://bugs.webkit.org/show_bug.cgi?id=196934.

[Truitt Savell]

It appears that the test http/tests/adClickAttribution/send-attribution-conversion-request.html

added in https://trac.webkit.org/changeset/244288/webkit

is a flakey failure. History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2FadClickAttribution%2Fsend-attribution-conversion-request.html

Diff:
--- /Volumes/Data/slave/ios-simulator-12-release-tests-wk2/build/layout-test-results/http/tests/adClickAttribution/send-attribution-conversion-request-expected.txt
+++ /Volumes/Data/slave/ios-simulator-12-release-tests-wk2/build/layout-test-results/http/tests/adClickAttribution/send-attribution-conversion-request-actual.txt
@@ -5,10 +5,7 @@
 --------
 Frame: '<!--frame1-->'
 --------
-Conversion received.
-HTTP_HOST: 127.0.0.1:8000
-REQUEST_URI: /adClickAttribution/resources/conversionReport.php?conversion=12&campaign=3
-No cookies in conversion request.
+Conversion not received - timed out.
 WebCore::AdClickAttribution 1
 Source: 127.0.0.1
 Destination: localhost
@@ -16,4 +13,4 @@
 Conversion data: 12
 Conversion priority: 0
 Conversion earliest time to send: Within 24-48 hours
-Conversion request sent: true
+Conversion request sent: false

[John Wilander]

(In reply to Truitt Savell from comment #18)
> It appears that the test
> http/tests/adClickAttribution/send-attribution-conversion-request.html
> 
> added in https://trac.webkit.org/changeset/244288/webkit
> 
> is a flakey failure. History:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=http%2Ftests%2FadClickAttribution%2Fsend-
> attribution-conversion-request.html
> 
> Diff:
> ---
> /Volumes/Data/slave/ios-simulator-12-release-tests-wk2/build/layout-test-
> results/http/tests/adClickAttribution/send-attribution-conversion-request-
> expected.txt
> +++
> /Volumes/Data/slave/ios-simulator-12-release-tests-wk2/build/layout-test-
> results/http/tests/adClickAttribution/send-attribution-conversion-request-
> actual.txt
> @@ -5,10 +5,7 @@
>  --------
>  Frame: '<!--frame1-->'
>  --------
> -Conversion received.
> -HTTP_HOST: 127.0.0.1:8000
> -REQUEST_URI:
> /adClickAttribution/resources/conversionReport.php?conversion=12&campaign=3
> -No cookies in conversion request.
> +Conversion not received - timed out.
>  WebCore::AdClickAttribution 1
>  Source: 127.0.0.1
>  Destination: localhost
> @@ -16,4 +13,4 @@
>  Conversion data: 12
>  Conversion priority: 0
>  Conversion earliest time to send: Within 24-48 hours
> -Conversion request sent: true
> +Conversion request sent: false

:( It is probably the php file write. Maybe I can introduce a nonce or something. I'll look into it.

[John Wilander]

Test gardening fix tracked in https://bugs.webkit.org/show_bug.cgi?id=196955.

[John Wilander]

Seems like we still see flakiness. I wonder if the timeout I set is too short. Will try to extend it next.

[John Wilander]

(In reply to John Wilander from comment #21)
> Seems like we still see flakiness. I wonder if the timeout I set is too
> short. Will try to extend it next.

Changed timeout tracked in https://bugs.webkit.org/show_bug.cgi?id=196970.

[John Wilander]

Further attempts at fixing the flakiness tracked in https://bugs.webkit.org/show_bug.cgi?id=196983. Note that this test is not flaky at all on my machine.

[John Wilander]

I have a change to the test infrastructure that seems to address the flakiness locally. It's bundled in a patch that soon goes up for review.

[John Wilander]

(In reply to John Wilander from comment #24)
> I have a change to the test infrastructure that seems to address the
> flakiness locally. It's bundled in a patch that soon goes up for review.

The fix mentioned above is tracked here and awaits review:
https://bugs.webkit.org/show_bug.cgi?id=196934

[John Wilander]

(In reply to John Wilander from comment #25)
> (In reply to John Wilander from comment #24)
> > I have a change to the test infrastructure that seems to address the
> > flakiness locally. It's bundled in a patch that soon goes up for review.
> 
> The fix mentioned above is tracked here and awaits review:
> https://bugs.webkit.org/show_bug.cgi?id=196934

Looks green after the above fix landed.