[webkit-reviews] review granted: [Bug 222217] PCM: Introduce PrivateClickMeasurementNetworkLoader : [Attachment 424388] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Jiewen Tan
<jiewen_tan at apple.com>'s request for review:
Bug 222217: PCM: Introduce PrivateClickMeasurementNetworkLoader
https://bugs.webkit.org/show_bug.cgi?id=222217

Attachment 424388: Patch

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




--- Comment #4 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 424388
  --> https://bugs.webkit.org/attachment.cgi?id=424388
Patch

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

> Source/WebKit/NetworkProcess/NetworkSession.cpp:124
>      m_privateClickMeasurement =
makeUnique<PrivateClickMeasurementManager>(*this, networkProcess,
parameters.sessionID);

Since we are creating the PCM here, let's pass the network load function as a
parameter to the constructor.

> Source/WebKit/NetworkProcess/NetworkSession.cpp:125
> +    m_privateClickMeasurement->setNetworkLoadFunction([this, weakThis =
makeWeakPtr(this)] (NetworkLoadParameters&& loadParameters,
CompletionHandler<void(const WebCore::ResourceError&, const
WebCore::ResourceResponse&, const RefPtr<JSON::Object>&)>&& completionHandler)
{

Could use auto&& loadParameters, auto&& completionHandler.

> Source/WebKit/NetworkProcess/NetworkSession.cpp:127
>	       return;

We probably need to call completionHandler if weakThis is null.
No need to capture 'this' as well.
You could replace with
PrivateClickMeasurementNetworkLoader::start(*weakThis,...

> Source/WebKit/NetworkProcess/NetworkSession.h:218
> +    HashSet<std::unique_ptr<PrivateClickMeasurementNetworkLoader>>
m_privateClickMeasurementNetworkLoaders;

I think Alex added support for map/set of UniqueRef. You could have a try here.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:155
> +    ASSERT(m_networkLoadFunction);

You could add this one in the constructor if we pass it a construction time.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:156
> +    m_networkLoadFunction(WTFMove(loadParameters), [weakThis =
makeWeakPtr(*this), this, attribution = WTFMove(attribution), callback =
WTFMove(callback)] (const WebCore::ResourceError& error, const
WebCore::ResourceResponse& response, const RefPtr<JSON::Object>& jsonObject)
mutable {

Could use more auto.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:166
> +	       m_networkProcess->broadcastConsoleMessage(weakThis->m_sessionID,
MessageSource::PrivateClickMeasurement, MessageLevel::Error,
makeString("[Private Click Measurement] JSON response is empty for token public
key request."_s));

Either stop capture 'this' and use weakThis everywhere or use this.
Do we need the call to makeString?

> Source/WebKit/NetworkProcess/PrivateClickMeasurementNetworkLoader.cpp:41
> +    auto loader = std::unique_ptr<PrivateClickMeasurementNetworkLoader>(new
PrivateClickMeasurementNetworkLoader(session, WTFMove(parameters),
WTFMove(completionHandler)));

makeUnique?

> Source/WebKit/NetworkProcess/PrivateClickMeasurementNetworkLoader.cpp:79
> +	       m_decoder = TextResourceDecoder::create("application/json"_s,
"UTF-8");

Could use a one liner maybe:
m_decoder = TextResourceDecoder::create("application/json"_s, 
m_response.textEncodingName().isEmpty() ? TextEncoding("UTF-8") :
TextEncoding(m_response.textEncodingName()));

> Source/WebKit/NetworkProcess/PrivateClickMeasurementNetworkLoader.cpp:92
> +    if (!jsonValue)

if (auto jsonValue = ...)

>
LayoutTests/http/tests/privateClickMeasurement/resources/fraudPreventionTestURL
.py:60
> +    'Content-Type: application/json\r\n\r\n'

We do not check Content-Type in PCM loader.
Should we do so? Should we test with and without it?

Do we have coverage for the case of empty application/json body