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.

Sign up for GitHub

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

Jump to bottom

Link attributes naming #56

Closed
johnwilander opened this issue Nov 13, 2020 · 5 comments
Closed

Link attributes naming #56

johnwilander opened this issue Nov 13, 2020 · 5 comments
Assignees
@johnwilander
Labels
layering Layering additional data and functionality on top of PCM

Comments

@johnwilander
Copy link
Collaborator

In #30 we're getting close to final naming for the fields in the JSON attribution report. We should align the link attribution names with that. However, we may need some name spacing or context there. In the case of the JSON report, context is provided but the .well-known URL path but link attributes are stand-alone.

adCampaignID –> attributionContentID?
adDestination –> attributionDestination?

What do you think, @csharrison?
@johnwilander johnwilander self-assigned this Nov 13, 2020
@johnwilander johnwilander added the layering Layering additional data and functionality on top of PCM label Nov 13, 2020
@johnwilander johnwilander changed the title Link attribution names Link attributes naming Nov 13, 2020
@csharrison
Copy link

cc @johnivdel @maudnals

Namespacing to "attribution" sounds good to me to make it clear this is the API about attribution. We were thinking of renaming our proposal to "Attribution Reporting API" which makes "attribution" a good sign-posting prefix.

One nit: Attribute names should not be camel-case. They should be lower-case concatenated. See naming principles from TAG.

@johnivdel
Copy link

In #30 the proposed field for the site attribution triggers on is "attributed-on-site", which is the same value as attributionDestination. It would be nice to coalesce these.

In this issue we proposed "attributeon" for the link attribute which would match the JSON field.

For "attributionContentID", the field is "source-id", so maybe "attributionsourceid"? (or "attributesourcedata" % the discussion in #30).

@johnwilander
Copy link
Collaborator Author

cc @johnivdel @maudnals

Namespacing to "attribution" sounds good to me to make it clear this is the API about attribution. We were thinking of renaming our proposal to "Attribution Reporting API" which makes "attribution" a good sign-posting prefix.

👍🏼

One nit: Attribute names should not be camel-case. They should be lower-case concatenated. See naming principles from TAG.

Yeah, there was some back and forth early on but I do know my IDL additions are all lower case.

In #30 the proposed field for the site attribution triggers on is "attributed-on-site", which is the same value as attributionDestination. It would be nice to coalesce these.

In this issue we proposed "attributeon" for the link attribute which would match the JSON field.

Just to make sure, this is "attribute on" concatenated because it's not been attributed yet? If so, sounds good.

For "attributionContentID", the field is "source-id", so maybe "attributionsourceid"? (or "attributesourcedata" % the discussion in #30).

🤦🏻‍♂️ I went too quickly here. My earlier JSON proposal was for content ID but you're right. attributionsourceid it is.

@johnivdel
Copy link

Just to make sure, this is "attribute on" concatenated because it's not been attributed yet? If so, sounds good.

Yes, that was our thinking as well.

bertogg pushed a commit to Igalia/webkit that referenced this issue Nov 18, 2020
PCM: Change attribute and JSON key names according to the W3C convers…
f1f3ff5 
…ation

https://bugs.webkit.org/show_bug.cgi?id=218967

Reviewed by Brent Fulgham.

We've discussed extensively with Google and others on naming for the link
attributes and JSON key names in these issues:
privacycg/private-click-measurement#30
privacycg/private-click-measurement#56

This patch changes PCM accordingly.

Source/WebCore:

No new tests. Exiting tests updated.

* html/HTMLAnchorElement.cpp:
(WebCore::HTMLAnchorElement::parsePrivateClickMeasurement const):
* html/HTMLAnchorElement.idl:
* html/HTMLAttributeNames.in:
* loader/PrivateClickMeasurement.cpp:
(WebCore::PrivateClickMeasurement::json const):

Tools:

* TestWebKitAPI/Tests/WebCore/PrivateClickMeasurement.cpp:
(TestWebKitAPI::TEST):

LayoutTests:

* http/tests/contentextensions/block-private-click-measurement.html:
* http/tests/privateClickMeasurement/anchor-tag-attributes-reflect-expected.txt:
* http/tests/privateClickMeasurement/anchor-tag-attributes-reflect.html:
* http/tests/privateClickMeasurement/anchor-tag-attributes-validation-expected.txt:
* http/tests/privateClickMeasurement/anchor-tag-attributes-validation.html:
* http/tests/privateClickMeasurement/attribution-conversion-through-cross-site-image-redirect.html:
* http/tests/privateClickMeasurement/attribution-conversion-through-image-redirect-in-new-window.html:
* http/tests/privateClickMeasurement/attribution-conversion-through-image-redirect-with-priority.html:
* http/tests/privateClickMeasurement/attribution-conversion-through-image-redirect-without-priority.html:
* http/tests/privateClickMeasurement/clear-through-website-data-removal.html:
* http/tests/privateClickMeasurement/conversion-disabled-in-ephemeral-session.html:
* http/tests/privateClickMeasurement/expired-attributions-removed.html:
* http/tests/privateClickMeasurement/second-attribution-converted-with-higher-priority.html:
* http/tests/privateClickMeasurement/second-attribution-converted-with-lower-priority.html:
* http/tests/privateClickMeasurement/second-conversion-with-higher-priority.html:
* http/tests/privateClickMeasurement/second-conversion-with-lower-priority.html:
* http/tests/privateClickMeasurement/send-attribution-conversion-request-expected.txt:
* http/tests/privateClickMeasurement/send-attribution-conversion-request.html:
* http/tests/privateClickMeasurement/store-disabled-in-ephemeral-session.html:
* http/tests/privateClickMeasurement/store-private-click-measurement.html:


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@269886 268f45cc-cd09-0410-ab3c-d52691b4dbfc
This was referenced Dec 11, 2020
Closed
Closed
ryanhaddad pushed a commit to WebKit/WebKit that referenced this issue Dec 22, 2020
@johnwilander
PCM: Change attribute and JSON key names according to the W3C convers…
1089deb 
…ation

https://bugs.webkit.org/show_bug.cgi?id=218967

Reviewed by Brent Fulgham.

We've discussed extensively with Google and others on naming for the link
attributes and JSON key names in these issues:
privacycg/private-click-measurement#30
privacycg/private-click-measurement#56

This patch changes PCM accordingly.

Source/WebCore:

No new tests. Exiting tests updated.

* html/HTMLAnchorElement.cpp:
(WebCore::HTMLAnchorElement::parsePrivateClickMeasurement const):
* html/HTMLAnchorElement.idl:
* html/HTMLAttributeNames.in:
* loader/PrivateClickMeasurement.cpp:
(WebCore::PrivateClickMeasurement::json const):

Tools:

* TestWebKitAPI/Tests/WebCore/PrivateClickMeasurement.cpp:
(TestWebKitAPI::TEST):

LayoutTests:

* http/tests/contentextensions/block-private-click-measurement.html:
* http/tests/privateClickMeasurement/anchor-tag-attributes-reflect-expected.txt:
* http/tests/privateClickMeasurement/anchor-tag-attributes-reflect.html:
* http/tests/privateClickMeasurement/anchor-tag-attributes-validation-expected.txt:
* http/tests/privateClickMeasurement/anchor-tag-attributes-validation.html:
* http/tests/privateClickMeasurement/attribution-conversion-through-cross-site-image-redirect.html:
* http/tests/privateClickMeasurement/attribution-conversion-through-image-redirect-in-new-window.html:
* http/tests/privateClickMeasurement/attribution-conversion-through-image-redirect-with-priority.html:
* http/tests/privateClickMeasurement/attribution-conversion-through-image-redirect-without-priority.html:
* http/tests/privateClickMeasurement/clear-through-website-data-removal.html:
* http/tests/privateClickMeasurement/conversion-disabled-in-ephemeral-session.html:
* http/tests/privateClickMeasurement/expired-attributions-removed.html:
* http/tests/privateClickMeasurement/second-attribution-converted-with-higher-priority.html:
* http/tests/privateClickMeasurement/second-attribution-converted-with-lower-priority.html:
* http/tests/privateClickMeasurement/second-conversion-with-higher-priority.html:
* http/tests/privateClickMeasurement/second-conversion-with-lower-priority.html:
* http/tests/privateClickMeasurement/send-attribution-conversion-request-expected.txt:
* http/tests/privateClickMeasurement/send-attribution-conversion-request.html:
* http/tests/privateClickMeasurement/store-disabled-in-ephemeral-session.html:
* http/tests/privateClickMeasurement/store-private-click-measurement.html:


Canonical link: https://commits.webkit.org/231639@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@269886 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@johnwilander johnwilander mentioned this issue Mar 9, 2021
Merged
@johnwilander
Copy link
Collaborator Author

This has now been updated in the spec: f7e51be

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@johnwilander johnwilander

Labels
layering Layering additional data and functionality on top of PCM
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@johnwilander @csharrison @johnivdel