Add prioritization of ad click conversions and cleaning of sent ad click conversions

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[John Wilander]

We should clean the Map in WebKit::AdClickAttributionManager from attributions that have already been converted and sent out.

The fix should cover these comments on the patch in https://bugs.webkit.org/show_bug.cgi?id=196838:

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.

[Radar WebKit Bug Importer]

<rdar://problem/49917773>

[John Wilander]

Created attachment 367598 [details]
Patch

[Chris Dumez]

Comment on attachment 367598 [details]
Patch

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

> Source/WebCore/loader/AdClickAttribution.h:144
> +        : registrableDomain { WTFMove(domain) }

Bad indentation.

> Source/WebKit/NetworkProcess/AdClickAttributionManager.cpp:74
> +    auto didConvertAttribution = false;

bool

> Source/WebKit/NetworkProcess/AdClickAttributionManager.cpp:79
> +        if (sourceIter->value.contains(destination)) {

You're doing a double lookup here, first for contains() then for take(). You can avoid this by using find() instead.

> Source/WebKit/NetworkProcess/AdClickAttributionManager.h:74
> +    HashMap<Source, DestinationMap> m_convertedAdClickAttributionMap;

Could these be written as follows?
HashMap<std::pair<Source, Destination>, AdClickAttribution> ?

If possible, it seems like it'd be simpler to use and easier to reason about.

[John Wilander]

(In reply to Chris Dumez from comment #3)
> Comment on attachment 367598 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367598&action=review
> 
> > Source/WebCore/loader/AdClickAttribution.h:144
> > +        : registrableDomain { WTFMove(domain) }
> 
> Bad indentation.

Will fix.

> > Source/WebKit/NetworkProcess/AdClickAttributionManager.cpp:74
> > +    auto didConvertAttribution = false;
> 
> bool

Will fix.

> > Source/WebKit/NetworkProcess/AdClickAttributionManager.cpp:79
> > +        if (sourceIter->value.contains(destination)) {
> 
> You're doing a double lookup here, first for contains() then for take(). You
> can avoid this by using find() instead.

Ah. This was a left over from the when I had the problems with the copy of the ref-counted string. Will fix.

> > Source/WebKit/NetworkProcess/AdClickAttributionManager.h:74
> > +    HashMap<Source, DestinationMap> m_convertedAdClickAttributionMap;
> 
> Could these be written as follows?
> HashMap<std::pair<Source, Destination>, AdClickAttribution> ?
> 
> If possible, it seems like it'd be simpler to use and easier to reason about.

I think so. I discussed moving to a std::pair it with Joe last night. I'll take a stab at it.

Thanks!

[John Wilander]

Created attachment 367672 [details]
Patch

[John Wilander]

Comment on attachment 367672 [details]
Patch

Just letting it run on EWS to see that everything is OK before landing.

[WebKit Commit Bot]

Comment on attachment 367672 [details]
Patch

Clearing flags on attachment: 367672

Committed r244402: <https://trac.webkit.org/changeset/244402>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.