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.
<rdar://problem/75762075>
Created attachment 424094 [details] Patch
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?
(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.
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.
(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!
Comment on attachment 424094 [details] Patch Let's see if this patch still applies or needs to be rebased.
Committed r275046: <https://commits.webkit.org/r275046> All reviewed patches have been landed. Closing bug and clearing flags on attachment 424094 [details].