Bug 223661 – PCM: Rename attributeOn to attributionDestination and change the IDL data type of attributionSourceId to unsigned long

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

[John Wilander]

The PR review of the update of the PCM spec lead WebKit, Mozilla, and Google to decide on two changes:
1. Rename attributeOn to attributionDestination
2. Change the IDL data type of attributionSourceId from DOMString to unsigned long

This change can be seen in this commit:
https://github.com/privacycg/private-click-measurement/pull/75/commits/8623b5df5e899e6652bc5b00313585f7eeafa797

We should make those changes.

[Radar WebKit Bug Importer]

<rdar://problem/75762075>

[John Wilander]

Created attachment 424094 [details]
Patch

[Kate Cheney]

Comment on attachment 424094 [details]
Patch

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

> Source/WebCore/html/HTMLAnchorElement.cpp:413
> +    auto hasAttributionSourceIDAttr = hasAttributeWithoutSynchronization(attributionsourceidAttr);

I know these names were already in here, but don't we typically avoid using abbreviations? Also, why split these out?

> Source/WebCore/loader/PrivateClickMeasurement.cpp:158
> +    reportDetails->setString("attributed_on_site"_s, m_destinationSite.registrableDomain.string());

Should this JSON string be changed to attribution_destination_site or something similar?

[John Wilander]

(In reply to katherine_cheney from comment #3)
> Comment on attachment 424094 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=424094&action=review
> 
> > Source/WebCore/html/HTMLAnchorElement.cpp:413
> > +    auto hasAttributionSourceIDAttr = hasAttributeWithoutSynchronization(attributionsourceidAttr);
> 
> I know these names were already in here, but don't we typically avoid using
> abbreviations? Also, why split these out?

The developer-facing names have been decided in the W3C Privacy CG. I'm just reflecting them in variable names.

As for splitting, I'm using the booleans twice and didn't want to call hasAttributeWithoutSynchronization() four times.

> > Source/WebCore/loader/PrivateClickMeasurement.cpp:158
> > +    reportDetails->setString("attributed_on_site"_s, m_destinationSite.registrableDomain.string());
> 
> Should this JSON string be changed to attribution_destination_site or
> something similar?

I thought about that. At minimum, we should bring it up with Google to see what they think. Not for this patch though.

[Brent Fulgham]

Comment on attachment 424094 [details]
Patch

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

r=me

> LayoutTests/http/tests/privateClickMeasurement/anchor-tag-attributes-validation-expected.txt:8
> +CONSOLE MESSAGE: Both attributionsourceid and attributiondestination need to be set for Private Click Measurement to work.

It's interesting that your renaming change seems to have affected the number and order of these messages. Is that expected?
Update: I guess this is a by-product of changing from string to number, and removing some test cases below.

[John Wilander]

(In reply to Brent Fulgham from comment #5)
> Comment on attachment 424094 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=424094&action=review
> 
> r=me
> 
> > LayoutTests/http/tests/privateClickMeasurement/anchor-tag-attributes-validation-expected.txt:8
> > +CONSOLE MESSAGE: Both attributionsourceid and attributiondestination need to be set for Private Click Measurement to work.
> 
> It's interesting that your renaming change seems to have affected the number
> and order of these messages. Is that expected?
> Update: I guess this is a by-product of changing from string to number, and
> removing some test cases below.

Yeah, what I did was to work through these one by one and remove the once that just duplicated results. I made sure all the remaining cases matter.

Thanks for the review, Brent!

[John Wilander]

Comment on attachment 424094 [details]
Patch

Let's see if this patch still applies or needs to be rebased.

[EWS]

Committed r275046: <https://commits.webkit.org/r275046>

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