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

Spec reserved.top_navigation_start and reserved.top_navigation_commit #130

Merged
merged 13 commits into from Nov 7, 2023

Conversation

blu25
Copy link
Collaborator

@blu25 blu25 commented Oct 23, 2023

This PR does a few things to allow the new automatic beacon types:

  • Creates a new "valid automatic beacon event types" list that contains the start and commit beacon types.
  • Creates a new implicit enumerated type for the automatic beacon event types. This contains the start and commit beacon types.
  • Creates a new "automatic beacon event" to be used for automatic beacons instead of "destination enum event", which contains the aforementioned implicit enumerated event type.
  • Splits out automatic beacon data into its own struct.
  • Turns the fenced frame reporter's automatic beacon data into a mapping to match the implementation.
  • Passes "reserved.top_navigation_commit" as a parameter when calling the automatic beacon algorithm from its existing place at navigation commit.
  • Also calls the automatic beacon algorithm at navigation start passing in "reserved.top_navigation_start".
  • Removes "reserved.top_navigation".

Preview | Diff

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
@michal-kalisz
Copy link

This change appears to be very valuable. However, it breaks backward compatibility (discontinues support for the automatic beacon event type: reserved.top_navigation). When the change will be merged and available in chrome, some users will have an older version of the browser (requiring reserved.top_navigation), and some will have a newer one (supporting reserved.top_navigation_(start|commit)).

Tracking clicks in our current tests is a critical component. Therefore, we are concerned that the proposed change should not affect either our tests or our partners' (SSP) tests.

How can we prepare for this? Would registering 3 automatic beacons: reserved.top_navigation, reserved.top_navigation_start, reserved.top_navigation_commit and then calling window.fence.setReportEventDataForAutomaticBeacons for 3 different event type in the ad be a solution?

Something like:
In report win / report result:

function reportWin() {
    registerAdBeacon({
        "reserved.top_navigation": "URL_for_commit",
        "reserved.top_navigation_start": "URL_for_start",
        "reserved.top_navigation_commit": "URL_for_commit"
    })
}

In ad:

window.fence.setReportEventDataForAutomaticBeacons({
    eventType: 'reserved.top_navigation',
    destination: ['buyer', 'direct-seller']
});
window.fence.setReportEventDataForAutomaticBeacons({
    eventType: 'reserved.top_navigation_start',
    destination: ['buyer', 'direct-seller']
});
window.fence.setReportEventDataForAutomaticBeacons({
    eventType: 'reserved.top_navigation_commit',
    destination: ['buyer', 'direct-seller']
});

Or do you suggest something else?

@blu25
Copy link
Collaborator Author

blu25 commented Nov 1, 2023

To clarify, reserved.top_navigation will continue to work in new versions of Chrome for the time being.

Our current plan is to remove reserved.top_navigation functionality at some point in the future once enough partners have switched over to reserved.top_navigation_commit. The initial version of this spec patch reflects what we want the eventual end-state to be. However, I can't commit to any timeline for actually removing reserved.top_navigation. In the meantime, and to avoid confusion about what is being supported on the Chrome side, I'll add it back to the spec with an advisement that it will be deprecated.

In terms of preparing for this change, what you described would be the preferred way of handling this. Once enough people are on the newer version of Chrome, you can remove the reserved.top_navigation part of your code.

@dmdabbs
Copy link

dmdabbs commented Nov 1, 2023

I also notice that this PR updates only the spec. Will explainers get updated as well?

@blu25
Copy link
Collaborator Author

blu25 commented Nov 1, 2023

Yes. That's in a separate repository, and I have a PR up for that as well.

Copy link
Collaborator

@domfarolino domfarolino left a comment

Choose a reason for hiding this comment

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

LGTM % nit

spec.bs Outdated Show resolved Hide resolved
@domfarolino domfarolino merged commit 161d00e into master Nov 7, 2023
2 checks passed
@domfarolino domfarolino deleted the liam-reserved-top-start-commit branch November 7, 2023 22:57
github-actions bot added a commit that referenced this pull request Nov 7, 2023
…#130)

SHA: 161d00e
Reason: push, by domfarolino

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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

4 participants