Bug 223238 – PCM: Test infrastructure for sending attribution reports to attribution website too

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[John Wilander]

We need the ability to set two different reporting endpoints in testing of PCM when it sends attribution reports to attribution website too.

[Radar WebKit Bug Importer]

<rdar://problem/75462031>

[John Wilander]

Created attachment 423296 [details]
Patch

[John Wilander]

Created attachment 423339 [details]
Patch

[John Wilander]

The timed out api-mac test is unrelated.

[Brent Fulgham]

Comment on attachment 423339 [details]
Patch

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

r=me

> LayoutTests/http/tests/contentextensions/block-private-click-measurement.html:18
> +        testRunner.setPrivateClickMeasurementAttributionReportURLsForTesting("http://127.0.0.1:8000/privateClickMeasurement/resources/conversionReport.php", "http://localhost:8000/privateClickMeasurement/resources/conversionReport.php");

It's too bad we don't have two separate endpoints (conversionReportForAttribution.php or something)

[John Wilander]

(In reply to Brent Fulgham from comment #5)
> Comment on attachment 423339 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=423339&action=review
> 
> r=me
> 
> > LayoutTests/http/tests/contentextensions/block-private-click-measurement.html:18
> > +        testRunner.setPrivateClickMeasurementAttributionReportURLsForTesting("http://127.0.0.1:8000/privateClickMeasurement/resources/conversionReport.php", "http://localhost:8000/privateClickMeasurement/resources/conversionReport.php");
> 
> It's too bad we don't have two separate endpoints
> (conversionReportForAttribution.php or something)

Yeah, I wanted that when Jiewen started reusing the same PHP file for multiple things. We should revisit it.

Thanks, Brent!

[John Wilander]

Comment on attachment 423339 [details]
Patch

Trying the cq again.

[EWS]

Committed r274513: <https://commits.webkit.org/r274513>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423339 [details].

[Darin Adler]

Comment on attachment 423339 [details]
Patch

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

> Tools/WebKitTestRunner/TestInvocation.cpp:1358
> +        auto sourceURL = WKURLCreateWithUTF8CString(toWTFString(stringValue(testDictionary, "SourceURLString")).utf8().data());
> +        auto attributeOnURL = WKURLCreateWithUTF8CString(toWTFString(stringValue(testDictionary, "AttributeOnURLString")).utf8().data());

Need adoptWK here so these don’t leak.

[John Wilander]

(In reply to Darin Adler from comment #9)
> Comment on attachment 423339 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=423339&action=review
> 
> > Tools/WebKitTestRunner/TestInvocation.cpp:1358
> > +        auto sourceURL = WKURLCreateWithUTF8CString(toWTFString(stringValue(testDictionary, "SourceURLString")).utf8().data());
> > +        auto attributeOnURL = WKURLCreateWithUTF8CString(toWTFString(stringValue(testDictionary, "AttributeOnURLString")).utf8().data());
> 
> Need adoptWK here so these don’t leak.

I willfully admit that I'm not great with these conversions and API types. When I use adoptWK here I get:
error: no viable conversion from 'WebKit::WKRetainPtr<const OpaqueWKURL *>' to 'WKURLRef' (aka 'const OpaqueWKURL *')

What is the right course of action? Thanks!

[Darin Adler]

Comment on attachment 423339 [details]
Patch

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

>>> Tools/WebKitTestRunner/TestInvocation.cpp:1358
>>> +        auto attributeOnURL = WKURLCreateWithUTF8CString(toWTFString(stringValue(testDictionary, "AttributeOnURLString")).utf8().data());
>> 
>> Need adoptWK here so these don’t leak.
> 
> I willfully admit that I'm not great with these conversions and API types. When I use adoptWK here I get:
> error: no viable conversion from 'WebKit::WKRetainPtr<const OpaqueWKURL *>' to 'WKURLRef' (aka 'const OpaqueWKURL *')
> 
> What is the right course of action? Thanks!

You also have to add .get() where they are used:

    TestController::singleton().setPrivateClickMeasurementAttributionReportURLsForTesting(sourceURL.get(), attributeOnURL.get());

Doing that should get rid of those errors.

[John Wilander]

(In reply to Darin Adler from comment #11)
> Comment on attachment 423339 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=423339&action=review
> 
> >>> Tools/WebKitTestRunner/TestInvocation.cpp:1358
> >>> +        auto attributeOnURL = WKURLCreateWithUTF8CString(toWTFString(stringValue(testDictionary, "AttributeOnURLString")).utf8().data());
> >> 
> >> Need adoptWK here so these don’t leak.
> > 
> > I willfully admit that I'm not great with these conversions and API types. When I use adoptWK here I get:
> > error: no viable conversion from 'WebKit::WKRetainPtr<const OpaqueWKURL *>' to 'WKURLRef' (aka 'const OpaqueWKURL *')
> > 
> > What is the right course of action? Thanks!
> 
> You also have to add .get() where they are used:
> 
>    
> TestController::singleton().
> setPrivateClickMeasurementAttributionReportURLsForTesting(sourceURL.get(),
> attributeOnURL.get());
> 
> Doing that should get rid of those errors.

Got it. I will land a follow-up patch shortly. OK if I state you as reviewer?

[Darin Adler]

(In reply to John Wilander from comment #12)
> Got it. I will land a follow-up patch shortly. OK if I state you as reviewer?

Sure. Sometimes people just write "Suggested by" instead of "Reviewed by" in cases like this.