Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update naming and terminology according to the layering effort with Google's Conversion Measurement API #75

Merged
merged 3 commits into from
Mar 18, 2021

Conversation

johnwilander
Copy link
Collaborator

No description provided.

@johnwilander johnwilander requested a review from hober March 9, 2021 01:51
@johnwilander johnwilander changed the title For review. Update naming and terminology according to the layering effort with Google's Conversion Measurement API Mar 9, 2021
private-click-measurement.bs Show resolved Hide resolved
private-click-measurement.bs Outdated Show resolved Hide resolved

<xmp class="highlight" highlight=html>
<a adcampaignid="17" addestination="https://destination.example/">
<a sourceid="17" attributeon="https://destination.example/">
Copy link
Member

@hober hober Mar 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some thoughts on these new names:

  • I liked that the previous ones had a common prefix, so you could tell they were related.
  • sourceid, in particular, seems weird if you don't know anything about PCM. It would be fair for someone to wonder if/assume it's the ID of this element in some source document, which for some reason got moved into this attribute from id="" by some tool.
  • Similarly, attributeon sounds like it's about HTML attributes.

I'd like @annevk's or @domenic's thoughts on these new attribute names.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My mistake on sourceid. It should be attributionsourceid. That's what's implemented in WebKit.

attributeon has been cringe-worthy all along but the Google folks wanted it to be "attribute-on" so it's called "the attribute-on website":
WICG/attribution-reporting-api#57 (comment)
Referenced in the PCM issue on naming:
#56

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd be happy to re-engage on those issues if a different naming is more appropriate. My apologies if it came across as though our mind was made up on the matter, that wasn't the intention.

Given the benefits of having a common prefix raised above and using "source" for the click side, attributiondestination may be a better fit (which was the original name on #56). "attributionsite" is another name that solves the above issues and mixes well with prose.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing attributeon would likely be a breaking change in WebKit. We've been in beta for five weeks with attributeon.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like attributiondestination much better than attributeon. Would Google Chrome be willing to change to that?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we're supportive of the change. I will file a PR against the Conversion Measurement API repo to use "attributiondestination".

cc @csharrison @maudnals

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I think you addressed both of my concerns in 8623b5d. Thanks!

Copy link

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think attributiondestination would have been clearer. In particular the word "attribute" already has quite a few meanings and "on" is associated with event handlers. Not sure it's a deal-breaker though.

</xmp>

Formally:

<pre class="idl">
partial interface HTMLAnchorElement {
[CEReactions] attribute DOMString adCampaignId;
[CEReactions] attribute USVString adDestination;
[CEReactions] attribute DOMString attributionSourceId;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this not an integer?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most IDs can contain (almost) arbitrary characters; I assume that's the case here?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yeah, then this should definitely be an integer (probably unsigned long to take best advantage of HTML's reflection abilities).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can’t find where but the reason I believe was layering. Google wants to be able to have far more entropy and might want to use more characters than digits. There was also talk of hierarchical IDs such as “low entropy value:high entropy value” that can either allow developers to have one markup that supports both or allow the browser to use one of the two or both depending on user settings or some privacy measurement.

WebKit has had this as an experimental feature since May 2019 and is five weeks into beta for official release. Changing it will likely be a breaking change for us.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the Attribution Reporting Explainer, we use an unsigned long for the sourceeventid ( https://github.com/WICG/conversion-measurement-api#registering-attribution-sources-for-anchor-tag-navigations).

(this is an intentionally different attribute that attributionsourceid since it is higher entropy per discussions on #56 )

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like unsigned long then. Hopefully, we won't have too many who've adopted PCM in WebKit browsers by the time we can get this change out.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johnwilander for most uses of this API it might not even be a breaking change as JavaScript is pretty lenient.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like 8623b5d addresses this concern, yeah?

@domenic
Copy link

domenic commented Mar 10, 2021

I don't have any particular insights to add here. I did take part in some discussions with @csharrison about https://github.com/WICG/conversion-measurement-api and I recall there being a variety of complex tradeoffs in the naming, e.g. purposefully choosing an "attribute" prefix for "attributeon" that is different from the "attribution" prefix for the others.

…'s type to unsigned long in alignment with Tess's, Anne's, Domenic's, and John's comments.
@johnwilander johnwilander requested a review from hober March 17, 2021 16:17
Copy link
Member

@hober hober left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me with the changes in 8623b5d.

@johnwilander
Copy link
Collaborator Author

Thanks, all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants