[webkit-reviews] review granted: [Bug 196934] Add prioritization of ad click conversions and cleaning of sent ad click conversions : [Attachment 367598] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted John Wilander <wilander at apple.com>'s
request for review:
Bug 196934: Add prioritization of ad click conversions and cleaning of sent ad
click conversions
https://bugs.webkit.org/show_bug.cgi?id=196934

Attachment 367598: Patch

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




--- Comment #3 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 367598
  --> https://bugs.webkit.org/attachment.cgi?id=367598
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.