Bug 219763 - Accept click measurement data from hosting application
Summary: Accept click measurement data from hosting application
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-12-10 16:52 PST by Alex Christensen
Modified: 2020-12-10 23:41 PST (History)
5 users (show)

See Also:


Attachments
Patch (29.02 KB, patch)
2020-12-10 16:56 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (35.41 KB, patch)
2020-12-10 19:18 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (32.82 KB, patch)
2020-12-10 20:01 PST, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2020-12-10 16:52:59 PST
Accept click measurement data from hosting application
Comment 1 Alex Christensen 2020-12-10 16:56:56 PST
Created attachment 415945 [details]
Patch
Comment 2 Alex Christensen 2020-12-10 16:56:59 PST
<rdar://problem/72121094>
Comment 3 John Wilander 2020-12-10 17:48:57 PST
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.
Comment 4 Alex Christensen 2020-12-10 17:57:48 PST
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.
Comment 5 Alex Christensen 2020-12-10 19:18:37 PST
Created attachment 415960 [details]
Patch
Comment 6 Alex Christensen 2020-12-10 20:01:39 PST
Created attachment 415963 [details]
Patch
Comment 7 EWS 2020-12-10 23:41:41 PST
Committed r270669: <https://trac.webkit.org/changeset/270669>

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