We should change WebKit to use the official name of the proposed standard Private Click Measurement https://github.com/privacycg/private-click-measurement.
<rdar://problem/71094296>
Created attachment 413646 [details] Patch
Sorry about the size of this patch. :(
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Comment on attachment 413646 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413646&action=review > Source/JavaScriptCore/ChangeLog:20016 > - Web Inspector: provide a way for inspector to turn on/off ITP debug mode and AdClickAttribution debug mode > + Web Inspector: provide a way for inspector to turn on/off ITP debug mode and PrivateClickMeasurement debug mode I don't think we should modify old ChangeLog entries > Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:101 > -localizedStrings["Ad Click Attribution Debug Mode"] = "Ad Click Attribution Debug Mode"; > +localizedStrings["Private Click Measurement Debug Mode"] = "Private Click Measurement Debug Mode"; This should be sorted alphabetically, so please move it down below. Alternatively, you can just run this command to regenerate this entire file from all the `WI.UIString` calls inside `Source/WebInspectorUI/UserInterface/` :) ``` $ Tools/Scripts/extract-localizable-js-strings --utf8 Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js Source/WebInspectorUI/UserInterface ``` > Source/WebInspectorUI/UserInterface/Base/Main.js:-2347 > - {name: WI.UIString("Ad Click Attribution Debug Mode"), setting: InspectorBackend.Enum.Page.Setting.AdClickAttributionDebugModeEnabled, value: true}, We'll actually want to keep this entry for when inspecting older iOS devices (see below for explanation). I believe this code properly handles when `InspectorBackend.Enum.Page.Setting.*` does not exist, which prevents anything from being shown, so I think you could just do something like this: ``` {name: WI.UIString("ITP Debug Mode"), setting: InspectorBackend.Enum.Page.Setting.ITPDebugModeEnabled, value: true}, // COMPATIBILITY (iOS 14.0): `Page.Setting.AdClickAttributionDebugModeEnabled` was renamed to `Page.Setting.PrivateClickMeasurementDebugModeEnabled`. {name: WI.UIString("Private Click Measurement Debug Mode"), setting: InspectorBackend.Enum.Page.Setting.PrivateClickMeasurementDebugModeEnabled, value: true}, {name: WI.UIString("Ad Click Attribution Debug Mode"), setting: InspectorBackend.Enum.Page.Setting.AdClickAttributionDebugModeEnabled, value: true}, ``` and at runtime only one of the two will appear. A simple way to test this is to do a local build with your changes and confirm that: 1. inspecting an older iOS device only shows "Ad Click Attribution Debug Mode" in the [device settings menu](https://webkit.org/web-inspector/device-settings/). 2. inspecting any page using your local build (on the same machine, not an iOS device) shows "Private Click Measurement Debug Mode" in the [device settings menu](https://webkit.org/web-inspector/device-settings/). Note that you may have to enable Web Inspector's debug mode (⌥⇧⌘D) and then re-open Web Inspector in order to see the [device settings menu](https://webkit.org/web-inspector/device-settings/). > Source/WebInspectorUI/UserInterface/Models/ConsoleMessage.js:-116 > - AdClickAttribution: "ad-click-attribution", ditto ``` ITPDebug: "itp-debug", PrivateClickMeasurement: "private-click-measurement", Other: "other", // COMPATIBILITY (iOS 14.0): `Console.ChannelSource.AdClickAttribution` was renamed to `Console.ChannelSource.PrivateClickMeasurement`. AdClickAttribution: "ad-click-attribution", ``` > Source/WebInspectorUI/UserInterface/Models/IssueMessage.js:-72 > - case WI.ConsoleMessage.MessageSource.AdClickAttribution: ditto ``` case WI.ConsoleMessage.MessageSource.ITPDebug: case WI.ConsoleMessage.MessageSource.PrivateClickMeasurement: case WI.ConsoleMessage.MessageSource.AdClickAttribution: // COMPATIBILITY (iOS 14.0): `Console.ChannelSource.AdClickAttribution` was renamed to `Console.ChannelSource.PrivateClickMeasurement`. case WI.ConsoleMessage.MessageSource.Other: ``` > Source/WebInspectorUI/UserInterface/Protocol/Legacy/14.0/InspectorBackendCommands.js:154 > -InspectorBackend.registerEnum("Console.ChannelSource", {XML: "xml", JavaScript: "javascript", Network: "network", ConsoleAPI: "console-api", Storage: "storage", Appcache: "appcache", Rendering: "rendering", CSS: "css", Security: "security", ContentBlocker: "content-blocker", Media: "media", MediaSource: "mediasource", WebRTC: "webrtc", ITPDebug: "itp-debug", AdClickAttribution: "ad-click-attribution", Other: "other"}); > +InspectorBackend.registerEnum("Console.ChannelSource", {XML: "xml", JavaScript: "javascript", Network: "network", ConsoleAPI: "console-api", Storage: "storage", Appcache: "appcache", Rendering: "rendering", CSS: "css", Security: "security", ContentBlocker: "content-blocker", Media: "media", MediaSource: "mediasource", WebRTC: "webrtc", ITPDebug: "itp-debug", PrivateClickMeasurement: "private-click-measurement", Other: "other"}); We actually almost never want to change anything inside `Source/WebInspectorUI/UserInterface/Protocol/Legacy/` (and `Source/WebInspectorUI/Versions/`, but you didn't touch that so it's OK) as those files are used by the Web Inspector frontend to determine the capabilities of older devices when they're remotely inspected. As an example, when Web Inspector used to inspect content using a local build of WebKit, the `InspectorBackendCommands.js` that's used is the one that's built with WebKit: <build>/<configuration>/DerivedSources/JavaScriptCore/inspector/InspectorBackendCommands.js (which is later copied into `WebInspectorUI.framework`). The default behavior is to just use the backend commands built with WebKit. When inspecting an older device, however, Web Inspector uses the closest (rounded down, so 13.2 becomes 13.0 when choosing between 13.0 and 13.4) backends commands file for that device's iOS version. This is what allows the Web Inspector frontend to be updated without having to worry about breaking inspecting older devices, as the capabilities of those devices are frozen in stone in these files. As such, we'll unfortunately need to leave this file as-is and have compatibility checks for `PrivateClickMeasurement` vs `AdClickAttribution` in various spots (see above).
(In reply to Devin Rousso from comment #5) > Comment on attachment 413646 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=413646&action=review > > > Source/JavaScriptCore/ChangeLog:20016 > > - Web Inspector: provide a way for inspector to turn on/off ITP debug mode and AdClickAttribution debug mode > > + Web Inspector: provide a way for inspector to turn on/off ITP debug mode and PrivateClickMeasurement debug mode > > I don't think we should modify old ChangeLog entries Agreed! That's a find+replace mistake. Good catch. > > Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:101 > > -localizedStrings["Ad Click Attribution Debug Mode"] = "Ad Click Attribution Debug Mode"; > > +localizedStrings["Private Click Measurement Debug Mode"] = "Private Click Measurement Debug Mode"; > > This should be sorted alphabetically, so please move it down below. Will do. > Alternatively, you can just run this command to regenerate this entire file > from all the `WI.UIString` calls inside > `Source/WebInspectorUI/UserInterface/` :) > ``` > $ Tools/Scripts/extract-localizable-js-strings --utf8 > Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js > Source/WebInspectorUI/UserInterface > ``` > > > Source/WebInspectorUI/UserInterface/Base/Main.js:-2347 > > - {name: WI.UIString("Ad Click Attribution Debug Mode"), setting: InspectorBackend.Enum.Page.Setting.AdClickAttributionDebugModeEnabled, value: true}, > > We'll actually want to keep this entry for when inspecting older iOS devices > (see below for explanation). > > I believe this code properly handles when > `InspectorBackend.Enum.Page.Setting.*` does not exist, which prevents > anything from being shown, so I think you could just do something like this: > ``` > {name: WI.UIString("ITP Debug Mode"), setting: > InspectorBackend.Enum.Page.Setting.ITPDebugModeEnabled, value: true}, > > // COMPATIBILITY (iOS 14.0): > `Page.Setting.AdClickAttributionDebugModeEnabled` was renamed to > `Page.Setting.PrivateClickMeasurementDebugModeEnabled`. > {name: WI.UIString("Private Click Measurement Debug Mode"), setting: > InspectorBackend.Enum.Page.Setting.PrivateClickMeasurementDebugModeEnabled, > value: true}, > {name: WI.UIString("Ad Click Attribution Debug Mode"), setting: > InspectorBackend.Enum.Page.Setting.AdClickAttributionDebugModeEnabled, > value: true}, > ``` > and at runtime only one of the two will appear. OK, will fix. > A simple way to test this is to do a local build with your changes and > confirm that: > 1. inspecting an older iOS device only shows "Ad Click Attribution Debug > Mode" in the [device settings > menu](https://webkit.org/web-inspector/device-settings/). > 2. inspecting any page using your local build (on the same machine, not an > iOS device) shows "Private Click Measurement Debug Mode" in the [device > settings menu](https://webkit.org/web-inspector/device-settings/). Note > that you may have to enable Web Inspector's debug mode (⌥⇧⌘D) and then > re-open Web Inspector in order to see the [device settings > menu](https://webkit.org/web-inspector/device-settings/). I'm not too worried about this since Ad Click Attribution is an experimental feature that has always been off by default. Devices with the old naming will never have it on by default. I just don't want anything to crash for old stuff. > > Source/WebInspectorUI/UserInterface/Models/ConsoleMessage.js:-116 > > - AdClickAttribution: "ad-click-attribution", > > ditto > ``` > ITPDebug: "itp-debug", > PrivateClickMeasurement: "private-click-measurement", > Other: "other", > > // COMPATIBILITY (iOS 14.0): `Console.ChannelSource.AdClickAttribution` > was renamed to `Console.ChannelSource.PrivateClickMeasurement`. > AdClickAttribution: "ad-click-attribution", > ``` > > > Source/WebInspectorUI/UserInterface/Models/IssueMessage.js:-72 > > - case WI.ConsoleMessage.MessageSource.AdClickAttribution: > > ditto > ``` > case WI.ConsoleMessage.MessageSource.ITPDebug: > case WI.ConsoleMessage.MessageSource.PrivateClickMeasurement: > case WI.ConsoleMessage.MessageSource.AdClickAttribution: // > COMPATIBILITY (iOS 14.0): `Console.ChannelSource.AdClickAttribution` was > renamed to `Console.ChannelSource.PrivateClickMeasurement`. > case WI.ConsoleMessage.MessageSource.Other: > ``` > > > Source/WebInspectorUI/UserInterface/Protocol/Legacy/14.0/InspectorBackendCommands.js:154 > > -InspectorBackend.registerEnum("Console.ChannelSource", {XML: "xml", JavaScript: "javascript", Network: "network", ConsoleAPI: "console-api", Storage: "storage", Appcache: "appcache", Rendering: "rendering", CSS: "css", Security: "security", ContentBlocker: "content-blocker", Media: "media", MediaSource: "mediasource", WebRTC: "webrtc", ITPDebug: "itp-debug", AdClickAttribution: "ad-click-attribution", Other: "other"}); > > +InspectorBackend.registerEnum("Console.ChannelSource", {XML: "xml", JavaScript: "javascript", Network: "network", ConsoleAPI: "console-api", Storage: "storage", Appcache: "appcache", Rendering: "rendering", CSS: "css", Security: "security", ContentBlocker: "content-blocker", Media: "media", MediaSource: "mediasource", WebRTC: "webrtc", ITPDebug: "itp-debug", PrivateClickMeasurement: "private-click-measurement", Other: "other"}); > > We actually almost never want to change anything inside > `Source/WebInspectorUI/UserInterface/Protocol/Legacy/` (and > `Source/WebInspectorUI/Versions/`, but you didn't touch that so it's OK) as > those files are used by the Web Inspector frontend to determine the > capabilities of older devices when they're remotely inspected. > > As an example, when Web Inspector used to inspect content using a local > build of WebKit, the `InspectorBackendCommands.js` that's used is the one > that's built with WebKit: > <build>/<configuration>/DerivedSources/JavaScriptCore/inspector/ > InspectorBackendCommands.js (which is later copied into > `WebInspectorUI.framework`). The default behavior is to just use the > backend commands built with WebKit. When inspecting an older device, > however, Web Inspector uses the closest (rounded down, so 13.2 becomes 13.0 > when choosing between 13.0 and 13.4) backends commands file for that > device's iOS version. > > This is what allows the Web Inspector frontend to be updated without having > to worry about breaking inspecting older devices, as the capabilities of > those devices are frozen in stone in these files. As such, we'll > unfortunately need to leave this file as-is and have compatibility checks > for `PrivateClickMeasurement` vs `AdClickAttribution` in various spots (see > above). Thanks for looking and explaining, Devin!
Created attachment 413745 [details] Patch
I hope I've now addressed all your comments, Devin.
Comment on attachment 413745 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413745&action=review r=me with a few remaining fixes > Source/JavaScriptCore/ChangeLog:20023 > - - `AdClickAttributionDebugModeEnabled` > + - `PrivateClickMeasurementDebugModeEnabled` I don't think we should modify old ChangeLog entries > Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:-101 > -localizedStrings["Ad Click Attribution Debug Mode"] = "Ad Click Attribution Debug Mode"; this shouldn't be removed since it's still used in `Source/WebInspectorUI/UserInterface/Base/Main.js` > Source/WebInspectorUI/UserInterface/Base/Main.js:2349 > + {name: WI.UIString("Ad Click Attribution Debug Mode"), setting: InspectorBackend.Enum.Page.Setting.AdClickAttributionDebugModeEnabled, value: true}, ], Style: the `],` should be on a separate line
(In reply to Devin Rousso from comment #9) > Comment on attachment 413745 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=413745&action=review > > r=me with a few remaining fixes > > > Source/JavaScriptCore/ChangeLog:20023 > > - - `AdClickAttributionDebugModeEnabled` > > + - `PrivateClickMeasurementDebugModeEnabled` > > I don't think we should modify old ChangeLog entries I'm positive I fixed this. Maybe this is another instance? Regardless, will fix. > > Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:-101 > > -localizedStrings["Ad Click Attribution Debug Mode"] = "Ad Click Attribution Debug Mode"; > > this shouldn't be removed since it's still used in > `Source/WebInspectorUI/UserInterface/Base/Main.js` Will fix. > > Source/WebInspectorUI/UserInterface/Base/Main.js:2349 > > + {name: WI.UIString("Ad Click Attribution Debug Mode"), setting: InspectorBackend.Enum.Page.Setting.AdClickAttributionDebugModeEnabled, value: true}, ], > > Style: the `],` should be on a separate line Will fix. Thanks, Devin!
Created attachment 413751 [details] Patch for landing
Committed r269660: <https://trac.webkit.org/changeset/269660> All reviewed patches have been landed. Closing bug and clearing flags on attachment 413751 [details].
Re-opened since this is blocked by bug 218786
Created attachment 413868 [details] Patch
The failed inspector test on mac-wk2 looks like a flaky test: https://results.webkit.org/?suite=layout-tests&test=http%2Ftests%2Finspector%2Fnetwork%2Fresource-timing.html
I asked Devin Rousso about the InspectorPageAgent.cpp build errors on the win bot earlier and he said: "For some reason the windows bot sometimes lags when dealing with changes in the Web Inspector protocol JSON files. Usually clicking 'Retry failed builds' does the trick. I can tell you tho that your changes are almost definitely not gonna cause a problem, especially since it builds fine on other platforms."
Comment on attachment 413868 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413868&action=review > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataRecordPrivate.h:37 > WK_EXTERN NSString * const _WKWebsiteDataTypeAdClickAttributions WK_API_AVAILABLE(macos(10.15), ios(13.0)); We should deprecate this now or soonish. Deprecating now would require ignoring deprecation warnings in Safari before landing this.
(In reply to Alex Christensen from comment #17) > Comment on attachment 413868 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=413868&action=review > > > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataRecordPrivate.h:37 > > WK_EXTERN NSString * const _WKWebsiteDataTypeAdClickAttributions WK_API_AVAILABLE(macos(10.15), ios(13.0)); > > We should deprecate this now or soonish. Deprecating now would require > ignoring deprecation warnings in Safari before landing this. Agreed. I just filed https://bugs.webkit.org/show_bug.cgi?id=218833 to track it. Thanks, Alex!
I asked Devin about the layout test crash in inspector/console/webcore-logging.html and he does not think it's related. He said "The stack involves InspectorFrontendAPIDispatcher. Brian Burg has been working on that lately."
The Windows bot is now complaining about privateClickMeasurementEnabled which should be generated into WebCore::Settings.
Committed r269712: <https://trac.webkit.org/changeset/269712> All reviewed patches have been landed. Closing bug and clearing flags on attachment 413868 [details].
I have pinged Brian Burg about the inspector test crasher.
(In reply to John Wilander from comment #22) > I have pinged Brian Burg about the inspector test crasher. A fix is being tracked here: https://bugs.webkit.org/show_bug.cgi?id=218840 Thanks for the heads up! WebKitLegacy is truly weird.