PCM should include the bundle ID of the app from which it originated
Created attachment 438861 [details] Patch
<rdar://problem/83065221>
Created attachment 438866 [details] Patch
Created attachment 438904 [details] Patch
(In reply to Alex Christensen from comment #4) > Created attachment 438904 [details] > Patch WebKitTestRunner has different bundleIDs for iOS and macOS, that is causing the test failures.
Comment on attachment 438904 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=438904&action=review > Source/WebKit/NetworkProcess/DatabaseUtilities.cpp:153 > + constexpr auto mobileSafariBundleID = "com.apple.mobilesafari"; I wonder if we should use com.apple.Safari for macOS. > Source/WebKit/NetworkProcess/PrivateClickMeasurement/PrivateClickMeasurementDatabase.cpp:370 > + attributionToStringForTesting(buildPrivateClickMeasurementFromDatabase(*unattributedScopedStatement.get(), PrivateClickMeasurementAttributionType::Unattributed))); Good change. > Source/WebKit/NetworkProcess/PrivateClickMeasurement/PrivateClickMeasurementDatabase.cpp:406 > + if (attributionTriggerData != std::numeric_limits<uint32_t>::max()) { I don't think this check is needed. If attributionTriggerData in the sqlite result is -1, buildPrivateClickMeasurementFromDatabase will not set the attribution value, so triggerData will be nullopt. > Source/WebKit/NetworkProcess/PrivateClickMeasurement/PrivateClickMeasurementDatabase.cpp:686 > + RELEASE_LOG_ERROR(PrivateClickMeasurement, "%p - Database::getColumnsFromTableInfoStatement Unable to prepare statement to fetch schema for table, error message: %" PRIVATE_LOG_STRING, this, m_database.lastErrorMsg()); Nit: Database::columnsForTable instead > Source/WebKit/NetworkProcess/PrivateClickMeasurement/PrivateClickMeasurementDatabase.cpp:713 > + statement->reset(); No need for this reset since this statement will go out of scope before you use it again. > Tools/TestWebKitAPI/Tests/WebKitCocoa/EventAttribution.mm:343 > + [webView _addEventAttributionWithSourceID:42 destinationURL:exampleURL() sourceDescription:@"test source description" purchaser:@"test purchaser" reportEndpoint:server.request().URL optionalNonce:nil applicationBundleID:@"test.bundle.id"]; What is the point of passing the bundleID here if we never check it for the test? > Tools/TestWebKitAPI/Tests/WebKitCocoa/PrivateClickMeasurement.mm:201 > + "Application bundle identifier: com.apple.mobilesafari\n"; Seems like this should be com.apple.Safari for macOS versions of this test.
(In reply to Kate Cheney from comment #6) > Comment on attachment 438904 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=438904&action=review > > > Source/WebKit/NetworkProcess/DatabaseUtilities.cpp:153 > > + constexpr auto mobileSafariBundleID = "com.apple.mobilesafari"; > > I wonder if we should use com.apple.Safari for macOS. I don't think so. com.apple.mobilesafari is the only application that has ever had this enabled. It's never been used on Mac. This is only for legacy data migration purposes. Maybe I'll rename it to make it more clear that is what is going on. > > Tools/TestWebKitAPI/Tests/WebKitCocoa/EventAttribution.mm:343 > > + [webView _addEventAttributionWithSourceID:42 destinationURL:exampleURL() sourceDescription:@"test source description" purchaser:@"test purchaser" reportEndpoint:server.request().URL optionalNonce:nil applicationBundleID:@"test.bundle.id"]; > > What is the point of passing the bundleID here if we never check it for the > test? I'm making it so that there is no way to make a PrivateClickMeasurement without a bundle ID except for pulling them out of legacy databases from mobile safari.
(In reply to Alex Christensen from comment #7) > (In reply to Kate Cheney from comment #6) > > Comment on attachment 438904 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=438904&action=review > > > > > Source/WebKit/NetworkProcess/DatabaseUtilities.cpp:153 > > > + constexpr auto mobileSafariBundleID = "com.apple.mobilesafari"; > > > > I wonder if we should use com.apple.Safari for macOS. > I don't think so. com.apple.mobilesafari is the only application that has > ever had this enabled. It's never been used on Mac. This is only for > legacy data migration purposes. Maybe I'll rename it to make it more clear > that is what is going on. MobileSafari is the only application that has PCM *app-to-web* enabled but regular web-to-web is very much enabled in Safari on macOS. > > > Tools/TestWebKitAPI/Tests/WebKitCocoa/EventAttribution.mm:343 > > > + [webView _addEventAttributionWithSourceID:42 destinationURL:exampleURL() sourceDescription:@"test source description" purchaser:@"test purchaser" reportEndpoint:server.request().URL optionalNonce:nil applicationBundleID:@"test.bundle.id"]; > > > > What is the point of passing the bundleID here if we never check it for the > > test? > I'm making it so that there is no way to make a PrivateClickMeasurement > without a bundle ID except for pulling them out of legacy databases from > mobile safari.
Comment on attachment 438904 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=438904&action=review I assume you will add tests with multiple bundle IDs to make sure the matching logic does the right thing once you add it. >> Source/WebKit/NetworkProcess/PrivateClickMeasurement/PrivateClickMeasurementDatabase.cpp:713 >> + statement->reset(); > > No need for this reset since this statement will go out of scope before you use it again. Can we add an ASSERT that tells everyone that we are relying on that side effect? I'm thinking of when another engineer comes along in two years and makes changes.
Created attachment 438965 [details] Patch
Comment on attachment 438965 [details] Patch r=me
Created attachment 438971 [details] Patch
Committed r282884 (242013@main): <https://commits.webkit.org/242013@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 438971 [details].