Bug 196934

Summary: Add prioritization of ad click conversions and cleaning of sent ad click conversions
Product: WebKit Reporter: John Wilander <wilander>
Component: WebKit Misc.Assignee: John Wilander <wilander>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, cdumez, commit-queue, dbates, ews-watchlist, japhet, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=196838
Attachments:
Description Flags
Patch
none
Patch none

John Wilander
Reported 2019-04-15 14:29:34 PDT
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.
Attachments
Patch (48.56 KB, patch)
2019-04-16 19:24 PDT, John Wilander 		no flags
DetailsFormatted DiffDiff
Patch (48.60 KB, patch)
2019-04-17 13:40 PDT, John Wilander 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2019-04-15 14:32:21 PDT
<rdar://problem/49917773>
John Wilander
Comment 2 2019-04-16 19:24:51 PDT
Created attachment 367598 [details] Patch
Chris Dumez
Comment 3 2019-04-17 10:18:29 PDT
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
Comment 4 2019-04-17 10:47:09 PDT
(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
Comment 5 2019-04-17 13:40:28 PDT
Created attachment 367672 [details] Patch
John Wilander
Comment 6 2019-04-17 13:41:21 PDT
Comment on attachment 367672 [details] Patch Just letting it run on EWS to see that everything is OK before landing.
WebKit Commit Bot
Comment 7 2019-04-17 14:47:10 PDT
Comment on attachment 367672 [details] Patch Clearing flags on attachment: 367672 Committed r244402: <https://trac.webkit.org/changeset/244402>
WebKit Commit Bot
Comment 8 2019-04-17 14:47:12 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.