Bug 237969 – Implement PCM to SKAdNetwork bridge

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Implement PCM to SKAdNetwork bridge

[Alex Christensen]

Created attachment 454864 [details]
Patch

[Alex Christensen]

Created attachment 454891 [details]
Patch

[John Wilander]

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.

[Alex Christensen]

Created attachment 455446 [details]
Patch

[Alex Christensen]

(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

[Alex Christensen]

r291735