Bug 219763 – Accept click measurement data from hosting application

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

[Alex Christensen]

Accept click measurement data from hosting application

[Alex Christensen]

Created attachment 415945 [details]
Patch

[Alex Christensen]

<rdar://problem/72121094>

[John Wilander]

Comment on attachment 415945 [details]
Patch

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

r+ with comments.

> Source/WebCore/loader/PrivateClickMeasurement.h:243
> +        , m_purchaser { WTFMove(purchaser) }

These will be in-memory-only for now since we're not updating the persistence layer. That's perfectly OK. However, it would be nice to restrict their size. I believe we've said 100 characters max. Not a deal breaker.

> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:79
> +// FIXME: Move this to UIKitSPI.h

Do we need a bug tracking this?

> Source/WebKit/UIProcess/WebPageProxy.h:2909
> +    Optional<WebCore::PrivateClickMeasurement> m_privateClickMeasurement;

I wonder if this is too generic of a member name. Isn't m_newPageNavigationPrivateClickMeasurement a pretty good description even with your patch?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/EventAttribution.mm:56
> +    _sourceIdentifier = 42;

Could we add a test with a too large sourceIdentifier? That's the most important input validation here.

[Alex Christensen]

Comment on attachment 415945 [details]
Patch

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

>> Source/WebCore/loader/PrivateClickMeasurement.h:243
>> +        , m_purchaser { WTFMove(purchaser) }
> 
> These will be in-memory-only for now since we're not updating the persistence layer. That's perfectly OK. However, it would be nice to restrict their size. I believe we've said 100 characters max. Not a deal breaker.

This was just to make the API getter work, which needs this information in memory to create a new event attribution.

>> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:79
>> +// FIXME: Move this to UIKitSPI.h
> 
> Do we need a bug tracking this?

I think I should do it in this same patch.

>> Source/WebKit/UIProcess/WebPageProxy.h:2909
>> +    Optional<WebCore::PrivateClickMeasurement> m_privateClickMeasurement;
> 
> I wonder if this is too generic of a member name. Isn't m_newPageNavigationPrivateClickMeasurement a pretty good description even with your patch?

In the first patch it's not, but if I move it to WKWebViewConfiguration, it will be.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/EventAttribution.mm:56
>> +    _sourceIdentifier = 42;
> 
> Could we add a test with a too large sourceIdentifier? That's the most important input validation here.

sourceIdentifier is a uint8_t, which doesn't allow sourceIdentifier to be larger than 255.  The language prevents me from overflow.

[Alex Christensen]

Created attachment 415960 [details]
Patch

[Alex Christensen]

Created attachment 415963 [details]
Patch

[EWS]

Committed r270669: <https://trac.webkit.org/changeset/270669>

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