Allow all HTML attributionsrc surfaces to make multiple background requests #744
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
csharrison
approved these changes
Mar 29, 2023
dmdabbs
reviewed
Mar 29, 2023
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We use spaces due to the precedent from
<a ping>. We will determine the syntax forwindow.openseparately.Fixes #718