[Topics] add Topics header for iframe navigation request [chromium/src : main]

0 views
Skip to first unread message

Yao Xiao (Gerrit)

unread,
Mar 20, 2023, 8:16:58 PM3/20/23
to Josh Karlin, alexmo...@chromium.org, asvitkine...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, creis...@chromium.org, navigation...@chromium.org

Attention is currently required from: Josh Karlin.

Yao Xiao would like Josh Karlin to review this change.

View Change

[Topics] add Topics header for iframe navigation request

Implement topics navigation header:
- Before a request or redirect is handled (i.e. NavigationRequest's
constructor, and OnRedirectChecksComplete()), calculate and append
the header.
- After the corresponding response is received (i.e.
OnRedirectChecksComplete() and CommitNavigation()), check and handle
the topics response header.

Also, fixed the checks in HTMLIFrameElement::ParseAttribute() and
HTMLIFrameElement::DidChangeAttributes(): currently, the RuntimeFeature
and SecureContext are not properly checked when the iframe attribute is
set via setAttribute. The annotation in the IDL wasn't sufficient.

Bug: 1352345
Change-Id: Ia84c67e8f9858a98d7161668bc82fa688c563497
---
M chrome/browser/browsing_topics/browsing_topics_service_browsertest.cc
M components/browsing_topics/browsing_topics_service_impl.cc
M components/browsing_topics/common/common_types.h
M content/browser/browsing_topics/header_util.cc
M content/browser/renderer_host/navigation_request.cc
M content/browser/renderer_host/navigation_request.h
M third_party/blink/renderer/core/html/html_iframe_element.cc
M third_party/blink/web_tests/external/wpt/browsing-topics/browsing-topics-permissions-policy-default.tentative.https.sub.html
M third_party/blink/web_tests/external/wpt/browsing-topics/browsing-topics-permissions-policy-none.tentative.https.sub.html
M third_party/blink/web_tests/external/wpt/browsing-topics/browsing-topics-permissions-policy-self.tentative.https.sub.html
A third_party/blink/web_tests/external/wpt/browsing-topics/iframe-topics-attribute-insecure-context.tentative.http.sub.html
A third_party/blink/web_tests/external/wpt/browsing-topics/iframe-topics-attribute.tentative.https.html
A third_party/blink/web_tests/external/wpt/browsing-topics/resources/check-topics-request-header-notify-parent.py
A third_party/blink/web_tests/external/wpt/browsing-topics/resources/navigation-header-util.sub.js
M tools/metrics/histograms/enums.xml
15 files changed, 544 insertions(+), 5 deletions(-)


To view, visit change 4356935. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Ia84c67e8f9858a98d7161668bc82fa688c563497
Gerrit-Change-Number: 4356935
Gerrit-PatchSet: 1
Gerrit-Owner: Yao Xiao <yao...@chromium.org>
Gerrit-Reviewer: Josh Karlin <jka...@chromium.org>
Gerrit-Reviewer: Yao Xiao <yao...@chromium.org>
Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>
Gerrit-Attention: Josh Karlin <jka...@chromium.org>
Gerrit-MessageType: newchange

Yao Xiao (Gerrit)

unread,
Mar 20, 2023, 8:17:03 PM3/20/23
to alexmo...@chromium.org, asvitkine...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, creis...@chromium.org, navigation...@chromium.org, Josh Karlin, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Josh Karlin.

View Change

1 comment:

To view, visit change 4356935. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Ia84c67e8f9858a98d7161668bc82fa688c563497
Gerrit-Change-Number: 4356935
Gerrit-PatchSet: 1
Gerrit-Owner: Yao Xiao <yao...@chromium.org>
Gerrit-Reviewer: Josh Karlin <jka...@chromium.org>
Gerrit-Reviewer: Yao Xiao <yao...@chromium.org>
Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>
Gerrit-Attention: Josh Karlin <jka...@chromium.org>
Gerrit-Comment-Date: Tue, 21 Mar 2023 00:16:56 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Blink W3C Test Autoroller (Gerrit)

unread,
Mar 20, 2023, 8:28:55 PM3/20/23
to Yao Xiao, alexmo...@chromium.org, asvitkine...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, creis...@chromium.org, navigation...@chromium.org, Josh Karlin, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Josh Karlin, Yao Xiao.

Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/39097.

When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.

WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process

View Change

    To view, visit change 4356935. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ia84c67e8f9858a98d7161668bc82fa688c563497
    Gerrit-Change-Number: 4356935
    Gerrit-PatchSet: 1
    Gerrit-Owner: Yao Xiao <yao...@chromium.org>
    Gerrit-Reviewer: Josh Karlin <jka...@chromium.org>
    Gerrit-Reviewer: Yao Xiao <yao...@chromium.org>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>
    Gerrit-Attention: Josh Karlin <jka...@chromium.org>
    Gerrit-Attention: Yao Xiao <yao...@chromium.org>
    Gerrit-Comment-Date: Tue, 21 Mar 2023 00:28:46 +0000
    Gerrit-HasComments: No
    Gerrit-Has-Labels: No
    Gerrit-MessageType: comment

    Josh Karlin (Gerrit)

    unread,
    Mar 22, 2023, 10:28:23 AM3/22/23
    to Yao Xiao, alexmo...@chromium.org, asvitkine...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, creis...@chromium.org, navigation...@chromium.org, Josh Karlin, Blink W3C Test Autoroller, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

    Attention is currently required from: Yao Xiao.

    Patch set 1:Code-Review +1

    View Change

    3 comments:

    • Patchset:

    • File chrome/browser/browsing_topics/browsing_topics_service_browsertest.cc:

      • Patch Set #1, Line 338: iframe.browsingTopics = $1;

        Please also test via the attribute in markup, e.g,. <iframe browsingTopics> vs <iframe>

    • File content/browser/renderer_host/navigation_request.cc:

    To view, visit change 4356935. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ia84c67e8f9858a98d7161668bc82fa688c563497
    Gerrit-Change-Number: 4356935
    Gerrit-PatchSet: 1
    Gerrit-Owner: Yao Xiao <yao...@chromium.org>
    Gerrit-Reviewer: Josh Karlin <jka...@chromium.org>
    Gerrit-Reviewer: Yao Xiao <yao...@chromium.org>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>
    Gerrit-Attention: Yao Xiao <yao...@chromium.org>
    Gerrit-Comment-Date: Wed, 22 Mar 2023 14:28:17 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    Gerrit-MessageType: comment

    Yao Xiao (Gerrit)

    unread,
    Mar 22, 2023, 1:30:57 PM3/22/23
    to Alex Moshchuk, alexmo...@chromium.org, asvitkine...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, creis...@chromium.org, navigation...@chromium.org, Josh Karlin

    Attention is currently required from: Alex Moshchuk.

    Yao Xiao would like Alex Moshchuk to review this change.

    View Change

    [Topics] add Topics header for iframe navigation request

    Implement topics navigation header:
    - Before a request or redirect is handled (i.e. NavigationRequest's
    constructor, and OnRedirectChecksComplete()), calculate and append
    the header.
    - After the corresponding response is received (i.e.
    OnRedirectChecksComplete() and CommitNavigation()), check and handle
    the topics response header.

    Also, fixed the checks in HTMLIFrameElement::ParseAttribute() and
    HTMLIFrameElement::DidChangeAttributes(): currently, the RuntimeFeature
    and SecureContext are not properly checked when the iframe attribute is
    set via setAttribute. The annotation in the IDL wasn't sufficient.

    Bug: 1352345
    Change-Id: Ia84c67e8f9858a98d7161668bc82fa688c563497
    ---
    M chrome/browser/browsing_topics/browsing_topics_service_browsertest.cc
    A chrome/test/data/browsing_topics/page_with_custom_attribute_iframe.html

    M components/browsing_topics/browsing_topics_service_impl.cc
    M components/browsing_topics/common/common_types.h
    M content/browser/browsing_topics/header_util.cc
    M content/browser/renderer_host/navigation_request.cc
    M content/browser/renderer_host/navigation_request.h
    M third_party/blink/renderer/core/html/html_iframe_element.cc
    M third_party/blink/web_tests/external/wpt/browsing-topics/browsing-topics-permissions-policy-default.tentative.https.sub.html
    M third_party/blink/web_tests/external/wpt/browsing-topics/browsing-topics-permissions-policy-none.tentative.https.sub.html
    M third_party/blink/web_tests/external/wpt/browsing-topics/browsing-topics-permissions-policy-self.tentative.https.sub.html
    A third_party/blink/web_tests/external/wpt/browsing-topics/iframe-topics-attribute-insecure-context.tentative.http.sub.html
    A third_party/blink/web_tests/external/wpt/browsing-topics/iframe-topics-attribute.tentative.https.html
    A third_party/blink/web_tests/external/wpt/browsing-topics/resources/check-topics-request-header-notify-parent.py
    A third_party/blink/web_tests/external/wpt/browsing-topics/resources/navigation-header-util.sub.js
    M tools/metrics/histograms/enums.xml
    16 files changed, 645 insertions(+), 5 deletions(-)


    To view, visit change 4356935. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ia84c67e8f9858a98d7161668bc82fa688c563497
    Gerrit-Change-Number: 4356935
    Gerrit-PatchSet: 2
    Gerrit-Owner: Yao Xiao <yao...@chromium.org>
    Gerrit-Reviewer: Alex Moshchuk <ale...@chromium.org>
    Gerrit-Reviewer: Josh Karlin <jka...@chromium.org>
    Gerrit-Reviewer: Yao Xiao <yao...@chromium.org>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>
    Gerrit-Attention: Alex Moshchuk <ale...@chromium.org>
    Gerrit-MessageType: newchange

    Yao Xiao (Gerrit)

    unread,
    Mar 22, 2023, 1:33:00 PM3/22/23
    to alexmo...@chromium.org, asvitkine...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, creis...@chromium.org, navigation...@chromium.org, Alex Moshchuk, Josh Karlin, Blink W3C Test Autoroller, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

    Attention is currently required from: Alex Moshchuk.

    View Change

    3 comments:

    • Patchset:

      • Patch Set #2:

        alexmos@: PTAL at navigation_request.h/.cc. Thanks!

    • File chrome/browser/browsing_topics/browsing_topics_service_browsertest.cc:

      • Please also test via the attribute in markup, e.g,. […]

        Done

    • File content/browser/renderer_host/navigation_request.cc:

      • Done

    To view, visit change 4356935. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ia84c67e8f9858a98d7161668bc82fa688c563497
    Gerrit-Change-Number: 4356935
    Gerrit-PatchSet: 2
    Gerrit-Owner: Yao Xiao <yao...@chromium.org>
    Gerrit-Reviewer: Alex Moshchuk <ale...@chromium.org>
    Gerrit-Reviewer: Josh Karlin <jka...@chromium.org>
    Gerrit-Reviewer: Yao Xiao <yao...@chromium.org>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>
    Gerrit-Attention: Alex Moshchuk <ale...@chromium.org>
    Gerrit-Comment-Date: Wed, 22 Mar 2023 17:30:55 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Josh Karlin <jka...@chromium.org>
    Gerrit-MessageType: comment

    Alex Moshchuk (Gerrit)

    unread,
    Mar 22, 2023, 6:45:40 PM3/22/23
    to Yao Xiao, alexmo...@chromium.org, asvitkine...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, creis...@chromium.org, navigation...@chromium.org, Josh Karlin, Blink W3C Test Autoroller, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

    Attention is currently required from: Yao Xiao.

    View Change

    8 comments:

    • Patchset:

    • File content/browser/renderer_host/navigation_request.cc:

      • Patch Set #2, Line 1068: DCHECK(rfh);

        I think this DCHECK isn't really needed, the fact that current_frame_host() isn't null is pretty fundamental.

      • Patch Set #2, Line 1074: rfh == rfh->GetMainFrame()

        Can we just check is_main_frame() instead?

      • Patch Set #2, Line 1095: url::Origin::Create(url);

        This won't account for things like sandbox flags, which could also make the final origin opaque. Is that ok?

      • Patch Set #2, Line 1111: parent_policy, container_policy, origin);

        Why do we need to recompute the permissions policy, rather than just using the one we already have in rfh->GetPermissionsPolicy()? Because we need to account for the target origin, and so you're trying to estimate what the future permissions policy would be when it commits? Can you please add a comment?

        More generally, I'm a bit worried this computation might diverge from how the permission policy is set in RFHI::ResetPermissionsPolicy(). Perhaps NavigationRequest should have a GetPermissionsPolicyToCommit(), similar to GetOriginToCommit() which we could reference both here and in ResetPermissionsPolicy()? This is fine to explore in a followup, though, as there's at least one other place that does this already and would need to change too (https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.cc;l=671;drc=b4bc946c63b2b95e1f05dec4e84adcadd10499c6)

      • Patch Set #2, Line 5016: if (topics_eligible_) {

        Can you add a high-level comment about what the redirect handling is achieving? I'm guessing the eligibility for sending the topics header might've changed based on the redirected URL, so we need to recompute it?

      • Patch Set #2, Line 5512: topics_eligible_ = false;

        Instead of CommitNavigation, we could end up going through CommitErrorPage in case of an error page. Should we reset this value in that case? (Not sure if it ever matters for anything, but it's probably best not to leave this as true for error pages.)

      • Patch Set #2, Line 5516: url::Origin::Create(common_params_->url)

        I'm curious whether you'd want to use GetOriginToCommit() here. That would also take into account sandbox flags and other weird cases, though on the other hand maybe you actually want to ignore all that for handling this.

    To view, visit change 4356935. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ia84c67e8f9858a98d7161668bc82fa688c563497
    Gerrit-Change-Number: 4356935
    Gerrit-PatchSet: 2
    Gerrit-Owner: Yao Xiao <yao...@chromium.org>
    Gerrit-Reviewer: Alex Moshchuk <ale...@chromium.org>
    Gerrit-Reviewer: Josh Karlin <jka...@chromium.org>
    Gerrit-Reviewer: Yao Xiao <yao...@chromium.org>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>
    Gerrit-Attention: Yao Xiao <yao...@chromium.org>
    Gerrit-Comment-Date: Wed, 22 Mar 2023 22:45:31 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Gerrit-MessageType: comment

    Yao Xiao (Gerrit)

    unread,
    Mar 23, 2023, 6:28:42 PM3/23/23
    to alexmo...@chromium.org, asvitkine...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, creis...@chromium.org, navigation...@chromium.org, Alex Moshchuk, Josh Karlin, Blink W3C Test Autoroller, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

    Attention is currently required from: Alex Moshchuk.

    View Change

    7 comments:

    • File content/browser/renderer_host/navigation_request.cc:

      • I think this DCHECK isn't really needed, the fact that current_frame_host() isn't null is pretty fun […]

        Done

      • Done

      • Why do we need to recompute the permissions policy, rather than just using the one we already have i […]

        I switched to `parent_policy->IsFeatureEnabledForOrigin()`. There's no general consensus or spec on how navigation request should be checked for permissions policy. All features are doing something special (ClientHint, TrustToken). I still did the switch, to align with the existing permissions policy checks for topics fetch() request.

      • Can you add a high-level comment about what the redirect handling is achieving? I'm guessing the el […]

        Done

      • Instead of CommitNavigation, we could end up going through CommitErrorPage in case of an error page. […]

        Done

      • I'm curious whether you'd want to use GetOriginToCommit() here. […]

        See the other comment. I think it's *okay* to use the URL.

    To view, visit change 4356935. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ia84c67e8f9858a98d7161668bc82fa688c563497
    Gerrit-Change-Number: 4356935
    Gerrit-PatchSet: 3
    Gerrit-Owner: Yao Xiao <yao...@chromium.org>
    Gerrit-Reviewer: Alex Moshchuk <ale...@chromium.org>
    Gerrit-Reviewer: Josh Karlin <jka...@chromium.org>
    Gerrit-Reviewer: Yao Xiao <yao...@chromium.org>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>
    Gerrit-Attention: Alex Moshchuk <ale...@chromium.org>
    Gerrit-Comment-Date: Thu, 23 Mar 2023 22:28:37 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Alex Moshchuk <ale...@chromium.org>
    Gerrit-MessageType: comment

    Alex Moshchuk (Gerrit)

    unread,
    Mar 24, 2023, 5:49:51 PM3/24/23
    to Yao Xiao, alexmo...@chromium.org, asvitkine...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, creis...@chromium.org, navigation...@chromium.org, Josh Karlin, Blink W3C Test Autoroller, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

    Attention is currently required from: Yao Xiao.

    Patch set 3:Code-Review +1

    View Change

    4 comments:

    • Patchset:

    • File content/browser/renderer_host/navigation_request.cc:

      • Patch Set #2, Line 1095: url::Origin::Create(url);

        I think this is okay. […]

        Ack. Could you please add a comment that this should align with the handling in GetTopicsHeaderValueForSubresourceRequest() that you pointed to, so that someone doesn't forget to update it also if this ever changes in the future?

      • I switched to `parent_policy->IsFeatureEnabledForOrigin()`. […]

        Ack. Might also be nice to file a bug and include permission policy folks to get agreement on how to properly check permissions policy in NavigationRequest.

      • See the other comment. I think it's *okay* to use the URL.

        Acknowledged

    To view, visit change 4356935. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ia84c67e8f9858a98d7161668bc82fa688c563497
    Gerrit-Change-Number: 4356935
    Gerrit-PatchSet: 3
    Gerrit-Owner: Yao Xiao <yao...@chromium.org>
    Gerrit-Reviewer: Alex Moshchuk <ale...@chromium.org>
    Gerrit-Reviewer: Josh Karlin <jka...@chromium.org>
    Gerrit-Reviewer: Yao Xiao <yao...@chromium.org>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>
    Gerrit-Attention: Yao Xiao <yao...@chromium.org>
    Gerrit-Comment-Date: Fri, 24 Mar 2023 21:49:44 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    Comment-In-Reply-To: Yao Xiao <yao...@chromium.org>

    Josh Karlin (Gerrit)

    unread,
    Mar 27, 2023, 9:34:46 AM3/27/23
    to Yao Xiao, alexmo...@chromium.org, asvitkine...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, creis...@chromium.org, navigation...@chromium.org, Josh Karlin, Alex Moshchuk, Blink W3C Test Autoroller, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

    Attention is currently required from: Yao Xiao.

    Patch set 3:Code-Review +1

    View Change

    1 comment:

    • Patchset:

    To view, visit change 4356935. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ia84c67e8f9858a98d7161668bc82fa688c563497
    Gerrit-Change-Number: 4356935
    Gerrit-PatchSet: 3
    Gerrit-Owner: Yao Xiao <yao...@chromium.org>
    Gerrit-Reviewer: Alex Moshchuk <ale...@chromium.org>
    Gerrit-Reviewer: Josh Karlin <jka...@chromium.org>
    Gerrit-Reviewer: Yao Xiao <yao...@chromium.org>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>
    Gerrit-Attention: Yao Xiao <yao...@chromium.org>
    Gerrit-Comment-Date: Mon, 27 Mar 2023 13:34:30 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    Gerrit-MessageType: comment

    Yao Xiao (Gerrit)

    unread,
    Mar 27, 2023, 12:39:31 PM3/27/23
    to Dominic Farolino, alexmo...@chromium.org, asvitkine...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, creis...@chromium.org, navigation...@chromium.org, Josh Karlin, Alex Moshchuk

    Attention is currently required from: Dominic Farolino.

    Yao Xiao would like Dominic Farolino to review this change.

    View Change

    16 files changed, 649 insertions(+), 5 deletions(-)


    To view, visit change 4356935. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ia84c67e8f9858a98d7161668bc82fa688c563497
    Gerrit-Change-Number: 4356935
    Gerrit-PatchSet: 4
    Gerrit-Owner: Yao Xiao <yao...@chromium.org>
    Gerrit-Reviewer: Alex Moshchuk <ale...@chromium.org>
    Gerrit-Reviewer: Dominic Farolino <d...@chromium.org>
    Gerrit-Reviewer: Josh Karlin <jka...@chromium.org>
    Gerrit-Reviewer: Yao Xiao <yao...@chromium.org>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>
    Gerrit-Attention: Dominic Farolino <d...@chromium.org>
    Gerrit-MessageType: newchange

    Yao Xiao (Gerrit)

    unread,
    Mar 27, 2023, 12:39:34 PM3/27/23
    to alexmo...@chromium.org, asvitkine...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, creis...@chromium.org, navigation...@chromium.org, Dominic Farolino, Josh Karlin, Alex Moshchuk, Blink W3C Test Autoroller, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

    Attention is currently required from: Dominic Farolino.

    View Change

    3 comments:

    • Patchset:

    • File content/browser/renderer_host/navigation_request.cc:

      • Ack. […]

        Done. Added a function-level comment.

    To view, visit change 4356935. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ia84c67e8f9858a98d7161668bc82fa688c563497
    Gerrit-Change-Number: 4356935
    Gerrit-PatchSet: 4
    Gerrit-Owner: Yao Xiao <yao...@chromium.org>
    Gerrit-Reviewer: Alex Moshchuk <ale...@chromium.org>
    Gerrit-Reviewer: Dominic Farolino <d...@chromium.org>
    Gerrit-Reviewer: Josh Karlin <jka...@chromium.org>
    Gerrit-Reviewer: Yao Xiao <yao...@chromium.org>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>
    Gerrit-Attention: Dominic Farolino <d...@chromium.org>
    Gerrit-Comment-Date: Mon, 27 Mar 2023 16:39:27 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Alex Moshchuk <ale...@chromium.org>
    Comment-In-Reply-To: Yao Xiao <yao...@chromium.org>
    Gerrit-MessageType: comment

    Dominic Farolino (Gerrit)

    unread,
    Mar 27, 2023, 1:45:47 PM3/27/23
    to Yao Xiao, alexmo...@chromium.org, asvitkine...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, creis...@chromium.org, navigation...@chromium.org, Josh Karlin, Alex Moshchuk, Blink W3C Test Autoroller, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

    Attention is currently required from: Yao Xiao.

    View Change

    1 comment:

    • Commit Message:

      • Patch Set #4, Line 20: set via setAttribute. The annotation in the IDL wasn't sufficient.

        I'm observing the current behavior working correctly in Chromium on HTTP sites. Can you clarify what you're observing, because this should not be necessary.

    To view, visit change 4356935. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ia84c67e8f9858a98d7161668bc82fa688c563497
    Gerrit-Change-Number: 4356935
    Gerrit-PatchSet: 4
    Gerrit-Owner: Yao Xiao <yao...@chromium.org>
    Gerrit-Reviewer: Alex Moshchuk <ale...@chromium.org>
    Gerrit-Reviewer: Dominic Farolino <d...@chromium.org>
    Gerrit-Reviewer: Josh Karlin <jka...@chromium.org>
    Gerrit-Reviewer: Yao Xiao <yao...@chromium.org>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>
    Gerrit-Attention: Yao Xiao <yao...@chromium.org>
    Gerrit-Comment-Date: Mon, 27 Mar 2023 17:45:35 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Gerrit-MessageType: comment

    Yao Xiao (Gerrit)

    unread,
    Mar 27, 2023, 2:17:29 PM3/27/23
    to alexmo...@chromium.org, asvitkine...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, creis...@chromium.org, navigation...@chromium.org, Dominic Farolino, Josh Karlin, Alex Moshchuk, Blink W3C Test Autoroller, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

    Attention is currently required from: Dominic Farolino.

    View Change

    1 comment:

    • Commit Message:

      • I'm observing the current behavior working correctly in Chromium on HTTP sites. […]

        Without the change, the second test case in `third_party/blink/web_tests/external/wpt/browsing-topics/iframe-topics-attribute-insecure-context.tentative.http.sub.html` would fail (which sets the attribute via `frame.setAttribute("browsingtopics", "123");` / doesn't go through the IDL attribute).

    To view, visit change 4356935. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ia84c67e8f9858a98d7161668bc82fa688c563497
    Gerrit-Change-Number: 4356935
    Gerrit-PatchSet: 4
    Gerrit-Owner: Yao Xiao <yao...@chromium.org>
    Gerrit-Reviewer: Alex Moshchuk <ale...@chromium.org>
    Gerrit-Reviewer: Dominic Farolino <d...@chromium.org>
    Gerrit-Reviewer: Josh Karlin <jka...@chromium.org>
    Gerrit-Reviewer: Yao Xiao <yao...@chromium.org>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>
    Gerrit-Attention: Dominic Farolino <d...@chromium.org>
    Gerrit-Comment-Date: Mon, 27 Mar 2023 18:17:21 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Dominic Farolino <d...@chromium.org>
    Gerrit-MessageType: comment

    Dominic Farolino (Gerrit)

    unread,
    Mar 27, 2023, 2:42:15 PM3/27/23
    to Yao Xiao, alexmo...@chromium.org, asvitkine...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, creis...@chromium.org, navigation...@chromium.org, Josh Karlin, Alex Moshchuk, Blink W3C Test Autoroller, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

    Attention is currently required from: Yao Xiao.

    Patch set 4:Code-Review +1

    View Change

    1 comment:

    • Commit Message:

      • Without the change, the second test case in `third_party/blink/web_tests/external/wpt/browsing-topic […]

        Ah that makes sense. In that case, please also update the spec accordingly to cater to this.

    To view, visit change 4356935. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ia84c67e8f9858a98d7161668bc82fa688c563497
    Gerrit-Change-Number: 4356935
    Gerrit-PatchSet: 4
    Gerrit-Owner: Yao Xiao <yao...@chromium.org>
    Gerrit-Reviewer: Alex Moshchuk <ale...@chromium.org>
    Gerrit-Reviewer: Dominic Farolino <d...@chromium.org>
    Gerrit-Reviewer: Josh Karlin <jka...@chromium.org>
    Gerrit-Reviewer: Yao Xiao <yao...@chromium.org>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>
    Gerrit-Attention: Yao Xiao <yao...@chromium.org>
    Gerrit-Comment-Date: Mon, 27 Mar 2023 18:42:09 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    Comment-In-Reply-To: Dominic Farolino <d...@chromium.org>

    Yao Xiao (Gerrit)

    unread,
    Mar 27, 2023, 2:56:03 PM3/27/23
    to alexmo...@chromium.org, asvitkine...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, creis...@chromium.org, navigation...@chromium.org, Dominic Farolino, Josh Karlin, Alex Moshchuk, Blink W3C Test Autoroller, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

    View Change

    1 comment:

    • Commit Message:

      • Ah that makes sense. In that case, please also update the spec accordingly to cater to this.

        Ack. Will update the spec.

    To view, visit change 4356935. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ia84c67e8f9858a98d7161668bc82fa688c563497
    Gerrit-Change-Number: 4356935
    Gerrit-PatchSet: 4
    Gerrit-Owner: Yao Xiao <yao...@chromium.org>
    Gerrit-Reviewer: Alex Moshchuk <ale...@chromium.org>
    Gerrit-Reviewer: Dominic Farolino <d...@chromium.org>
    Gerrit-Reviewer: Josh Karlin <jka...@chromium.org>
    Gerrit-Reviewer: Yao Xiao <yao...@chromium.org>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>
    Gerrit-Comment-Date: Mon, 27 Mar 2023 18:55:51 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No

    Yao Xiao (Gerrit)

    unread,
    Mar 27, 2023, 2:56:05 PM3/27/23
    to alexmo...@chromium.org, asvitkine...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, creis...@chromium.org, navigation...@chromium.org, Dominic Farolino, Josh Karlin, Alex Moshchuk, Blink W3C Test Autoroller, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

    Patch set 4:Commit-Queue +2

    View Change

      To view, visit change 4356935. To unsubscribe, or for help writing mail filters, visit settings.

      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: Ia84c67e8f9858a98d7161668bc82fa688c563497
      Gerrit-Change-Number: 4356935
      Gerrit-PatchSet: 4
      Gerrit-Owner: Yao Xiao <yao...@chromium.org>
      Gerrit-Reviewer: Alex Moshchuk <ale...@chromium.org>
      Gerrit-Reviewer: Dominic Farolino <d...@chromium.org>
      Gerrit-Reviewer: Josh Karlin <jka...@chromium.org>
      Gerrit-Reviewer: Yao Xiao <yao...@chromium.org>
      Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
      Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>
      Gerrit-Comment-Date: Mon, 27 Mar 2023 18:55:57 +0000
      Gerrit-HasComments: No
      Gerrit-Has-Labels: Yes
      Gerrit-MessageType: comment

      Chromium LUCI CQ (Gerrit)

      unread,
      Mar 27, 2023, 3:02:03 PM3/27/23
      to Yao Xiao, alexmo...@chromium.org, asvitkine...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, creis...@chromium.org, navigation...@chromium.org, Dominic Farolino, Josh Karlin, Alex Moshchuk, Blink W3C Test Autoroller, Chromium Metrics Reviews, chromium...@chromium.org

      Chromium LUCI CQ submitted this change.

      View Change

      Approvals: Yao Xiao: Commit Dominic Farolino: Looks good to me Josh Karlin: Looks good to me Alex Moshchuk: Looks good to me
      [Topics] add Topics header for iframe navigation request

      Implement topics navigation header:
      - Before a request or redirect is handled (i.e. NavigationRequest's
      constructor, and OnRedirectChecksComplete()), calculate and append
      the header.
      - After the corresponding response is received (i.e.
      OnRedirectChecksComplete() and CommitNavigation()), check and handle
      the topics response header.

      Also, fixed the checks in HTMLIFrameElement::ParseAttribute() and
      HTMLIFrameElement::DidChangeAttributes(): currently, the RuntimeFeature
      and SecureContext are not properly checked when the iframe attribute is
      set via setAttribute. The annotation in the IDL wasn't sufficient.

      Bug: 1352345
      Change-Id: Ia84c67e8f9858a98d7161668bc82fa688c563497
      Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4356935
      Reviewed-by: Alex Moshchuk <ale...@chromium.org>
      Reviewed-by: Josh Karlin <jka...@chromium.org>
      Commit-Queue: Yao Xiao <yao...@chromium.org>
      Reviewed-by: Dominic Farolino <d...@chromium.org>
      Cr-Commit-Position: refs/heads/main@{#1122570}

      ---
      M chrome/browser/browsing_topics/browsing_topics_service_browsertest.cc
      A chrome/test/data/browsing_topics/page_with_custom_attribute_iframe.html
      M components/browsing_topics/browsing_topics_service_impl.cc
      M components/browsing_topics/common/common_types.h
      M content/browser/browsing_topics/header_util.cc
      M content/browser/renderer_host/navigation_request.cc
      M content/browser/renderer_host/navigation_request.h
      M third_party/blink/renderer/core/html/html_iframe_element.cc
      M third_party/blink/web_tests/external/wpt/browsing-topics/browsing-topics-permissions-policy-default.tentative.https.sub.html
      M third_party/blink/web_tests/external/wpt/browsing-topics/browsing-topics-permissions-policy-none.tentative.https.sub.html
      M third_party/blink/web_tests/external/wpt/browsing-topics/browsing-topics-permissions-policy-self.tentative.https.sub.html
      A third_party/blink/web_tests/external/wpt/browsing-topics/iframe-topics-attribute-insecure-context.tentative.http.sub.html
      A third_party/blink/web_tests/external/wpt/browsing-topics/iframe-topics-attribute.tentative.https.html
      A third_party/blink/web_tests/external/wpt/browsing-topics/resources/check-topics-request-header-notify-parent.py
      A third_party/blink/web_tests/external/wpt/browsing-topics/resources/navigation-header-util.sub.js
      M tools/metrics/histograms/enums.xml
      16 files changed, 649 insertions(+), 5 deletions(-)


      To view, visit change 4356935. To unsubscribe, or for help writing mail filters, visit settings.

      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: Ia84c67e8f9858a98d7161668bc82fa688c563497
      Gerrit-Change-Number: 4356935
      Gerrit-PatchSet: 5
      Gerrit-Owner: Yao Xiao <yao...@chromium.org>
      Gerrit-Reviewer: Alex Moshchuk <ale...@chromium.org>
      Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>
      Gerrit-Reviewer: Dominic Farolino <d...@chromium.org>
      Gerrit-Reviewer: Josh Karlin <jka...@chromium.org>
      Gerrit-Reviewer: Yao Xiao <yao...@chromium.org>
      Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
      Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>
      Gerrit-MessageType: merged

      Blink W3C Test Autoroller (Gerrit)

      unread,
      Mar 27, 2023, 3:27:03 PM3/27/23
      to Chromium LUCI CQ, Yao Xiao, alexmo...@chromium.org, asvitkine...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, creis...@chromium.org, navigation...@chromium.org, Dominic Farolino, Josh Karlin, Alex Moshchuk, Chromium Metrics Reviews, chromium...@chromium.org

      The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/39097

      View Change

        To view, visit change 4356935. To unsubscribe, or for help writing mail filters, visit settings.

        Gerrit-Project: chromium/src
        Gerrit-Branch: main
        Gerrit-Change-Id: Ia84c67e8f9858a98d7161668bc82fa688c563497
        Gerrit-Change-Number: 4356935
        Gerrit-PatchSet: 5
        Gerrit-Owner: Yao Xiao <yao...@chromium.org>
        Gerrit-Reviewer: Alex Moshchuk <ale...@chromium.org>
        Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>
        Gerrit-Reviewer: Dominic Farolino <d...@chromium.org>
        Gerrit-Reviewer: Josh Karlin <jka...@chromium.org>
        Gerrit-Reviewer: Yao Xiao <yao...@chromium.org>
        Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
        Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>
        Gerrit-Comment-Date: Mon, 27 Mar 2023 19:26:54 +0000
        Reply all
        Reply to author
        Forward
        0 new messages