Explainer and spec update for padding #186

xyaoinum · 2023-05-26T20:14:14Z

Update the header format, and include the padding algorithm, per discussion in Sec-Browsing-Topics header format seems a bit awkward #183.
Reorder the comparator conditions of BrowsingTopic (i.e. "version" first, and then "topic id"). This aligns with the current implementation.

xyaoinum · 2023-05-26T20:20:36Z

@domenic PTAL. Thanks! (Thanks for filing the issues giving suggestions to the spec. I will address them later, but this patch is a behavior change and we'd like to add it to the spec first)

@jkarlin PTAL. Thanks!

domenic

This seems closely related to my open issue of #183. Although I am glad you are changing the format to be a bit less problematic, I think the core issue remains: the desire to not be explicit about the version association.

Have you considered just always adding the version, to every topic? That makes parsing much easier, and if I understand this correctly, it also would make the padding less necessary. (Although still necessary in some cases.)

domenic · 2023-05-30T09:15:10Z

spec.bs

 <h2 id="topics-for-caller-header">Topics for caller</h2>
 <div algorithm>
- To <dfn>calculate the topics for caller</dfn>, given a [=topics caller context=] |callerContext|, perform the following steps. They return a list of {{BrowsingTopic}}s.
+ To <dfn>calculate the topics for caller</dfn>, given a [=topics caller context=] |callerContext|, perform the following steps. They return a list of {{BrowsingTopic}}s


Nit: the period was good, keep the period :)

domenic · 2023-05-30T09:17:57Z

spec.bs

+ 1. Insert |topicItem| to |topicsInnerList|.
+ 1. Let |maxNumberOfEpochs| be 3 (i.e. topics are selected from the last 3 epochs).
+ 1. Let |topicMaxLength| be 3 (i.e. [=browsing topics types/topic id=] has maximum 3 digits).
+ 1. Let |versionMaxLength| be 13 (i.e. chrome.1:1:11).


This "i.e." isn't clarifying to me. Why is 13 related to chrome.1:1:11? What does this mean for user agents with long names, like, say, internetexplorer?

Clarified -- switched to rely on the current [=browsing topics types/version=].

domenic · 2023-05-30T09:18:43Z

spec.bs

+ 1. Let |innerListItemsSeparatorLength| be 1 (i.e. structured fields use a single character (' ') to separate inner list items).
+ 1. Let |maxPaddingLength| be <code>|maxNumberOfEpochs| * |topicMaxLength| + |numVersionsInEpochs| * (|versionMaxLength| + |paramsSeparatorLength| + |paramKeyLength|) + (|maxNumberOfEpochs| - 1) * |innerListItemsSeparatorLength|</code>.
+
+ Note: the numbers used in the step above are specific to one of Chrome's implementation. Implementers should use numbers compatible with their configuration.


Let's write the spec to fit any user agent. Putting on my hat as another implementer, it's not clear which of these steps are applicable to me and which aren't.

Done. Some are switched to use the configuration params, some are intended to be fixed constant.

domenic · 2023-05-30T09:19:44Z

spec.bs

+ <div class="example">
+ One returned topics, and underlying epochs have two different versions:
+
+ <code>t=(1;v=chrome.1:1:2 2), p=P0000000000000000000000000</code>


I only see one version here?

Done. (Removed. As example are less applicable given we'd always expose all versions.)

Always send all versions

xyaoinum · 2023-05-30T15:55:52Z

@domenic I switched to always expose the version per your suggestion (although still would like to see Josh's position), and addressed your other comments. PTAL again. Thanks!

domenic

I appreciate the move to the simpler format. We can continue discussing other ideas in #183.

Current significant blocker is that tokens cannot contain :.

domenic · 2023-05-31T05:25:24Z

spec.bs

 1. For each |topic| in |topics|:
+ 1. Let |digitsInTopic| be the number of digits in |topic|["{{BrowsingTopic/topic}}"].


Suggested change

1. Let |digitsInTopic| be the number of digits in |topic|["{{BrowsingTopic/topic}}"].

1. Let |digitsInTopic| be the number of base-10 digits in |topic|["{{BrowsingTopic/topic}}"].

just for extra clarity

domenic · 2023-05-31T05:28:32Z

README.md

@@ -71,9 +71,9 @@ The topics will be inferred by the browser. The browser will leverage a classifi
 * The request header will not modify state for the caller unless there is a corresponding response header. That is, the topic of the page won't be considered observed, nor will it affect the user's topic calculation for the next epoch. 
 * The response header will only be honored if the corresponding request included the topics header (or would have included the header if it wasn't empty).
 * The registrable domain used for topic observation is that of the url of the request.
- * Example request header: `Sec-Browsing-Topics: 123;v=chrome.1:1:2, 2`
+ * Example request header: `Sec-Browsing-Topics: t=(123;v=chrome.1:1:2 2;v=chrome.1:1:2), p=P000000000000000000000000`


So, I realized that : is not allowed in structured field tokens. Probably you need to change them to be strings instead.

I think the spec does allow ":" and "."? https://www.rfc-editor.org/rfc/rfc8941.html#section-3.3.4

Sorry for missing this; you're right!

domenic · 2023-05-31T05:29:16Z

spec.bs

+ 1. Let |topicMaxLength| be number of digits in the maximum [=browsing topics types/topic id=] (e.g. for <a href="https://github.com/patcg-individual-drafts/topics/blob/main/taxonomy_v1.md">Chrome's initial taxonomy</a>, |topicMaxLength| is 3, as the [=browsing topics types/topic id=] has maximum 3 digits).
+ 1. Let |versionMaxLength| be the length of the current [=browsing topics types/version=].
+ 1. Let |innerListItemsSeparatorLength| be 1 (i.e. structured fields use a single character (' ') to separate inner list items).
+ 1. Let |maxPaddingLength| be <code>|maxNumberOfEpochs| * (|topicMaxLength| + |versionMaxLength| + |paramsSeparatorLength| + |paramKeyLength|) + (|maxNumberOfEpochs| - 1) * |innerListItemsSeparatorLength|</code>.


Suggested change

1. Let |maxPaddingLength| be <code>|maxNumberOfEpochs| * (|topicMaxLength| + |versionMaxLength| + |paramsSeparatorLength| + |paramKeyLength|) + (|maxNumberOfEpochs| - 1) * |innerListItemsSeparatorLength|</code>.

1. Let |maxPaddingLength| be |maxNumberOfEpochs| * (|topicMaxLength| + |versionMaxLength| + |paramsSeparatorLength| + |paramKeyLength|) + (|maxNumberOfEpochs| - 1) * |innerListItemsSeparatorLength|.

domenic · 2023-05-31T05:34:44Z

spec.bs

+ 1. Insert |topicItem| to |topicsInnerList|.
+ 1. Let |maxNumberOfEpochs| be 3 (i.e. topics are selected from the last 3 epochs).
+ 1. Let |topicMaxLength| be number of digits in the maximum [=browsing topics types/topic id=] (e.g. for <a href="https://github.com/patcg-individual-drafts/topics/blob/main/taxonomy_v1.md">Chrome's initial taxonomy</a>, |topicMaxLength| is 3, as the [=browsing topics types/topic id=] has maximum 3 digits).
+ 1. Let |versionMaxLength| be the length of the current [=browsing topics types/version=].


What is the "current" one? [=browsing topics types/version=] is only defined given the three inputs configurationVersion, taxonomyVersion, and modelVersion.

Probably the best solution here is to move this definition to https://patcg-individual-drafts.github.io/topics/#terminology-and-types-header to make it clear you're trying to figure out a user-agent-wide maximum. E.g. something like

The maximum version string length is the maximum possible string length of a [=browsing topics types/version=] that a user agent could possibly generate in a given browsing session.

(or maybe in a given "software release"??)

and then adding an example would be even nicer.

domenic · 2023-05-31T05:37:01Z

spec.bs

 1. [=Set a structured field value=] given ([:Sec-Browsing-Topics:], |headerStructuredFields|) in |request|'s [=request/header list=].

- Note: In Chrome's experimentation phase, it will additionally require a valid <a href="https://github.com/GoogleChrome/OriginTrials/blob/gh-pages/explainer.md">Origin Trial</a> token to exist in |initiatorWindow|'s <a data-cite="!HTML#concept-document-window">associated document</a> for the request to be eligible for topics.
+ <div class="note">


Possible idea for spec-writing, feel free to disregard: you could try to generate the padding length by first generating an artificial maximum-length header value, then generating a first draft of your real header value, and then subtracting their serialized lengths. It's inefficient, but it's perhaps easier to understand for the reader, and the implementation could do the more efficient thing.

(You could maybe even generate the artificial maximum-length header value ahead of time, in the terminology section or something?)

I incorporated your first suggestion. (I kept the generation of maximum-length inline as I feel it'd give more context here.)

domenic

LGTM with nits and one suggestion for clarity improvement, which you can either do via another review pass or via a followup PR (since I imagine it might be important to get the normative changes settled quickly).

I think the current algorithm is efficient but hard to read. The reuse of currentTopicsInnerList, and the way it relies on the topics that come back being sorted, makes it quite stateful. I'd suggest an easier to read two-pass algorithm like so:

Let versionsToTopics be an ordered map.
For each topic of topics:
1. Let version be topic["version"].
2. Let topicInteger be topic["topic"].
3. If versionsToTopics[version] does not exist, set it to an empty list.
4. Append topicInteger to versionsToTopics[version].
Let topicsStructuredFieldsList be an empty Structured Fields List.
For each version → topicIntegers of versionsToTopics:
1. Let innerList be an empty Structured Fields Inner List.
2. Append all items from topicIntegers to innerList.
3. Let topicParameters be an empty Structured Fields Parameters.
4. Set topicParameters["v"] to a Structured Field Token having value version.
5. Associated topicParamters with innerList.
6. Append innerList to topicsStructuredFieldsList.

spec.bs

xyaoinum · 2023-06-01T05:09:31Z

Thanks @domenic! Landing this now. I will create followup PR for the clarity improvement and other feedback if any.

SHA: 9d8fec1 Reason: push, by xyaoinum Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

SHA: 9d8fec1 Reason: push, by pull[bot] Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Addresses the following issues: - patcg-individual-drafts#176 - patcg-individual-drafts#177 - patcg-individual-drafts#178 - patcg-individual-drafts#179 - patcg-individual-drafts#180 - patcg-individual-drafts#181 - patcg-individual-drafts#182 - patcg-individual-drafts#183 (already addressed the main issue, but there was one unresolved PR comment in patcg-individual-drafts#186 (review))

xyaoinum added 3 commits May 26, 2023 15:54

Update spec.bs to include padding

0e01114

Update README.md for mention padding

d12a0bf

minor

74b548c

xyaoinum requested a review from jkarlin May 26, 2023 20:20

domenic reviewed May 30, 2023

View reviewed changes

xyaoinum added 2 commits May 30, 2023 11:47

Merge branch 'patcg-individual-drafts:main' into main

b75238a

Address spec feedback

d50e952

Always send all versions

xyaoinum added 3 commits May 30, 2023 11:59

Update spec.bs

ba081a9

Update README.md

1e26caf

minor

f72cfa4

domenic reviewed May 31, 2023

View reviewed changes

domenic mentioned this pull request May 31, 2023

Sec-Browsing-Topics header format seems a bit awkward #183

Closed

xyaoinum added 3 commits June 1, 2023 00:04

Merge branch 'patcg-individual-drafts:main' into main

c78ef5a

Update padding and address spec comments

ed1fae2

Update padding

614bd48

xyaoinum requested a review from domenic June 1, 2023 04:15

domenic approved these changes Jun 1, 2023

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

spec.bs Outdated Show resolved Hide resolved

spec.bs Outdated Show resolved Hide resolved

spec.bs Outdated Show resolved Hide resolved

spec.bs Outdated Show resolved Hide resolved

spec.bs Outdated Show resolved Hide resolved

xyaoinum added 2 commits June 1, 2023 01:03

Merge branch 'patcg-individual-drafts:main' into main

dd940da

Address comments

c83af89

xyaoinum merged commit 9d8fec1 into patcg-individual-drafts:main Jun 1, 2023

github-actions bot added a commit that referenced this pull request Jun 1, 2023

Merge pull request #186 from xyaoinum/main

6a28b21

SHA: 9d8fec1 Reason: push, by xyaoinum Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

xyaoinum mentioned this pull request Jun 6, 2023

Improve spec.bs #188

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Explainer and spec update for padding #186

Explainer and spec update for padding #186

xyaoinum commented May 26, 2023 •

edited

xyaoinum commented May 26, 2023

domenic left a comment

domenic May 30, 2023

xyaoinum May 30, 2023

domenic May 30, 2023

xyaoinum May 30, 2023

domenic May 30, 2023

xyaoinum May 30, 2023

domenic May 30, 2023

xyaoinum May 30, 2023

xyaoinum commented May 30, 2023

domenic left a comment

domenic May 31, 2023

xyaoinum Jun 1, 2023

domenic May 31, 2023

xyaoinum Jun 1, 2023

domenic Jun 1, 2023

domenic May 31, 2023

xyaoinum Jun 1, 2023

domenic May 31, 2023

xyaoinum Jun 1, 2023

domenic May 31, 2023

xyaoinum Jun 1, 2023

domenic left a comment

xyaoinum commented Jun 1, 2023

		1. For each \|topic\| in \|topics\|:
		1. Let \|digitsInTopic\| be the number of digits in \|topic\|["{{BrowsingTopic/topic}}"].

	1. Let \|maxPaddingLength\| be <code>\|maxNumberOfEpochs\| * (\|topicMaxLength\| + \|versionMaxLength\| + \|paramsSeparatorLength\| + \|paramKeyLength\|) + (\|maxNumberOfEpochs\| - 1) * \|innerListItemsSeparatorLength\|</code>.
	1. Let \|maxPaddingLength\| be \|maxNumberOfEpochs\| * (\|topicMaxLength\| + \|versionMaxLength\| + \|paramsSeparatorLength\| + \|paramKeyLength\|) + (\|maxNumberOfEpochs\| - 1) * \|innerListItemsSeparatorLength\|.

Explainer and spec update for padding #186

Explainer and spec update for padding #186

Conversation

xyaoinum commented May 26, 2023 • edited

xyaoinum commented May 26, 2023

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xyaoinum commented May 30, 2023

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic left a comment

Choose a reason for hiding this comment

xyaoinum commented Jun 1, 2023

xyaoinum commented May 26, 2023 •

edited