Accept click measurement data from hosting application
Created attachment 415945 [details] Patch
<rdar://problem/72121094>
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 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.
Created attachment 415960 [details] Patch
Created attachment 415963 [details] Patch
Committed r270669: <https://trac.webkit.org/changeset/270669> All reviewed patches have been landed. Closing bug and clearing flags on attachment 415963 [details].