Bug 221113 – PCM: Indicate if network load data is user identifiable

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Kate Cheney]

We should add a way to determine if a load is carrying user-identifiable data.

[Kate Cheney]

rdar://problem/71348187

[Kate Cheney]

Created attachment 418694 [details]
Patch

[John Wilander]

Comment on attachment 418694 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=418694&action=review

Looks good but let's work some more on naming and flip the currently used setting.

> Source/WebCore/ChangeLog:3
> +        PCM: Indicate if load data is user identifiable

Maybe add "network load"

> Source/WebKit/ChangeLog:3
> +        PCM: Indicate if load data is user identifiable

Ditto.

> Source/WebKit/NetworkProcess/NetworkDataTask.cpp:57
> +    return NetworkDataTaskCocoa::create(session, client, parameters.request, parameters.webFrameID, parameters.webPageID, parameters.storedCredentialsPolicy, parameters.contentSniffingPolicy, parameters.contentEncodingSniffingPolicy, parameters.shouldClearReferrerOnHTTPSToHTTPRedirect, parameters.shouldPreconnectOnly, parameters.isMainFrameNavigation, parameters.isMainResourceNavigationForAnyFrame, parameters.networkActivityTracker, parameters.isNavigatingToAppBoundDomain, parameters.shouldRelaxThirdPartyCookieBlocking, parameters.carriesUserIdentifiableData);

Hmm. It's unfortunate that we lose scoping here. If something said PCM, it would be much easier to understand what this parameter is for. I was hoping PrivateClickMeasurement::CarriesUserIdentifiableData would do it but it doesn't help on the call site. Ideas? Maybe carriesUserIdentifiableClickData or carriesUserIdentifiablePCMData?

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:166
> +    loadParameters.carriesUserIdentifiableData = WebCore::PrivateClickMeasurement::CarriesUserIdentifiableData::Yes;

It's the opposite. :) This load does not carry user identifying data.

[Kate Cheney]

Comment on attachment 418694 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=418694&action=review

I'll make these changes, thanks John!

>> Source/WebCore/ChangeLog:3
>> +        PCM: Indicate if load data is user identifiable
> 
> Maybe add "network load"

Will fix.

>> Source/WebKit/NetworkProcess/NetworkDataTask.cpp:57
>> +    return NetworkDataTaskCocoa::create(session, client, parameters.request, parameters.webFrameID, parameters.webPageID, parameters.storedCredentialsPolicy, parameters.contentSniffingPolicy, parameters.contentEncodingSniffingPolicy, parameters.shouldClearReferrerOnHTTPSToHTTPRedirect, parameters.shouldPreconnectOnly, parameters.isMainFrameNavigation, parameters.isMainResourceNavigationForAnyFrame, parameters.networkActivityTracker, parameters.isNavigatingToAppBoundDomain, parameters.shouldRelaxThirdPartyCookieBlocking, parameters.carriesUserIdentifiableData);
> 
> Hmm. It's unfortunate that we lose scoping here. If something said PCM, it would be much easier to understand what this parameter is for. I was hoping PrivateClickMeasurement::CarriesUserIdentifiableData would do it but it doesn't help on the call site. Ideas? Maybe carriesUserIdentifiableClickData or carriesUserIdentifiablePCMData?

Yes I suppose this is still vague. I'm leaning toward carriesUserIdentifiablePCMData because I could see 'clickData' being confused with things unrelated to PCM.

>> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:166
>> +    loadParameters.carriesUserIdentifiableData = WebCore::PrivateClickMeasurement::CarriesUserIdentifiableData::Yes;
> 
> It's the opposite. :) This load does not carry user identifying data.

Oops, I was thinking backwards after our conversation on naming -- good catch.

[Kate Cheney]

Created attachment 418912 [details]
Patch

[John Wilander]

Comment on attachment 418912 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=418912&action=review

> Source/WebKit/NetworkProcess/NetworkDataTask.cpp:57
> +    return NetworkDataTaskCocoa::create(session, client, parameters.request, parameters.webFrameID, parameters.webPageID, parameters.storedCredentialsPolicy, parameters.contentSniffingPolicy, parameters.contentEncodingSniffingPolicy, parameters.shouldClearReferrerOnHTTPSToHTTPRedirect, parameters.shouldPreconnectOnly, parameters.isMainFrameNavigation, parameters.isMainResourceNavigationForAnyFrame, parameters.networkActivityTracker, parameters.isNavigatingToAppBoundDomain, parameters.shouldRelaxThirdPartyCookieBlocking, parameters.carriesUserIdentifiablePCMData);

Good choice.

[Kate Cheney]

Created attachment 418940 [details]
Patch

[Kate Cheney]

(In reply to katherine_cheney from comment #7)
> Created attachment 418940 [details]
> Patch

Name change based on offline conversation.

[John Wilander]

Comment on attachment 418940 [details]
Patch

LGTM. I don't know why cq has a minus.

[Kate Cheney]

Created attachment 419055 [details]
Patch for landing

[EWS]

Committed r272239: <https://trac.webkit.org/changeset/272239>

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