[WebAudio/MIDI] String return types, use StrCat not String::Format [chromium/src : main]

1 view
Skip to first unread message

Michael Wilson (Gerrit)

unread,
May 19, 2026, 5:11:51 PM (5 days ago) May 19
to Mahesh Kannan, Chromium LUCI CQ, chromium...@chromium.org, Hongchan Choi, blink-...@chromium.org, devtools-re...@chromium.org, kinuko...@chromium.org, toyosh...@chromium.org
Attention needed from Mahesh Kannan

Michael Wilson added 1 comment

Patchset-level comments
File-level comment, Patchset 2 (Latest):
Michael Wilson . resolved

PTAL (the CI failure seems unrelated)

Open in Gerrit

Related details

Attention is currently required from:
  • Mahesh Kannan
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement 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: I099c5a023280ba8a3ff82545766f6f1c47faa5bb
Gerrit-Change-Number: 7859242
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Wilson <mjwi...@chromium.org>
Gerrit-Reviewer: Mahesh Kannan <kmah...@google.com>
Gerrit-Reviewer: Michael Wilson <mjwi...@chromium.org>
Gerrit-CC: Hongchan Choi <hong...@chromium.org>
Gerrit-Attention: Mahesh Kannan <kmah...@google.com>
Gerrit-Comment-Date: Tue, 19 May 2026 21:11:39 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Mahesh Kannan (Gerrit)

unread,
May 22, 2026, 7:18:34 PM (2 days ago) May 22
to Code Review Nudger, Chromium LUCI CQ, chromium...@chromium.org, Hongchan Choi, blink-...@chromium.org, devtools-re...@chromium.org, kinuko...@chromium.org, toyosh...@chromium.org
Attention needed from Michael Wilson

Mahesh Kannan voted and added 2 comments

Votes added by Mahesh Kannan

Code-Review+1

2 comments

Patchset-level comments
Mahesh Kannan . resolved

LGTM overall

File third_party/blink/renderer/modules/webaudio/audio_node.cc
Line 210, Patchset 2 (Latest): StrCat(
{"({output=[index:", String::Number(output_index),
", type:", Handler().NodeTypeName(), ", handler:0x",
String::Format("%" PRIXPTR, reinterpret_cast<uintptr_t>(&Handler())),
"]} --> {input=[index:", String::Number(input_index),
", type:", destination->Handler().NodeTypeName(), ", handler:0x",
String::Format("%" PRIXPTR,
reinterpret_cast<uintptr_t>(&destination->Handler())),
"]})"}));
Mahesh Kannan . unresolved

Can we consider using `StringBuilder` here to make it more readable?

Open in Gerrit

Related details

Attention is currently required from:
  • Michael Wilson
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: I099c5a023280ba8a3ff82545766f6f1c47faa5bb
    Gerrit-Change-Number: 7859242
    Gerrit-PatchSet: 2
    Gerrit-Owner: Michael Wilson <mjwi...@chromium.org>
    Gerrit-Reviewer: Mahesh Kannan <kmah...@google.com>
    Gerrit-Reviewer: Michael Wilson <mjwi...@chromium.org>
    Gerrit-CC: Code Review Nudger <android-build...@prod.google.com>
    Gerrit-CC: Hongchan Choi <hong...@chromium.org>
    Gerrit-Attention: Michael Wilson <mjwi...@chromium.org>
    Gerrit-Comment-Date: Fri, 22 May 2026 23:18:23 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Michael Wilson (Gerrit)

    unread,
    May 22, 2026, 8:47:32 PM (2 days ago) May 22
    to Mahesh Kannan, Code Review Nudger, Chromium LUCI CQ, chromium...@chromium.org, Hongchan Choi, blink-...@chromium.org, devtools-re...@chromium.org, kinuko...@chromium.org, toyosh...@chromium.org

    Michael Wilson voted and added 2 comments

    Votes added by Michael Wilson

    Commit-Queue+2

    2 comments

    Patchset-level comments
    Mahesh Kannan . resolved

    LGTM overall

    Michael Wilson

    Thank you!

    File third_party/blink/renderer/modules/webaudio/audio_node.cc
    Line 210, Patchset 2 (Latest): StrCat(
    {"({output=[index:", String::Number(output_index),
    ", type:", Handler().NodeTypeName(), ", handler:0x",
    String::Format("%" PRIXPTR, reinterpret_cast<uintptr_t>(&Handler())),
    "]} --> {input=[index:", String::Number(input_index),
    ", type:", destination->Handler().NodeTypeName(), ", handler:0x",
    String::Format("%" PRIXPTR,
    reinterpret_cast<uintptr_t>(&destination->Handler())),
    "]})"}));
    Mahesh Kannan . resolved

    Can we consider using `StringBuilder` here to make it more readable?

    Michael Wilson

    StringBuilder may have to reallocate when appending the results, which would be worse for performance. StrCat can pre-compute the size and perform a single allocation. Thus, for performance reasons, I prefer to keep this as StrCat.

    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: I099c5a023280ba8a3ff82545766f6f1c47faa5bb
      Gerrit-Change-Number: 7859242
      Gerrit-PatchSet: 2
      Gerrit-Owner: Michael Wilson <mjwi...@chromium.org>
      Gerrit-Reviewer: Mahesh Kannan <kmah...@google.com>
      Gerrit-Reviewer: Michael Wilson <mjwi...@chromium.org>
      Gerrit-CC: Code Review Nudger <android-build...@prod.google.com>
      Gerrit-CC: Hongchan Choi <hong...@chromium.org>
      Gerrit-Comment-Date: Sat, 23 May 2026 00:47:21 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: Yes
      Comment-In-Reply-To: Mahesh Kannan <kmah...@google.com>
      satisfied_requirement
      open
      diffy

      Chromium LUCI CQ (Gerrit)

      unread,
      May 22, 2026, 10:09:06 PM (2 days ago) May 22
      to Mahesh Kannan, Code Review Nudger, chromium...@chromium.org, Hongchan Choi, blink-...@chromium.org, devtools-re...@chromium.org, kinuko...@chromium.org, toyosh...@chromium.org

      Chromium LUCI CQ submitted the change

      Change information

      Commit message:
      [WebAudio/MIDI] String return types, use StrCat not String::Format

      Returning const String is an anti-pattern because it prevents eliding
      copies.  By using StrCat we can return const char* in functions called
      by the logging functions and avoid these additional copies.  Similarly,
      returning class members by const reference (not just const) can avoid
      copies in some cases.

      This should cause no functional change.
      Change-Id: I099c5a023280ba8a3ff82545766f6f1c47faa5bb
      Reviewed-by: Mahesh Kannan <kmah...@google.com>
      Commit-Queue: Michael Wilson <mjwi...@chromium.org>
      Cr-Commit-Position: refs/heads/main@{#1635349}
      Files:
      • M third_party/blink/renderer/modules/webaudio/audio_handler.cc
      • M third_party/blink/renderer/modules/webaudio/audio_handler.h
      • M third_party/blink/renderer/modules/webaudio/audio_node.cc
      • M third_party/blink/renderer/modules/webaudio/audio_node.h
      • M third_party/blink/renderer/modules/webaudio/inspector_web_audio_agent.cc
      • M third_party/blink/renderer/modules/webmidi/midi_port.h
      • M third_party/blink/renderer/platform/audio/audio_destination.cc
      Change size: M
      Delta: 7 files changed, 29 insertions(+), 25 deletions(-)
      Branch: refs/heads/main
      Submit Requirements:
      • requirement satisfiedCode-Review: +1 by Mahesh Kannan
      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: I099c5a023280ba8a3ff82545766f6f1c47faa5bb
      Gerrit-Change-Number: 7859242
      Gerrit-PatchSet: 3
      Gerrit-Owner: Michael Wilson <mjwi...@chromium.org>
      Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>
      Gerrit-Reviewer: Mahesh Kannan <kmah...@google.com>
      Gerrit-Reviewer: Michael Wilson <mjwi...@chromium.org>
      open
      diffy
      satisfied_requirement
      Reply all
      Reply to author
      Forward
      0 new messages