[webkit-reviews] review granted: [Bug 219134] PCM: Persist pending ad clicks and attributions so they can survive browser restart : [Attachment 414691] Patch

[bugzilla-daemon at webkit.org]

John Wilander <wilander at apple.com> has granted katherine_cheney at apple.com's
request for review:
Bug 219134: PCM: Persist pending ad clicks and attributions so they can survive
browser restart
https://bugs.webkit.org/show_bug.cgi?id=219134

Attachment 414691: Patch

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




--- Comment #10 from John Wilander <wilander at apple.com> ---
Comment on attachment 414691
  --> https://bugs.webkit.org/attachment.cgi?id=414691
Patch

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

R+ with comments. The existing tests should ensure that the logic is kept
sound. As for database expertise, I can't help you much. I'm relying on your
skill there. Just making sure - PCM is still disabled for ephemeral sessions,
right? That is the intention.

> Source/WebCore/ChangeLog:18
> +	   - convert(ed) â> attribute(d)

Seems to be a Unicode dash in these arrows. Please change to ancient ASCII. :/

> Source/WebKit/ChangeLog:21
> +	   This adds 3 SQLite tables: one for ad clicks that haven't been

I'd just call them clicks to avoid advertising language.

> Source/WebCore/html/HTMLAnchorElement.cpp:405
> +    using AttributeOnSite = PrivateClickMeasurement::AttributeOnSite;

Thank you for doing this renaming work!

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:3064
> +    clearExpiredPrivateClickMeasurement();

We may want to add a comment here to make it clear that this call is important.
We're lazily clearing before we attribute and that's how we make sure we don't
attribute expired clicks.

> Source/WebKit/NetworkProcess/NetworkSession.cpp:193
> +    m_privateClickMeasurement->startTimer(0_s);

I seem to recall some startImmediate() function. Was this how it was done in
the earlier implementation? Maybe I found that the *immediate function didn't
behave as I wanted.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:61
> +    startTimer(5_s);

What is this timer for?

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.h:43
> +enum class PrivateClickMeasurementAttributionType { Unattributed, Attributed
};

bool