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.
<rdar://problem/49917773>
Created attachment 367598 [details] Patch
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.
(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!
Created attachment 367672 [details] Patch
Comment on attachment 367672 [details] Patch Just letting it run on EWS to see that everything is OK before landing.
Comment on attachment 367672 [details] Patch Clearing flags on attachment: 367672 Committed r244402: <https://trac.webkit.org/changeset/244402>
All reviewed patches have been landed. Closing bug.