Split WebRTC.UserMediaRequest.Result2 per request type [chromium/src : main]

0 views
Skip to first unread message

Palak Agarwal (Gerrit)

unread,
Oct 22, 2025, 2:30:21 AM (11 days ago) Oct 22
to Kentaro Hara, Raphael Kubo da Costa, Johannes Kron, AyeAye, AI Code Reviewer, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, blink-revie...@chromium.org, asvitkine...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, kinuko...@chromium.org, tommyw+w...@chromium.org
Attention needed from Johannes Kron

Palak Agarwal added 3 comments

File third_party/blink/renderer/modules/mediastream/user_media_processor.cc
Line 93, Patchset 4: UMA_HISTOGRAM_ENUMERATION(
Johannes Kron . resolved

It's usually preferred to use the histogram functions, `base::UmaHistogramEnumeration` instead of UMA_HISTOGRAM_ENUMERATION.

See http://go/uma-api#coding-emitting-to-histograms

Palak Agarwal

Done

Line 1220, Patchset 4: record_uma_cb_.emplace(
Johannes Kron . resolved

I don't see where the entries in `record_uma_cb_` are removed?

Palak Agarwal

I've removed the callback now, so this comment is no longer relevant.

File tools/metrics/histograms/metadata/web_rtc/histograms.xml
Line 1909, Patchset 4:<histogram name="WebRTC.UserMediaRequest.GetAllScreensMedia.Result"
Johannes Kron . resolved

You may use `<token>` and `<variant>` to put everything in one histogram entry. See for example `WebRTC.Stun.Integrity.{StunPacketType}` a few lines above. In that case, you can refer to the variant in the summary by using the token name: `Counters for UserMediaRequests results for type {RequestType}...`.

Palak Agarwal

Done

Open in Gerrit

Related details

Attention is currently required from:
  • Johannes Kron
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: I882eedf64a0801f780999298b2435d94aa4dca21
Gerrit-Change-Number: 7062737
Gerrit-PatchSet: 6
Gerrit-Owner: Palak Agarwal <agp...@chromium.org>
Gerrit-Reviewer: Johannes Kron <kr...@chromium.org>
Gerrit-Reviewer: Palak Agarwal <agp...@chromium.org>
Gerrit-CC: AI Code Reviewer <peep-gen...@system.gserviceaccount.com>
Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-CC: Raphael Kubo da Costa <ku...@igalia.com>
Gerrit-Attention: Johannes Kron <kr...@chromium.org>
Gerrit-Comment-Date: Wed, 22 Oct 2025 06:29:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Johannes Kron <kr...@chromium.org>
satisfied_requirement
unsatisfied_requirement
open
diffy

Johannes Kron (Gerrit)

unread,
Oct 22, 2025, 4:14:44 AM (11 days ago) Oct 22
to Palak Agarwal, Kentaro Hara, Raphael Kubo da Costa, AyeAye, AI Code Reviewer, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, blink-revie...@chromium.org, asvitkine...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, kinuko...@chromium.org, tommyw+w...@chromium.org
Attention needed from Palak Agarwal

Johannes Kron added 2 comments

File third_party/blink/renderer/bindings/core/v8/script_promise_resolver_with_tracker.h
Line 131, Patchset 6 (Latest): LOG(ERROR) << __func__;
Johannes Kron . unresolved

Isn't this called also when the result is kOk?

File third_party/blink/renderer/modules/mediastream/user_media_processor.h
File-level comment, Patchset 6 (Latest):
Johannes Kron . unresolved

Revert the changes to this file.

Open in Gerrit

Related details

Attention is currently required from:
  • Palak Agarwal
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: I882eedf64a0801f780999298b2435d94aa4dca21
    Gerrit-Change-Number: 7062737
    Gerrit-PatchSet: 6
    Gerrit-Owner: Palak Agarwal <agp...@chromium.org>
    Gerrit-Reviewer: Johannes Kron <kr...@chromium.org>
    Gerrit-Reviewer: Palak Agarwal <agp...@chromium.org>
    Gerrit-CC: AI Code Reviewer <peep-gen...@system.gserviceaccount.com>
    Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>
    Gerrit-CC: Kentaro Hara <har...@chromium.org>
    Gerrit-CC: Raphael Kubo da Costa <ku...@igalia.com>
    Gerrit-Attention: Palak Agarwal <agp...@chromium.org>
    Gerrit-Comment-Date: Wed, 22 Oct 2025 08:14:31 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Palak Agarwal (Gerrit)

    unread,
    Oct 22, 2025, 4:50:04 AM (11 days ago) Oct 22
    to Kentaro Hara, Raphael Kubo da Costa, Johannes Kron, AyeAye, AI Code Reviewer, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, blink-revie...@chromium.org, asvitkine...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, kinuko...@chromium.org, tommyw+w...@chromium.org
    Attention needed from Johannes Kron

    Palak Agarwal added 2 comments

    File third_party/blink/renderer/bindings/core/v8/script_promise_resolver_with_tracker.h
    Line 131, Patchset 6: LOG(ERROR) << __func__;
    Johannes Kron . resolved

    Isn't this called also when the result is kOk?

    Palak Agarwal

    I had this added this for local debugging, forgot to remove it.

    File third_party/blink/renderer/modules/mediastream/user_media_processor.h
    File-level comment, Patchset 6:
    Johannes Kron . resolved

    Revert the changes to this file.

    Palak Agarwal

    Done

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Johannes Kron
    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: I882eedf64a0801f780999298b2435d94aa4dca21
      Gerrit-Change-Number: 7062737
      Gerrit-PatchSet: 7
      Gerrit-Owner: Palak Agarwal <agp...@chromium.org>
      Gerrit-Reviewer: Johannes Kron <kr...@chromium.org>
      Gerrit-Reviewer: Palak Agarwal <agp...@chromium.org>
      Gerrit-CC: AI Code Reviewer <peep-gen...@system.gserviceaccount.com>
      Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>
      Gerrit-CC: Kentaro Hara <har...@chromium.org>
      Gerrit-CC: Raphael Kubo da Costa <ku...@igalia.com>
      Gerrit-Attention: Johannes Kron <kr...@chromium.org>
      Gerrit-Comment-Date: Wed, 22 Oct 2025 08:49:33 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: No
      Comment-In-Reply-To: Johannes Kron <kr...@chromium.org>
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Johannes Kron (Gerrit)

      unread,
      Oct 22, 2025, 4:56:50 AM (11 days ago) Oct 22
      to Palak Agarwal, Kentaro Hara, Raphael Kubo da Costa, AyeAye, AI Code Reviewer, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, blink-revie...@chromium.org, asvitkine...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, kinuko...@chromium.org, tommyw+w...@chromium.org
      Attention needed from Palak Agarwal

      Johannes Kron voted Code-Review+1

      Code-Review+1
      Open in Gerrit

      Related details

      Attention is currently required from:
      • Palak Agarwal
      Submit Requirements:
        • requirement satisfiedCode-Coverage
        • requirement is not satisfiedCode-Owners
        • requirement satisfiedCode-Review
        • requirement 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: I882eedf64a0801f780999298b2435d94aa4dca21
        Gerrit-Change-Number: 7062737
        Gerrit-PatchSet: 7
        Gerrit-Owner: Palak Agarwal <agp...@chromium.org>
        Gerrit-Reviewer: Johannes Kron <kr...@chromium.org>
        Gerrit-Reviewer: Palak Agarwal <agp...@chromium.org>
        Gerrit-CC: AI Code Reviewer <peep-gen...@system.gserviceaccount.com>
        Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>
        Gerrit-CC: Kentaro Hara <har...@chromium.org>
        Gerrit-CC: Raphael Kubo da Costa <ku...@igalia.com>
        Gerrit-Attention: Palak Agarwal <agp...@chromium.org>
        Gerrit-Comment-Date: Wed, 22 Oct 2025 08:56:31 +0000
        Gerrit-HasComments: No
        Gerrit-Has-Labels: Yes
        satisfied_requirement
        unsatisfied_requirement
        open
        diffy

        Tony Herre (Gerrit)

        unread,
        Oct 22, 2025, 6:02:59 AM (11 days ago) Oct 22
        to Palak Agarwal, Johannes Kron, Kentaro Hara, Raphael Kubo da Costa, AyeAye, AI Code Reviewer, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, blink-revie...@chromium.org, asvitkine...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, kinuko...@chromium.org, tommyw+w...@chromium.org
        Attention needed from Palak Agarwal

        Tony Herre voted and added 2 comments

        Votes added by Tony Herre

        Code-Review+1

        2 comments

        File third_party/blink/renderer/modules/mediastream/user_media_processor.cc
        Line 106, Patchset 7 (Latest): default:
        Tony Herre . unresolved

        You don't like having exhaustive enum switch statements? Without the 'default' it'll fail to compile when adding a new value, rather than running and crashing.

        Line 1297, Patchset 7 (Parent): blink::LogUserMediaRequestResult(
        Tony Herre . unresolved

        Shouldn't we preserve this? Or is it captured elsewhere?

        Open in Gerrit

        Related details

        Attention is currently required from:
        • Palak Agarwal
        Submit Requirements:
        • requirement satisfiedCode-Coverage
        • requirement satisfiedCode-Owners
        • requirement satisfiedCode-Review
        • requirement is not satisfiedNo-Unresolved-Comments
        • requirement 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: I882eedf64a0801f780999298b2435d94aa4dca21
        Gerrit-Change-Number: 7062737
        Gerrit-PatchSet: 7
        Gerrit-Owner: Palak Agarwal <agp...@chromium.org>
        Gerrit-Reviewer: Johannes Kron <kr...@chromium.org>
        Gerrit-Reviewer: Palak Agarwal <agp...@chromium.org>
        Gerrit-Reviewer: Tony Herre <top...@chromium.org>
        Gerrit-CC: AI Code Reviewer <peep-gen...@system.gserviceaccount.com>
        Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>
        Gerrit-CC: Kentaro Hara <har...@chromium.org>
        Gerrit-CC: Raphael Kubo da Costa <ku...@igalia.com>
        Gerrit-Attention: Palak Agarwal <agp...@chromium.org>
        Gerrit-Comment-Date: Wed, 22 Oct 2025 10:02:43 +0000
        Gerrit-HasComments: Yes
        Gerrit-Has-Labels: Yes
        satisfied_requirement
        unsatisfied_requirement
        open
        diffy

        Palak Agarwal (Gerrit)

        unread,
        Oct 22, 2025, 7:13:50 AM (11 days ago) Oct 22
        to Tony Herre, Johannes Kron, Kentaro Hara, Raphael Kubo da Costa, AyeAye, AI Code Reviewer, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, blink-revie...@chromium.org, asvitkine...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, kinuko...@chromium.org, tommyw+w...@chromium.org

        Palak Agarwal added 2 comments

        File third_party/blink/renderer/modules/mediastream/user_media_processor.cc
        Line 106, Patchset 7: default:
        Tony Herre . resolved

        You don't like having exhaustive enum switch statements? Without the 'default' it'll fail to compile when adding a new value, rather than running and crashing.

        Palak Agarwal

        Done

        Line 1297, Patchset 7 (Parent): blink::LogUserMediaRequestResult(
        Tony Herre . resolved

        Shouldn't we preserve this? Or is it captured elsewhere?

        Palak Agarwal

        This would be captured earlier when the `current_request_info_` would be reset. The request can be reset at `UserMediaProcessor::CancelRequest` and `UserMediaProcessor::StopAllProcessing`, where new logging statements have been added with this change.

        Open in Gerrit

        Related details

        Attention set is empty
        Submit Requirements:
          • requirement satisfiedCode-Coverage
          • requirement satisfiedCode-Owners
          • requirement satisfiedCode-Review
          • requirement 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: I882eedf64a0801f780999298b2435d94aa4dca21
          Gerrit-Change-Number: 7062737
          Gerrit-PatchSet: 8
          Gerrit-Owner: Palak Agarwal <agp...@chromium.org>
          Gerrit-Reviewer: Johannes Kron <kr...@chromium.org>
          Gerrit-Reviewer: Palak Agarwal <agp...@chromium.org>
          Gerrit-Reviewer: Tony Herre <top...@chromium.org>
          Gerrit-CC: AI Code Reviewer <peep-gen...@system.gserviceaccount.com>
          Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>
          Gerrit-CC: Kentaro Hara <har...@chromium.org>
          Gerrit-CC: Raphael Kubo da Costa <ku...@igalia.com>
          Gerrit-Comment-Date: Wed, 22 Oct 2025 11:13:27 +0000
          Gerrit-HasComments: Yes
          Gerrit-Has-Labels: No
          Comment-In-Reply-To: Tony Herre <top...@chromium.org>
          satisfied_requirement
          open
          diffy

          Palak Agarwal (Gerrit)

          unread,
          Oct 24, 2025, 6:37:09 AM (9 days ago) Oct 24
          to Tove Petersson, Tony Herre, Johannes Kron, Kentaro Hara, Raphael Kubo da Costa, AyeAye, AI Code Reviewer, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, blink-revie...@chromium.org, asvitkine...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, kinuko...@chromium.org, tommyw+w...@chromium.org

          Palak Agarwal voted Commit-Queue+2

          Commit-Queue+2
          Open in Gerrit

          Related details

          Attention set is empty
          Submit Requirements:
          • requirement satisfiedCode-Coverage
          • requirement satisfiedCode-Owners
          • requirement satisfiedCode-Review
          • requirement 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: I882eedf64a0801f780999298b2435d94aa4dca21
          Gerrit-Change-Number: 7062737
          Gerrit-PatchSet: 10
          Gerrit-Owner: Palak Agarwal <agp...@chromium.org>
          Gerrit-Reviewer: Johannes Kron <kr...@chromium.org>
          Gerrit-Reviewer: Palak Agarwal <agp...@chromium.org>
          Gerrit-Reviewer: Tony Herre <top...@chromium.org>
          Gerrit-CC: AI Code Reviewer <peep-gen...@system.gserviceaccount.com>
          Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>
          Gerrit-CC: Kentaro Hara <har...@chromium.org>
          Gerrit-CC: Raphael Kubo da Costa <ku...@igalia.com>
          Gerrit-CC: Tove Petersson <to...@chromium.org>
          Gerrit-Comment-Date: Fri, 24 Oct 2025 10:36:34 +0000
          Gerrit-HasComments: No
          Gerrit-Has-Labels: Yes
          satisfied_requirement
          open
          diffy

          Chromium LUCI CQ (Gerrit)

          unread,
          Oct 24, 2025, 6:43:17 AM (9 days ago) Oct 24
          to Palak Agarwal, Tove Petersson, Tony Herre, Johannes Kron, Kentaro Hara, Raphael Kubo da Costa, AyeAye, AI Code Reviewer, Chromium Metrics Reviews, chromium...@chromium.org, blink-revie...@chromium.org, asvitkine...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, kinuko...@chromium.org, tommyw+w...@chromium.org

          Chromium LUCI CQ submitted the change with unreviewed changes

          Unreviewed changes

          7 is the latest approved patch-set.
          The change was submitted with unreviewed changes in the following files:

          ```
          The name of the file: third_party/blink/renderer/modules/mediastream/user_media_processor.cc
          Insertions: 18, Deletions: 5.

          The diff is too large to show. Please review the diff.
          ```

          Change information

          Commit message:
          Split WebRTC.UserMediaRequest.Result2 per request type

          OBSOLETE_HISTOGRAM[WebRTC.UserMediaRequest.Result2]= Split into 3 new
          histograms WebRTC.UserMediaRequest.
          {GetUserMedia|GetDisplayMedia|GetAllScreensMedia}.Result based on
          request type
          Bug: 453600255
          Change-Id: I882eedf64a0801f780999298b2435d94aa4dca21
          Reviewed-by: Tony Herre <top...@chromium.org>
          Commit-Queue: Palak Agarwal <agp...@chromium.org>
          Reviewed-by: Johannes Kron <kr...@chromium.org>
          Cr-Commit-Position: refs/heads/main@{#1534907}
          Files:
          • M third_party/blink/renderer/modules/mediastream/user_media_processor.cc
          • M third_party/blink/renderer/platform/mediastream/webrtc_uma_histograms.cc
          • M third_party/blink/renderer/platform/mediastream/webrtc_uma_histograms.h
          • M tools/metrics/histograms/metadata/web_rtc/histograms.xml
          Change size: M
          Delta: 4 files changed, 39 insertions(+), 20 deletions(-)
          Branch: refs/heads/main
          Submit Requirements:
          • requirement satisfiedCode-Review: +1 by Tony Herre, +1 by Johannes Kron
          Open in Gerrit
          Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
          Gerrit-MessageType: merged
          Gerrit-Project: chromium/src
          Gerrit-Branch: main
          Gerrit-Change-Id: I882eedf64a0801f780999298b2435d94aa4dca21
          Gerrit-Change-Number: 7062737
          Gerrit-PatchSet: 11
          Gerrit-Owner: Palak Agarwal <agp...@chromium.org>
          Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>
          Gerrit-Reviewer: Johannes Kron <kr...@chromium.org>
          Gerrit-Reviewer: Palak Agarwal <agp...@chromium.org>
          Gerrit-Reviewer: Tony Herre <top...@chromium.org>
          Gerrit-CC: AI Code Reviewer <peep-gen...@system.gserviceaccount.com>
          open
          diffy
          satisfied_requirement
          Reply all
          Reply to author
          Forward
          0 new messages