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

Continue work on API #10

Merged
merged 3 commits into from
Sep 13, 2024
Merged

Continue work on API #10

merged 3 commits into from
Sep 13, 2024

Conversation

andyleiserson
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

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

Good. I've given you commit access, so fix as much as you like and land it when you are happy.

I'm looking at defining last-touch logic. I should have something soon.

api.bs Outdated Show resolved Hide resolved
api.bs Outdated Show resolved Hide resolved
api.bs Outdated Show resolved Hide resolved
api.bs Outdated Show resolved Hide resolved
api.bs Outdated Show resolved Hide resolved
api.bs Outdated Show resolved Hide resolved
api.bs Outdated
Comment on lines 336 to 343
<pre>
navigator.privateAttribution.saveImpression({
aggregator: "aggregator.example", // the name of the aggregation system
index: 3, // the histogram index for counting this impression
aggregator: "aggregator.example",
histogramIndex: 3,
ad: "sample-campaign-eijb", // a unique identifier for the ad placement
target: "advertiser.example", // the advertiser site where a conversion will occur
conversionSite: "advertiser.example", // the advertiser site where a conversion will occur
});
</pre>
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably wrap this in a <div class=example> and put some words in.

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 was considering getting rid of the examples entirely, given that the detailed descriptions of the parameters will include the information that is in the comments here. Do you think it's worth keeping them?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a fan of using examples to illustrate how things work. Maybe I can suggest removing them from here and then adding an examples section at the top of the API section, so that you can give a bit of an overview of how things fit together.

As for the rules... we're not doing an explainer so we should have that sort of explainy stuff in there somewhere.

api.bs Outdated
<xmp class=idl>
dictionary PrivateAttributionImpressionOptions {
required DOMString aggregator;
required unsigned long histogramIndex;
required DOMString ad;
required DOMString target;
required DOMString conversionSite;
unsigned long expiration;
Copy link
Collaborator

Choose a reason for hiding this comment

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

JavaScript uses long for dates as well. So this might be confused.

It might pay to call this keepFor or lifetime or duration or something like that so that it isn't mistaken for an absolute time.

api.bs Outdated
</dd>
<dt><dfn>expiration</dfn></dt>
<dd>
A "time to live" (in seconds) after which the [=impression=] can no longer
Copy link
Collaborator

Choose a reason for hiding this comment

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

JavaScript does times in milliseconds by default, but I think that even seconds is a bit silly for this. If our lookback is in days, what do you think about making this a duration in days as well?

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 think days makes sense. I got seconds from the google doc.

api.bs Outdated
Comment on lines 509 to 510
Expiration: The number of seconds an [=/impression=] remains eligible for attribution,
Expiration: either from the call to <a>saveImpression()</a>, or a [=/user agent=]-defined limit.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, that's how you do line continuations? Gross.

andyleiserson and others added 2 commits September 13, 2024 10:14
* Updates for not passing `aggregator` to `saveImpression`.
* Change `expiration` to `lifetimeDays`.
@andyleiserson andyleiserson merged commit 808f9d3 into patcg:main Sep 13, 2024
1 check passed
@andyleiserson andyleiserson deleted the api-2 branch September 13, 2024 17:28
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.

2 participants