Implement PCM to SKAdNetwork bridge
Created attachment 454864 [details] Patch
Created attachment 454891 [details] Patch
Comment on attachment 454891 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454891&action=review r=me with comments. Please make sure the WK2 bots are green before landing. > Source/WTF/wtf/PlatformHave.h:1227 > +#if (PLATFORM(IOS) && __IPHONE_OS_VERSION_MAX_ALLOWED >= 160000) I think this should be TBD. > Source/WebKit/ChangeLog:8 > + Please add some comments on the changes in the WebKit layer. > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1822 > +void NetworkSessionCocoa::donateToSKAdNetwork(WebCore::PrivateClickMeasurement&& pcm) This method doesn't move the pcm object further. Would it make sense to use & const instead? > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1826 > + config.get().appAdamId = @(*pcm.adamID()); I think we should check that there is an adamID and if not, broadcast an error message and return without forwarding to the daemon. See my comment further down on isSKAdNetworkAttribution(). > Source/WebKit/UIProcess/WebPageProxy.cpp:4982 > + if (privateClickMeasurement->destinationSite().matches(frame->url()) || privateClickMeasurement->adamID()) Since we use it this way, I'd prefer a convenience function isSKAdNetworkAttribution() which internally checks whether it is a valid such PCM object. > Tools/TestWebKitAPI/Tests/WebKitCocoa/EventAttribution.mm:599 > + EXPECT_WK_STREQ(consoleMessages[1], "Submitting potential install attribution for AdamId: 1234567890, adNetworkRegistrableDomain: destination, impressionId: MTIzNDU2Nzg5MDEyMzQ1Ng, sourceWebRegistrableDomain: example.com, version: 3"); Huh, I didn't know you could test console messages. Neat.
Created attachment 455446 [details] Patch
(In reply to John Wilander from comment #3) > This method doesn't move the pcm object further. Would it make sense to use > & const instead? It is consuming the pcm, so although it doesn't call a move constructor or anything, I think move semantics is conceptually how we should be thinking about the pcm that is moved here and we shouldn't do anything with it afterwards. > Since we use it this way, I'd prefer a convenience function > isSKAdNetworkAttribution() which internally checks whether it is a valid > such PCM object. Good idea. Better name for the same thing. > > Tools/TestWebKitAPI/Tests/WebKitCocoa/EventAttribution.mm:599 > > + EXPECT_WK_STREQ(consoleMessages[1], "Submitting potential install attribution for AdamId: 1234567890, adNetworkRegistrableDomain: destination, impressionId: MTIzNDU2Nzg5MDEyMzQ1Ng, sourceWebRegistrableDomain: example.com, version: 3"); > > Huh, I didn't know you could test console messages. Neat. You can on macOS
r291735