[webkit-reviews] review denied: [Bug 233173] PCM: Add capability for click destination to fire triggering event without cross-site requests to the click source : [Attachment 444422] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied John Wilander
<wilander at apple.com>'s request for review:
Bug 233173: PCM: Add capability for click destination to fire triggering event
without cross-site requests to the click source
https://bugs.webkit.org/show_bug.cgi?id=233173

Attachment 444422: Patch

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




--- Comment #4 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 444422
  --> https://bugs.webkit.org/attachment.cgi?id=444422
Patch

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

> Source/WTF/wtf/URL.cpp:1195
> +    return URLParser::parseURLEncodedForm(url.query());

This decodes percent encoded characters and replaces spaces with '+'.  It would
be worth adding some percent-encoded non-ASCII characters in your tests to
verify what happens there.

> Source/WTF/wtf/URL.h:253
> +WTF_EXPORT_PRIVATE Vector<KeyValuePair<String, String>>
queryParameters(const URL&);

This should probably be Vector<KeyValuePair<String, String>>
URL::queryParameters() const

> Source/WebCore/loader/PrivateClickMeasurement.cpp:107
> +std::optional<String>
PrivateClickMeasurement::parseAttributionRequestQuery(const URL& redirectURL,
AttributionTriggerData& attributionTriggerData)

I would make this return an Expected<AttributionTriggerData, String> so you
don't need a default constructed out param.

> Source/WebCore/loader/PrivateClickMeasurement.cpp:109
> +    if (!redirectURL.hasQuery())

I would check if !parameters.size() to make it more straightforward below that
parameters.first() will never go out of bounds.

>
Source/WebKit/NetworkProcess/PrivateClickMeasurement/PrivateClickMeasurementMan
ager.cpp:251
> +    SourceSite sourceSite;

It isn't necessary to add a default constructor to SourceSite.	Could you just
change the RegistrableDomain in the lines below, then still make a SourceSite
with a RegistrableDomain?