Implement performance.markConditional(...) [chromium/src : main]

0 views
Skip to first unread message

Guohui Deng (Gerrit)

unread,
Jun 18, 2026, 11:41:56 PMJun 18
to Noam Rosenthal, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, Menard, Alexis, chromium...@chromium.org, Daniel Cheng, Olga Gerchikov, Kentaro Hara, Raphael Kubo da Costa, android-web...@chromium.org, ashleynewson+watch...@chromium.org, ashleynewson+watch-...@chromium.org, blink-revie...@chromium.org, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, core-timi...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, peter+watch...@chromium.org, speed-metrics...@chromium.org
Attention needed from Noam Rosenthal

Guohui Deng added 1 comment

Patchset-level comments
File-level comment, Patchset 1 (Latest):
Guohui Deng . resolved

Thanks for reviewing!

Open in Gerrit

Related details

Attention is currently required from:
  • Noam Rosenthal
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Owners
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I9e6728a44975cd8e7bd8ef3ff280841f058fcaea
Gerrit-Change-Number: 7965263
Gerrit-PatchSet: 1
Gerrit-Owner: Guohui Deng <guohu...@microsoft.com>
Gerrit-Reviewer: Guohui Deng <guohu...@microsoft.com>
Gerrit-Reviewer: Noam Rosenthal <nrose...@google.com>
Gerrit-CC: Daniel Cheng <dch...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-CC: Menard, Alexis <alexis...@intel.com>
Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>
Gerrit-CC: Raphael Kubo da Costa <ku...@igalia.com>
Gerrit-Attention: Noam Rosenthal <nrose...@google.com>
Gerrit-Comment-Date: Fri, 19 Jun 2026 03:41:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Noam Rosenthal (Gerrit)

unread,
Jun 22, 2026, 10:54:53 AM (11 days ago) Jun 22
to Guohui Deng, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, Menard, Alexis, chromium...@chromium.org, Daniel Cheng, Olga Gerchikov, Kentaro Hara, Raphael Kubo da Costa, android-web...@chromium.org, ashleynewson+watch...@chromium.org, ashleynewson+watch-...@chromium.org, blink-revie...@chromium.org, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, core-timi...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, peter+watch...@chromium.org, speed-metrics...@chromium.org
Attention needed from Guohui Deng

Noam Rosenthal added 7 comments

Patchset-level comments
Noam Rosenthal . resolved

Thanks for working on this! Mostly looks good.
A few nits about how to check for flag, and one substantive comment:
https://chromium-review.git.corp.google.com/c/chromium/src/+/7965263/comment/3fd6dc37_89cc7c78/

File third_party/blink/renderer/core/frame/animation_frame_timing_monitor.cc
Line 130, Patchset 1 (Latest): if (RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) {
Noam Rosenthal . unresolved

Would this be a noop if the feature is disabled? If so, perhaps make this non-conditional and CHECK instead of guarding by condition?

Line 170, Patchset 1 (Latest): if (RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) {
Noam Rosenthal . unresolved

Would this be a noop if the feature is disabled? If so, perhaps make this non-conditional and CHECK instead of guarding by condition?

Line 283, Patchset 1 (Latest): if (RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) {
Noam Rosenthal . unresolved

Would this be a noop if the feature is disabled? If so, perhaps make this non-conditional and CHECK instead of guarding by condition?

Line 315, Patchset 1 (Latest): if (RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) {
Noam Rosenthal . unresolved

Would this be a noop if the feature is disabled? If so, perhaps make this non-conditional and CHECK instead of guarding by condition?

File third_party/blink/renderer/core/timing/animation_frame_timing_info.h
Line 199, Patchset 1 (Latest): PerformanceEntryVector conditional_user_timing_;
Noam Rosenthal . unresolved

I don't think this is the right approach. We should retain a vector of structs (containing string+time), kind of like ScriptTimingInfo, Convert to PerformanceEntry only when reporting to the actual DOM timeline.

File third_party/blink/renderer/core/timing/performance_long_animation_frame_timing.cc
Line 98, Patchset 1 (Latest): if (RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) {
Noam Rosenthal . unresolved

Would this be a noop if the feature is disabled? If so, perhaps make this non-conditional and CHECK instead of guarding by condition?

Open in Gerrit

Related details

Attention is currently required from:
  • Guohui Deng
Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Owners
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    • requirement is not satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I9e6728a44975cd8e7bd8ef3ff280841f058fcaea
    Gerrit-Change-Number: 7965263
    Gerrit-PatchSet: 1
    Gerrit-Owner: Guohui Deng <guohu...@microsoft.com>
    Gerrit-Reviewer: Guohui Deng <guohu...@microsoft.com>
    Gerrit-Reviewer: Noam Rosenthal <nrose...@google.com>
    Gerrit-CC: Daniel Cheng <dch...@chromium.org>
    Gerrit-CC: Kentaro Hara <har...@chromium.org>
    Gerrit-CC: Menard, Alexis <alexis...@intel.com>
    Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>
    Gerrit-CC: Raphael Kubo da Costa <ku...@igalia.com>
    Gerrit-Attention: Guohui Deng <guohu...@microsoft.com>
    Gerrit-Comment-Date: Mon, 22 Jun 2026 14:54:36 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Guohui Deng (Gerrit)

    unread,
    Jun 25, 2026, 3:15:39 PM (8 days ago) Jun 25
    to Noam Rosenthal, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, Menard, Alexis, chromium...@chromium.org, Daniel Cheng, Olga Gerchikov, Kentaro Hara, Raphael Kubo da Costa, android-web...@chromium.org, ashleynewson+watch...@chromium.org, ashleynewson+watch-...@chromium.org, blink-revie...@chromium.org, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, core-timi...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, peter+watch...@chromium.org, speed-metrics...@chromium.org
    Attention needed from Noam Rosenthal

    Guohui Deng added 6 comments

    File third_party/blink/renderer/core/frame/animation_frame_timing_monitor.cc
    Line 130, Patchset 1: if (RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) {
    Noam Rosenthal . unresolved

    Would this be a noop if the feature is disabled? If so, perhaps make this non-conditional and CHECK instead of guarding by condition?

    Guohui Deng

    I used DCHECK instead of CHECK. Is it O.K.? (This question also applies to the rest.)

    Line 170, Patchset 1: if (RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) {
    Noam Rosenthal . unresolved

    Would this be a noop if the feature is disabled? If so, perhaps make this non-conditional and CHECK instead of guarding by condition?

    Guohui Deng

    Ditto

    Line 283, Patchset 1: if (RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) {
    Noam Rosenthal . unresolved

    Would this be a noop if the feature is disabled? If so, perhaps make this non-conditional and CHECK instead of guarding by condition?

    Guohui Deng

    Ditto

    Line 315, Patchset 1: if (RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) {
    Noam Rosenthal . unresolved

    Would this be a noop if the feature is disabled? If so, perhaps make this non-conditional and CHECK instead of guarding by condition?

    Guohui Deng

    Ditto.

    File third_party/blink/renderer/core/timing/animation_frame_timing_info.h
    Line 199, Patchset 1: PerformanceEntryVector conditional_user_timing_;
    Noam Rosenthal . unresolved

    I don't think this is the right approach. We should retain a vector of structs (containing string+time), kind of like ScriptTimingInfo, Convert to PerformanceEntry only when reporting to the actual DOM timeline.

    Guohui Deng

    Done. I also added security check (following the "scripts" example) -- I realized this is different from "standard user timing". The top frame can get the "conditional marks" from an Iframe.

    File third_party/blink/renderer/core/timing/performance_long_animation_frame_timing.cc
    Line 98, Patchset 1: if (RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) {
    Noam Rosenthal . unresolved

    Would this be a noop if the feature is disabled? If so, perhaps make this non-conditional and CHECK instead of guarding by condition?

    Guohui Deng

    Ditto.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Noam Rosenthal
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Owners
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    • requirement is not satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I9e6728a44975cd8e7bd8ef3ff280841f058fcaea
    Gerrit-Change-Number: 7965263
    Gerrit-PatchSet: 2
    Gerrit-Owner: Guohui Deng <guohu...@microsoft.com>
    Gerrit-Reviewer: Guohui Deng <guohu...@microsoft.com>
    Gerrit-Reviewer: Noam Rosenthal <nrose...@google.com>
    Gerrit-CC: Daniel Cheng <dch...@chromium.org>
    Gerrit-CC: Kentaro Hara <har...@chromium.org>
    Gerrit-CC: Menard, Alexis <alexis...@intel.com>
    Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>
    Gerrit-CC: Raphael Kubo da Costa <ku...@igalia.com>
    Gerrit-Attention: Noam Rosenthal <nrose...@google.com>
    Gerrit-Comment-Date: Thu, 25 Jun 2026 19:15:27 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Noam Rosenthal <nrose...@google.com>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Noam Rosenthal (Gerrit)

    unread,
    Jun 27, 2026, 3:32:19 PM (6 days ago) Jun 27
    to Guohui Deng, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, Menard, Alexis, chromium...@chromium.org, Daniel Cheng, Olga Gerchikov, Kentaro Hara, Raphael Kubo da Costa, android-web...@chromium.org, ashleynewson+watch...@chromium.org, ashleynewson+watch-...@chromium.org, blink-revie...@chromium.org, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, core-timi...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, peter+watch...@chromium.org, speed-metrics...@chromium.org
    Attention needed from Guohui Deng

    Noam Rosenthal added 5 comments

    File third_party/blink/renderer/core/frame/animation_frame_timing_monitor.cc
    Line 130, Patchset 1: if (RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) {
    Noam Rosenthal . resolved

    Would this be a noop if the feature is disabled? If so, perhaps make this non-conditional and CHECK instead of guarding by condition?

    Guohui Deng

    I used DCHECK instead of CHECK. Is it O.K.? (This question also applies to the rest.)

    Noam Rosenthal

    Acknowledged

    Line 170, Patchset 1: if (RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) {
    Noam Rosenthal . resolved

    Would this be a noop if the feature is disabled? If so, perhaps make this non-conditional and CHECK instead of guarding by condition?

    Guohui Deng

    Ditto

    Noam Rosenthal

    Done

    Line 283, Patchset 1: if (RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) {
    Noam Rosenthal . resolved

    Would this be a noop if the feature is disabled? If so, perhaps make this non-conditional and CHECK instead of guarding by condition?

    Guohui Deng

    Ditto

    Noam Rosenthal

    Acknowledged

    Line 315, Patchset 1: if (RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) {
    Noam Rosenthal . resolved

    Would this be a noop if the feature is disabled? If so, perhaps make this non-conditional and CHECK instead of guarding by condition?

    Guohui Deng

    Ditto.

    Noam Rosenthal

    Acknowledged

    File third_party/blink/renderer/core/timing/performance_long_animation_frame_timing.cc
    Line 98, Patchset 1: if (RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) {
    Noam Rosenthal . resolved

    Would this be a noop if the feature is disabled? If so, perhaps make this non-conditional and CHECK instead of guarding by condition?

    Guohui Deng

    Ditto.

    Noam Rosenthal

    Acknowledged

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Guohui Deng
    Gerrit-Attention: Guohui Deng <guohu...@microsoft.com>
    Gerrit-Comment-Date: Sat, 27 Jun 2026 19:31:53 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Guohui Deng <guohu...@microsoft.com>
    Comment-In-Reply-To: Noam Rosenthal <nrose...@google.com>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Noam Rosenthal (Gerrit)

    unread,
    Jun 27, 2026, 3:36:42 PM (6 days ago) Jun 27
    to Guohui Deng, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, Menard, Alexis, chromium...@chromium.org, Daniel Cheng, Olga Gerchikov, Kentaro Hara, Raphael Kubo da Costa, android-web...@chromium.org, ashleynewson+watch...@chromium.org, ashleynewson+watch-...@chromium.org, blink-revie...@chromium.org, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, core-timi...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, peter+watch...@chromium.org, speed-metrics...@chromium.org
    Attention needed from Guohui Deng

    Noam Rosenthal added 5 comments

    File third_party/blink/renderer/core/frame/animation_frame_timing_monitor.cc
    Line 135, Patchset 2 (Latest): DCHECK(conditional_marks_.empty());
    Noam Rosenthal . unresolved

    I think just CHECK(RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) in the condition below?

    (CHECK and not DCHECK plz)

    Line 177, Patchset 2 (Latest): if (!RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) {
    Noam Rosenthal . unresolved

    Ditto

    Line 294, Patchset 2 (Latest): if (!RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) {
    Noam Rosenthal . unresolved

    Ditto

    Line 331, Patchset 2 (Latest): timing_info->SetConditionalMarks(conditional_marks);
    Noam Rosenthal . unresolved

    Another flag check?

    File third_party/blink/web_tests/external/wpt/long-animation-frame/conditional-tracing.html
    Line 1, Patchset 2 (Latest):<!DOCTYPE HTML>
    Noam Rosenthal . unresolved

    Is there a test for the security origin checks?

    Gerrit-Comment-Date: Sat, 27 Jun 2026 19:36:16 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy
    Reply all
    Reply to author
    Forward
    0 new messages