Bug 218730 – PCM: Change from ad-click-attribution to private-click-measurement (in all forms, including .well-known URL)

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[John Wilander]

We should change WebKit to use the official name of the proposed standard Private Click Measurement https://github.com/privacycg/private-click-measurement.

[John Wilander]

<rdar://problem/71094296>

[John Wilander]

Created attachment 413646 [details]
Patch

[John Wilander]

Sorry about the size of this patch. :(

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Devin Rousso]

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).

[John Wilander]

(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!

[John Wilander]

Created attachment 413745 [details]
Patch

[John Wilander]

I hope I've now addressed all your comments, Devin.

[Devin Rousso]

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

[John Wilander]

(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!

[John Wilander]

Created attachment 413751 [details]
Patch for landing

[EWS]

Committed r269660: <https://trac.webkit.org/changeset/269660>

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 218786

[John Wilander]

Created attachment 413868 [details]
Patch

[John Wilander]

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

[John Wilander]

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."

[Alex Christensen]

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.

[John Wilander]

(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!

[John Wilander]

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."

[John Wilander]

The Windows bot is now complaining about privateClickMeasurementEnabled which should be generated into WebCore::Settings.

[EWS]

Committed r269712: <https://trac.webkit.org/changeset/269712>

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

[John Wilander]

I have pinged Brian Burg about the inspector test crasher.

[Blaze Burg]

(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.