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

Allow all HTML attributionsrc surfaces to make multiple background requests #744

Merged
merged 2 commits into from
Apr 3, 2023

Conversation

apasel422
Copy link
Collaborator

@apasel422 apasel422 commented Mar 29, 2023

We use spaces due to the precedent from <a ping>. We will determine the syntax for window.open separately.

Fixes #718

@apasel422 apasel422 marked this pull request as ready for review March 29, 2023 22:07
EVENT.md Show resolved Hide resolved
Copy link
Collaborator

@johnivdel johnivdel left a comment

Choose a reason for hiding this comment

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

LGTM

@apasel422 apasel422 merged commit 744d645 into WICG:main Apr 3, 2023
@apasel422 apasel422 deleted the multi branch April 3, 2023 18:20
aarongable pushed a commit to chromium/chromium that referenced this pull request Apr 20, 2023
WICG/attribution-reporting-api#744
WICG/attribution-reporting-api#752

The attributionsrc attribute on <a>, <img>, and <script> now supports
a space-separated list of URLs to which to make background requests in
the same contexts in which a single such request was previously
supported. The strings are resolved to full URLs as previously: relative
URLs are supported.

Likewise, the attributionsrc window.open feature can be specified
multiple times.

URLs that are not allowed to register are logged to DevTools but
otherwise ignored, and do not affect the valid URLs in the list, if any.

Registrations from multiple requests are multiplexed onto a single
AttributionDataHost Mojo remote because the registration order *between*
requests is unspecified. This reduces resource consumption in both the
renderer and the browser and makes it unnecessary to associate a single
AttributionSrcToken with multiple data hosts, which would require
significant browser-side refactoring.

As a result of the multiplexing change, we move the
Conversions.Registered(Sources|Triggers)PerDataHost metrics to the
renderer, where it is easier to track per redirect chain, and combine
them into a single metric for both types of registrations.

Bug: 1430194, 1430496, 1434306
Change-Id: Id5dbc42a20a78394560f11aa7fc57bfa2d92ae78
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4393405
Reviewed-by: Nate Chapin <japhet@chromium.org>
Quick-Run: Andrew Paseltiner <apaseltiner@chromium.org>
Reviewed-by: Nan Lin <linnan@chromium.org>
Commit-Queue: Andrew Paseltiner <apaseltiner@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1133249}
aarongable pushed a commit to chromium/chromium that referenced this pull request Apr 20, 2023
This reverts commit a8cbb3c.

Reason for revert: Failures of AttributionSrcBrowserTest.AttributionSrcAnchor_MultipleBackgroundRequests on multiple fuchsia builders:
https://ci.chromium.org/ui/p/chromium/builders/ci/fuchsia-arm64-rel/5723/overview
https://ci.chromium.org/ui/p/chromium/builders/ci/fuchsia-x64-rel/5285/overview

Original change's description:
> Support multiple background attributionsrc requests
>
> WICG/attribution-reporting-api#744
> WICG/attribution-reporting-api#752
>
> The attributionsrc attribute on <a>, <img>, and <script> now supports
> a space-separated list of URLs to which to make background requests in
> the same contexts in which a single such request was previously
> supported. The strings are resolved to full URLs as previously: relative
> URLs are supported.
>
> Likewise, the attributionsrc window.open feature can be specified
> multiple times.
>
> URLs that are not allowed to register are logged to DevTools but
> otherwise ignored, and do not affect the valid URLs in the list, if any.
>
> Registrations from multiple requests are multiplexed onto a single
> AttributionDataHost Mojo remote because the registration order *between*
> requests is unspecified. This reduces resource consumption in both the
> renderer and the browser and makes it unnecessary to associate a single
> AttributionSrcToken with multiple data hosts, which would require
> significant browser-side refactoring.
>
> As a result of the multiplexing change, we move the
> Conversions.Registered(Sources|Triggers)PerDataHost metrics to the
> renderer, where it is easier to track per redirect chain, and combine
> them into a single metric for both types of registrations.
>
> Bug: 1430194, 1430496, 1434306
> Change-Id: Id5dbc42a20a78394560f11aa7fc57bfa2d92ae78
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4393405
> Reviewed-by: Nate Chapin <japhet@chromium.org>
> Quick-Run: Andrew Paseltiner <apaseltiner@chromium.org>
> Reviewed-by: Nan Lin <linnan@chromium.org>
> Commit-Queue: Andrew Paseltiner <apaseltiner@chromium.org>
> Reviewed-by: Charlie Harrison <csharrison@chromium.org>
> Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
> Cr-Commit-Position: refs/heads/main@{#1133249}

Bug: 1430194, 1430496, 1434306
Change-Id: Ief97f3f08956f7ac215bc8e01ca1ac9e2c3a90b3
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4455256
Reviewed-by: Nate Chapin <japhet@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: David Baron <dbaron@chromium.org>
Owners-Override: David Baron <dbaron@chromium.org>
Auto-Submit: David Baron <dbaron@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1133306}
aarongable pushed a commit to chromium/chromium that referenced this pull request Apr 21, 2023
This is a reland of commit a8cbb3c.
We've actually encountered a similar bug before related to navigations
(http://crbug.com/1405318) so for now I've simply removed the flaky
test and will investigate separately.

Original change's description:
> Support multiple background attributionsrc requests
>
> WICG/attribution-reporting-api#744
> WICG/attribution-reporting-api#752
>
> The attributionsrc attribute on <a>, <img>, and <script> now supports
> a space-separated list of URLs to which to make background requests in
> the same contexts in which a single such request was previously
> supported. The strings are resolved to full URLs as previously: relative
> URLs are supported.
>
> Likewise, the attributionsrc window.open feature can be specified
> multiple times.
>
> URLs that are not allowed to register are logged to DevTools but
> otherwise ignored, and do not affect the valid URLs in the list, if any.
>
> Registrations from multiple requests are multiplexed onto a single
> AttributionDataHost Mojo remote because the registration order *between*
> requests is unspecified. This reduces resource consumption in both the
> renderer and the browser and makes it unnecessary to associate a single
> AttributionSrcToken with multiple data hosts, which would require
> significant browser-side refactoring.
>
> As a result of the multiplexing change, we move the
> Conversions.Registered(Sources|Triggers)PerDataHost metrics to the
> renderer, where it is easier to track per redirect chain, and combine
> them into a single metric for both types of registrations.
>
> Bug: 1430194, 1430496, 1434306
> Change-Id: Id5dbc42a20a78394560f11aa7fc57bfa2d92ae78
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4393405
> Reviewed-by: Nate Chapin <japhet@chromium.org>
> Quick-Run: Andrew Paseltiner <apaseltiner@chromium.org>
> Reviewed-by: Nan Lin <linnan@chromium.org>
> Commit-Queue: Andrew Paseltiner <apaseltiner@chromium.org>
> Reviewed-by: Charlie Harrison <csharrison@chromium.org>
> Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
> Cr-Commit-Position: refs/heads/main@{#1133249}

Bug: 1430194, 1430496, 1434306
Change-Id: I09257bc5711a08c3312c1bd846b57bd86c139ebd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4454297
Reviewed-by: Nate Chapin <japhet@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Commit-Queue: Andrew Paseltiner <apaseltiner@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1133882}
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.

Consider supporting a list of url in attributionsrc on anchors/window.open
4 participants