Figure out a new way to do the network pipeline such that contents of the response can be processed.
<rdar://problem/74817863>
Created attachment 424386 [details] Patch
Created attachment 424388 [details] Patch
Comment on attachment 424388 [details] 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
Comment on attachment 424388 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424388&action=review Thanks Youenn for r+ this patch. >> 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. Sounds good to me. >> 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. It sounds like black magic. Fixed. >> 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,... Fixed. >> 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. I'm not sure though. >> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:155 >> + ASSERT(m_networkLoadFunction); > > You could add this one in the constructor if we pass it a construction time. Fixed. >> 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. Fixed. >> 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? Fixed. >> Source/WebKit/NetworkProcess/PrivateClickMeasurementNetworkLoader.cpp:41 >> + auto loader = std::unique_ptr<PrivateClickMeasurementNetworkLoader>(new PrivateClickMeasurementNetworkLoader(session, WTFMove(parameters), WTFMove(completionHandler))); > > makeUnique? No, we couldn't. The constructor is private. >> 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())); Fixed. >> Source/WebKit/NetworkProcess/PrivateClickMeasurementNetworkLoader.cpp:92 >> + if (!jsonValue) > > if (auto jsonValue = ...) Fixed. >> 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 Added the check, and the test cases.
Comment on attachment 424388 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424388&action=review Looks good over all. Please see my comments. > Source/WebKit/ChangeLog:9 > + This patch refactors PrivateClickMeasurementManager to use the newly introduced PrivateClickMeasurementNetworkLoader I would say "... to use a newly introduced ..." since this patch adds that class. > Source/WebKit/ChangeLog:10 > + instead of the PingLoad to handle network traffics such that PCMM can then extract contents of the response. "... such that PCMM can receive and process response bodies." > Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:-125 > - loadParameters.identifier = ++identifier; Do we know why NetworkResourceLoadParameters needs an identifier but NetworkLoadParameters doesn't? > Source/WebKit/NetworkProcess/PrivateClickMeasurementNetworkLoader.cpp:64 > + fail(ResourceError { ResourceError::Type::Cancellation }); Interesting solution. >>> 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())); > > Fixed. You should use WebCore::HTTPHeaderValues::applicationJSONContentType() and there you'll see this comment: // The default encoding is UTF-8: https://www.ietf.org/rfc/rfc4627.txt. ... which means you don't need to set UTF-8.
Comment on attachment 424388 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424388&action=review Thanks John for reviewing the patch. >> Source/WebKit/ChangeLog:9 >> + This patch refactors PrivateClickMeasurementManager to use the newly introduced PrivateClickMeasurementNetworkLoader > > I would say "... to use a newly introduced ..." since this patch adds that class. Fixed. >> Source/WebKit/ChangeLog:10 >> + instead of the PingLoad to handle network traffics such that PCMM can then extract contents of the response. > > "... such that PCMM can receive and process response bodies." Fixed. >> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:-125 >> - loadParameters.identifier = ++identifier; > > Do we know why NetworkResourceLoadParameters needs an identifier but NetworkLoadParameters doesn't? A good question for Youenn. >>>> 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())); >> >> Fixed. > > You should use WebCore::HTTPHeaderValues::applicationJSONContentType() and there you'll see this comment: > // The default encoding is UTF-8: https://www.ietf.org/rfc/rfc4627.txt. > ... which means you don't need to set UTF-8. Sure.
I filed two follow-up bugs and linked them here.
Created attachment 424721 [details] Patch for landing
Tools/Scripts/svn-apply failed to apply attachment 424721 [details] to trunk. Please resolve the conflicts and upload a new patch.
Created attachment 424724 [details] Patch for landing
Committed r275260: <https://commits.webkit.org/r275260> All reviewed patches have been landed. Closing bug and clearing flags on attachment 424724 [details].