WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
196934
Add prioritization of ad click conversions and cleaning of sent ad click conversions
https://bugs.webkit.org/show_bug.cgi?id=196934
Summary
Add prioritization of ad click conversions and cleaning of sent ad click conv...
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
Details
Formatted Diff
Diff
Patch
(48.60 KB, patch)
2019-04-17 13:40 PDT
,
John Wilander
no flags
Details
Formatted Diff
Diff
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.
Top of Page
Format For Printing
XML
Clone This Bug