Bug 223238 - PCM: Test infrastructure for sending attribution reports to attribution website too
Summary: PCM: Test infrastructure for sending attribution reports to attribution websi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Wilander
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-03-15 21:25 PDT by John Wilander
Modified: 2021-04-09 22:11 PDT (History)
3 users (show)

See Also:


Attachments
Patch (39.84 KB, patch)
2021-03-15 21:45 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (40.90 KB, patch)
2021-03-16 09:27 PDT, John Wilander
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Wilander 2021-03-15 21:25:16 PDT
We need the ability to set two different reporting endpoints in testing of PCM when it sends attribution reports to attribution website too.
Comment 1 Radar WebKit Bug Importer 2021-03-15 21:25:40 PDT
<rdar://problem/75462031>
Comment 2 John Wilander 2021-03-15 21:45:30 PDT
Created attachment 423296 [details]
Patch
Comment 3 John Wilander 2021-03-16 09:27:11 PDT
Created attachment 423339 [details]
Patch
Comment 4 John Wilander 2021-03-16 10:20:13 PDT
The timed out api-mac test is unrelated.
Comment 5 Brent Fulgham 2021-03-16 11:43:23 PDT
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)
Comment 6 John Wilander 2021-03-16 11:44:47 PDT
(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!
Comment 7 John Wilander 2021-03-16 13:21:57 PDT
Comment on attachment 423339 [details]
Patch

Trying the cq again.
Comment 8 EWS 2021-03-16 13:57:09 PDT
Committed r274513: <https://commits.webkit.org/r274513>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423339 [details].
Comment 9 Darin Adler 2021-03-16 15:06:46 PDT
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.
Comment 10 John Wilander 2021-03-16 15:13:51 PDT
(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!
Comment 11 Darin Adler 2021-03-16 15:15:58 PDT
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.
Comment 12 John Wilander 2021-03-16 15:19:48 PDT
(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?
Comment 13 Darin Adler 2021-03-16 15:28:05 PDT
(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.